Refactor dns #1247

LiZhenCheng9527 · 2025-02-21T03:35:55Z

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
Refactored the DNS resolution module to decouple it from the CDS in Kernel-Native mode. This facilitates the reuse of the DNS resolution module in Dual-Engine mode.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

codecov · 2025-02-21T07:32:28Z

Codecov Report

Attention: Patch coverage is 62.93333% with 139 lines in your changes missing coverage. Please review.

Project coverage is 45.65%. Comparing base (726da17) to head (2f054db).
Report is 99 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/controller/ads/dns.go	71.35%	46 Missing and 13 partials ⚠️
pkg/dns/dns.go	34.48%	52 Missing and 5 partials ⚠️
pkg/dns/utils.go	78.46%	10 Missing and 4 partials ⚠️
pkg/controller/ads/ads_controller.go	50.00%	7 Missing and 1 partial ⚠️
pkg/controller/controller.go	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/controller/controller.go	`0.00% <0.00%> (ø)`
pkg/controller/ads/ads_controller.go	`68.05% <50.00%> (-5.63%)`	⬇️
pkg/dns/utils.go	`78.46% <78.46%> (ø)`
pkg/dns/dns.go	`51.54% <34.48%> (-22.66%)`	⬇️
pkg/controller/ads/dns.go	`71.35% <71.35%> (ø)`

... and 14 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938599d...2f054db. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LiZhenCheng9527 · 2025-02-24T06:10:42Z

PTAL
/cc @hzxuzhonghu

hzxuzhonghu · 2025-02-24T06:41:30Z

pkg/dns/ads_handler.go

I would suggest we do provide a dns pkg fundamental capability and make ads/workload bussiness related part in their own pkg

hzxuzhonghu · 2025-02-24T06:45:31Z

pkg/dns/ads_handler.go

