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?:

kmesh-bot · 2025-02-21T03:36:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from lizhencheng9527. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

pkg/OWNERS

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

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

Codecov Report

Attention: Patch coverage is 66.01307% with 104 lines in your changes missing coverage. Please review.

Project coverage is 44.89%. Comparing base (726da17) to head (b9ce631).
Report is 50 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/dns/dns.go	33.33%	53 Missing and 5 partials ⚠️
pkg/controller/ads/dns.go	81.33%	17 Missing and 11 partials ⚠️
pkg/dns/utils.go	78.46%	10 Missing and 4 partials ⚠️
pkg/controller/controller.go	0.00%	4 Missing ⚠️

❌ Your patch check has failed because the patch coverage (66.01%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Files with missing lines	Coverage Δ
pkg/controller/controller.go	`0.00% <0.00%> (ø)`
pkg/dns/utils.go	`78.46% <78.46%> (ø)`
pkg/controller/ads/dns.go	`81.33% <81.33%> (ø)`
pkg/dns/dns.go	`51.54% <33.33%> (-22.66%)`	⬇️

... and 8 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...b9ce631. 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 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

YaoZengzeng · 2025-02-28T07:52:56Z

@LiZhenCheng9527 add a design doc first would be better for review.

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

pkg/dns/dns.go

hzxuzhonghu · 2025-03-06T07:34:07Z

pkg/controller/ads/dns.go

+		}
+		r.dnsResolver.AddDomainIntoRefreshQueue(domainInfo, 0)
+	}
+	go r.refreshAdsWorker(domains)


when will this thread exit?

hzxuzhonghu · 2025-03-06T07:37:00Z

pkg/controller/ads/dns.go

+func (r *dnsController) startDnsController() {
+	rateLimiter := make(chan struct{}, dns.MaxConcurrency)
+	for clusters := range r.Clusters {
+		rateLimiter <- struct{}{}


I am not sure how does the ratelimiter effect, because in the dns pkg, it always run in order

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

LiZhenCheng9527 force-pushed the refactor-dns branch from 9bb253d to 883dd18 Compare March 4, 2025 10:32

LiZhenCheng9527 added 4 commits March 4, 2025 18:36

refector dns resolver

35176df

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

modify UT of dns

4b08473

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

go lint

c41a2e1

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

delete unused struct

72c9a32

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

LiZhenCheng9527 force-pushed the refactor-dns branch from 883dd18 to 292da49 Compare March 4, 2025 10:51

address comments

b9fca75

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

LiZhenCheng9527 force-pushed the refactor-dns branch from 292da49 to b9fca75 Compare March 5, 2025 01:37

LiZhenCheng9527 mentioned this pull request Mar 5, 2025

refactor dns resolver of kmesh #1266

Open

Minimize the number of public elements in the DNSResolver

b9ce631

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

hzxuzhonghu reviewed Mar 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dns #1247

Refactor dns #1247

LiZhenCheng9527 commented Feb 21, 2025

kmesh-bot commented Feb 21, 2025

codecov bot commented Feb 21, 2025 •

edited

Loading

LiZhenCheng9527 commented Feb 24, 2025 •

edited

Loading

hzxuzhonghu Feb 24, 2025

hzxuzhonghu Feb 24, 2025

hzxuzhonghu Feb 26, 2025

hzxuzhonghu Feb 26, 2025

hzxuzhonghu Feb 26, 2025

Copilot AI left a comment

Copilot AI Feb 27, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Feb 27, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI left a comment

Copilot AI Feb 28, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Feb 28, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

hzxuzhonghu Feb 28, 2025

LiZhenCheng9527 Mar 4, 2025

hzxuzhonghu Feb 28, 2025

YaoZengzeng commented Feb 28, 2025

hzxuzhonghu Mar 6, 2025

hzxuzhonghu Mar 6, 2025

	func NewAdsDnsResolver(adsCache AdsCache) (AdsDnsResolver, error) {
	func NewDnsResolver(adsCache AdsCache) (DnsResolver, error) {

Refactor dns #1247

Are you sure you want to change the base?

Refactor dns #1247

Conversation

LiZhenCheng9527 commented Feb 21, 2025

kmesh-bot commented Feb 21, 2025

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

LiZhenCheng9527 commented Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Copilot AI Feb 28, 2025

Choose a reason for hiding this comment

Copilot AI Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaoZengzeng commented Feb 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 21, 2025 •

edited

Loading

LiZhenCheng9527 commented Feb 24, 2025 •

edited

Loading