+}
+
+func (adsResolver *AdsDnsResolver) refreshAdsWorker() {
+	for adsResolver.refreshAdsDns() {


I do believe such arch should be shared with workload

hzxuzhonghu · 2025-02-26T07:40:23Z

pkg/dns/dns.go

-	}
-
-	return ready
+type PendingResolveDomain struct {


How do you plean to support dual-engine then?
If you are going to add another struct here, wouldn't that still coupling

hzxuzhonghu · 2025-02-26T07:41:38Z

pkg/controller/ads/dns.go

+)
+
+// adsDnsResolver is DNS resolver of Kernel Native
+type AdsDnsResolver struct {


Nit:

Suggested change

type AdsDnsResolver struct {

type DnsResolver struct {

hzxuzhonghu · 2025-02-26T07:42:18Z

pkg/controller/ads/dns.go

+	dnsRefreshQueue workqueue.TypedDelayingInterface[any]
+}
+
+func NewAdsDnsResolver(adsCache *AdsCache) (*AdsDnsResolver, error) {


It is under ads pkg, so we can remove the redundant

Suggested change

func NewAdsDnsResolver(adsCache *AdsCache) (*AdsDnsResolver, error) {

func NewDnsResolver(adsCache *AdsCache) (*DnsResolver, error) {

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

PR Overview

This PR refactors and decouples the DNS resolution module from the CDS in Kernel-Native mode to support reuse in Dual-Engine mode. The changes introduce a fake DNS server for testing, a new ADS DNS Resolver, controller updates to use the new resolver, and significant refactoring in the DNS resolver implementation.

Added a fakeDNSServer implementation in pkg/dns/utils.go.
Created a new AdsDnsResolver in pkg/controller/ads/dns.go and updated the controller to utilize it.
Refactored pkg/dns/dns.go by cleaning up internal structures and updating locking and resolution logic.

Reviewed Changes

File	Description
pkg/dns/utils.go	Introduces a fake DNS server for testing purposes.
pkg/controller/ads/dns.go	Implements a new ADS DNS Resolver for Kernel-Native mode.
pkg/controller/controller.go	Updates the controller to use the new ADS DNS Resolver.
pkg/dns/dns.go	Refactors the DNS resolver implementation, updating locks and API.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Copilot · 2025-02-27T07:14:21Z

pkg/dns/utils.go

+func (s *fakeDNSServer) SetHosts(domain string, surfix int) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.hosts[dns.Fqdn(domain)] = surfix


The parameter name 'surfix' appears to be a typo. It should likely be renamed to 'suffix' for clarity.

Suggested change

func (s *fakeDNSServer) SetHosts(domain string, surfix int) {

s.mu.Lock()

defer s.mu.Unlock()

s.hosts[dns.Fqdn(domain)] = surfix

func (s *fakeDNSServer) SetHosts(domain string, suffix int) {

s.mu.Lock()

defer s.mu.Unlock()

s.hosts[dns.Fqdn(domain)] = suffix

Copilot · 2025-02-27T07:14:22Z

pkg/dns/dns.go

 	r.RLock()
-	_, exist := r.cache[dr.domainName]
+	entry.addresses = addrs
 	r.RUnlock()
-	// if the domain is no longer watched, no need to refresh it
-	if !exist {
-		return true
-	}
-	r.resolve(dr)
-	r.adsCache.ClusterCache.Flush()
-	return true
-}



A write operation is performed under a read lock, which can lead to race conditions. Consider acquiring a full write lock when updating 'entry.addresses'.

Copilot

PR Overview

This PR refactors the DNS resolution module to decouple it from the CDS for Kernel-Native mode, enabling reuse in Dual-Engine mode. Key changes include:

Addition of a fake DNS server in pkg/dns/utils.go for testing and simulation.
Introduction of an Ads DNS resolver and its integration in pkg/controller/ads/dns.go and pkg/controller/controller.go.
Updates to the DNS resolver implementation in pkg/dns/dns.go to simplify the interface and remove dependencies on AdsCache.

Reviewed Changes

File	Description
pkg/dns/utils.go	Adds a fake DNS server implementation for testing DNS responses and host mapping.
pkg/controller/ads/dns_test.go	Includes tests verifying DNS cluster overwrite, concurrent cache updates, and domain handling.
pkg/controller/ads/dns.go	Introduces the Ads DNS resolver that integrates with the cluster cache and DNS module.
pkg/controller/controller.go	Modifies controller start-up to use the new Ads DNS resolver for Kernel-Native mode.
pkg/dns/dns.go	Refactors the DNS resolver to remove unused parameters and adjust the refresh loop handling.

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Copilot · 2025-02-28T06:38:27Z

pkg/dns/utils.go

+func (s *fakeDNSServer) SetHosts(domain string, surfix int) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.hosts[dns.Fqdn(domain)] = surfix


The parameter name 'surfix' appears to be a misspelling; consider renaming it to 'suffix' for clarity.

Suggested change

func (s *fakeDNSServer) SetHosts(domain string, surfix int) {

s.mu.Lock()

defer s.mu.Unlock()

s.hosts[dns.Fqdn(domain)] = surfix

func (s *fakeDNSServer) SetHosts(domain string, suffix int) {

s.mu.Lock()

defer s.mu.Unlock()

s.hosts[dns.Fqdn(domain)] = suffix

Copilot · 2025-02-28T06:38:28Z

pkg/dns/dns.go

+	_, ttl, err := r.resolve(e.Domain)
+	if err != nil {
+		log.Errorf("failed to dns resolve: %v", err)
+		return false
+	}
+	if ttl > e.RefreshRate {
+		ttl = e.RefreshRate
+	}
+	if ttl == 0 {
+		ttl = DeRefreshInterval
+	}
+	r.RefreshQueue.AddAfter(e, ttl)
+	r.AdsDnsChan <- e.Domain
+


In the refreshDns function, returning false upon a DNS resolution error stops the resolver loop. Consider handling errors with a retry mechanism instead of terminating the refresh loop.

pkg/dns/dns.go

hzxuzhonghu · 2025-02-28T07:33:55Z

pkg/dns/dns.go

+		ttl = DeRefreshInterval
+	}
+	r.RefreshQueue.AddAfter(e, ttl)
+	r.AdsDnsChan <- e.Domain


what does this mean?

Notify adsDnsResolver that the domain has completed dns resolution

pkg/controller/ads/dns.go

hzxuzhonghu · 2025-02-28T07:41:26Z

pkg/controller/ads/dns.go

+
+func (adsResolver *AdsDnsResolver) StartAdsDnsResolver(stopCh <-chan struct{}) {
+	go adsResolver.startAdsResolver()
+	go adsResolver.dnsResolver.StartDnsResolver()


I feel this is messy cand confusing, it is unclear what is AdsResolver and what is dns resolver

hzxuzhonghu

Please ADD SOME COMMENTS，it is very hard to review

pkg/controller/ads/dns.go

hzxuzhonghu · 2025-03-24T02:10:17Z

pkg/controller/ads/dns.go

+	domains := getPendingResolveDomain(cds)
+	hostNames := make(map[string]struct{})
+
+	for k := range domains {
+		hostNames[k] = struct{}{}


If you need convert after calling getPendingResolveDomain, why not directly return what you want

hzxuzhonghu · 2025-03-24T02:12:12Z

pkg/controller/ads/dns.go

+		clusterName := cluster.GetName()
+		hostInfo := getHostInfo(cluster)
+		info := clusterInfo{
+			info: hostInfo,


can you explain why store ip address?

hzxuzhonghu · 2025-03-24T02:14:44Z

pkg/controller/ads/dns.go

+func (r *dnsController) startDnsController() {
+	for clusters := range r.Clusters {
+		go func(clusters []*clusterv3.Cluster) {
+			r.resolveDomains(clusters)


I think this is not safe running async in separate routines,

think about this case:

t0: a resolveDomains thread

t1: another resolveDomains

If t1 is run before t0, even some step.The state machine would be broken

Signed-off-by: LiZhenCheng9527 <[email protected]>

pkg/controller/ads/ads_controller.go

pkg/controller/ads/dns.go

hzxuzhonghu · 2025-03-25T02:49:40Z

pkg/controller/ads/dns.go

+	// Handle cds updates when a hostname completes resolution
+	go r.refreshAdsWorker(stopCh)
+	// Consumption of clusters to be resolved.
+	go r.startDnsController()


It looks confusing with StartDnsResolver.

Already change function name

pkg/controller/ads/ads_controller.go

hzxuzhonghu · 2025-03-25T02:59:11Z

pkg/controller/ads/dns.go

+
+func (r *dnsController) startDnsController() {
+	for clusters := range r.Clusters {
+		r.resolveDomains(clusters)


this is not actually resolve domains, confusing with the resolver. This is to get the pending domains, right?

Yes. I will change function name.

hzxuzhonghu · 2025-03-25T03:00:48Z

pkg/controller/ads/dns.go

+	for _, cluster := range cds {
+		clusterName := cluster.GetName()
+		info := getHostName(cluster)
+		r.pendingClusterInfo[clusterName] = info


From this line, i can understand pendingClusterInfo is not a good name. please rename

I changed it to pendingHostnamesByCluster

pkg/controller/ads/dns.go

hzxuzhonghu · 2025-03-25T03:03:26Z

pkg/controller/ads/dns.go

+		// Already have record in dns cache
+		if addresses != nil {
+			r.updateClusters(v, addresses)
+			go r.cache.ClusterCache.Flush()


Is this right? You are getting one domain resolved address and then flush the cluster

hzxuzhonghu · 2025-03-25T03:04:47Z

pkg/controller/ads/dns.go

+			domain := <-r.dnsResolver.DnsChan
+			pendingDomain := r.getClustersByDomain(domain)
+			addrs := r.dnsResolver.GetDNSAddresses(domain)
+			r.updateClusters(pendingDomain, addrs)


How many go routines aare there callling cluster flush? There is already one above in resolveDomains

Signed-off-by: LiZhenCheng9527 <[email protected]>

LiZhenCheng9527 · 2025-03-27T09:34:50Z

/cc @hzxuzhonghu

Copilot

Pull Request Overview

This PR refactors the DNS resolution module to decouple it from the CDS in Kernel-Native mode, enabling its reuse in Dual-Engine mode.

Introduces a fake DNS server implementation for testing purposes.
Updates the DNSResolver API including channel names, caching, and refresh logic.
Integrates the updated DNS module with the ADS controller and adjusts related tests.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
pkg/dns/utils.go	Added fakeDNSServer implementation with helper methods
pkg/dns/dns.go	Refactored DNSResolver API, caching and refresh mechanisms
pkg/controller/controller.go	Updated dependency injection for DNS resolver integration
pkg/controller/ads/dns*.go	Modified ADS controller to use the new DNS resolver methods
pkg/controller/ads/dns_test.go	Updated tests to verify the refactored DNS resolution and ADS behavior

Comments suppressed due to low confidence (1)

pkg/dns/dns.go:300

The parameter name 'time' conflicts with the imported 'time' package. Consider renaming it to 'delay' or 'duration' to avoid shadowing.

func (r *DNSResolver) AddDomainInQueue(info *DomainInfo, time time.Duration) {

pkg/dns/utils.go

pkg/dns/dns.go

pkg/controller/ads/ads_controller.go

pkg/controller/ads/dns.go

hzxuzhonghu · 2025-03-27T11:14:28Z

pkg/controller/ads/dns.go

+}
+
+func (r *dnsController) processDomains(cds []*clusterv3.Cluster) {
+	domains, hostNames := getPendingResolveDomain(cds)


Are these two maps with same keys? If so can you pass domains to RemoveUnwatchDomain

The pendingResolveDomain structure is in the ads package. If domains are transferred to RemoveUnwantedDomain, cyclic invoking of the package is triggered.
The pendingResolveDomain is placed in the ads package because it contains clusters, which is unique to the kernel-native mode. So I wanted to put it in my ads bag.

hzxuzhonghu · 2025-03-27T11:21:28Z

pkg/controller/ads/dns.go

+		// Already have record in dns cache
+		if addresses != nil {
+			// k(domain) has been resolved, triggering refreshWorker.
+			r.dnsResolver.DnsChan <- k


I would suggest not write a same channel in two packages

pkg/controller/ads/dns.go

Signed-off-by: LiZhenCheng9527 <[email protected]>

hzxuzhonghu

/lgtm

kmesh-bot · 2025-04-08T03:13:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LiZhenCheng9527 added this to the v1.1 milestone Feb 21, 2025

kmesh-bot added the kind/enhancement New feature or request label Feb 21, 2025

kmesh-bot requested review from hzxuzhonghu and Okabe-Rintarou-0 February 21, 2025 03:36

kmesh-bot added the size/XL label Feb 21, 2025

LiZhenCheng9527 force-pushed the refactor-dns branch from 82a7658 to 7bfdd21 Compare February 21, 2025 07:03

hzxuzhonghu reviewed Feb 24, 2025

View reviewed changes

kmesh-bot added size/XXL and removed size/XL labels Feb 25, 2025

LiZhenCheng9527 force-pushed the refactor-dns branch from 8406980 to 74aa0d2 Compare February 25, 2025 11:02

hzxuzhonghu reviewed Feb 26, 2025

View reviewed changes

hzxuzhonghu requested review from Copilot and removed request for Copilot February 27, 2025 06:36

Copilot AI reviewed Feb 27, 2025

View reviewed changes

hzxuzhonghu requested review from Copilot and removed request for Copilot February 27, 2025 07:02

Copilot AI reviewed Feb 27, 2025

View reviewed changes

LiZhenCheng9527 force-pushed the refactor-dns branch from 74aa0d2 to 9bb253d Compare February 27, 2025 09:13

hzxuzhonghu requested a review from Copilot February 28, 2025 06:37

Copilot AI reviewed Feb 28, 2025

View reviewed changes

hzxuzhonghu reviewed Feb 28, 2025

View reviewed changes

hzxuzhonghu reviewed Mar 24, 2025

View reviewed changes

LiZhenCheng9527 added 2 commits March 24, 2025 19:23

Optimized cds refresh processing

f4397ea

Signed-off-by: LiZhenCheng9527 <[email protected]>

add more comments

c4cb113

Signed-off-by: LiZhenCheng9527 <[email protected]>

LiZhenCheng9527 force-pushed the refactor-dns branch from 967c3e1 to c4cb113 Compare March 24, 2025 11:48

hzxuzhonghu reviewed Mar 25, 2025

View reviewed changes

fix go lint error

44d5b2b

Signed-off-by: LiZhenCheng9527 <[email protected]>

LiZhenCheng9527 force-pushed the refactor-dns branch from 929fb83 to 44d5b2b Compare March 27, 2025 07:18

change function name

282fdda

Signed-off-by: LiZhenCheng9527 <[email protected]>

kmesh-bot requested a review from hzxuzhonghu March 27, 2025 09:34

hzxuzhonghu requested a review from Copilot March 27, 2025 09:37

Copilot AI reviewed Mar 27, 2025

View reviewed changes

pkg/dns/utils.go Show resolved Hide resolved

pkg/dns/dns.go Show resolved Hide resolved