optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks #672

zhlsunshine · 2023-08-30T11:38:44Z

optimize the function load_balance_for_hostname to avoid starting multiple background on-demand DNS tasks.

… more than one background on-demand DNS task

istio-policy-bot · 2023-08-30T11:38:46Z

😊 Welcome @zhlsunshine! This is either your first contribution to the Istio ztunnel repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

src/state.rs

…the first request

src/state.rs

zhlsunshine · 2023-09-07T10:03:11Z

Hi @howardjohn & @hzxuzhonghu, is there any comment for this PR? thanks!

hzxuzhonghu · 2023-09-07T12:17:31Z

Will take a look

hzxuzhonghu

I donot fully understand the idea.

I think we just need CachedResolvedDNSMap which stores the hash map of hostname to SingleFlight

The single flight only need to store the Notify

src/state.rs

zhlsunshine · 2023-09-08T04:31:10Z

I donot fully understand the idea.

I think we just need CachedResolvedDNSMap which stores the hash map of hostname to SingleFlight

The single flight only need to store the Notify

Hi @hzxuzhonghu, I think I can give more explains here for these 2 AtomicBools:
Although state is Arc<RwLock>, all the reading and writing operations themselves for ProxyState are atomic just in the code block, such as read CachedResolvedDNSMap via get_cached_resolve_dns_for_hostname and write CachedResolvedDNSMap via set_cached_resolve_dns_for_hostname, so we need these 2 AtomicBools to make sure that only the first arrive request to write the hashmap, and others need to wait for the first completed.

zhlsunshine · 2023-09-18T09:34:48Z

Hi @howardjohn & @hzxuzhonghu & @nmittler, is there anything that I can do to make it approved? Thanks!

src/state.rs

hzxuzhonghu

In addition, we can have a test for it

zhlsunshine · 2023-09-19T04:25:47Z

In addition, we can have a test for it

Hi @hzxuzhonghu, sure, I added a concurrency requests test named build_concurrency_requests_unknown_dest for this change!

…reslovDNS

zhlsunshine · 2023-11-03T08:15:58Z

Hi @howardjohn, @hzxuzhonghu and @nmittler, there is a change for this PR based on @nmittler 's suggestion, creating a new struct named DnsResolver to holds all DNS resolving relates to simplify the ProxyState, and DnsResolver is part feature of DemandProxyState. Could you please help to review it again? Thanks in advance!

src/state.rs

nmittler · 2023-11-03T13:28:39Z

src/state.rs

@@ -333,20 +570,17 @@ impl DemandProxyState {
        let workload_uid = workload.uid.to_owned();
        let hostname = workload.hostname.to_owned();
        metrics.as_ref().on_demand_dns.get_or_create(&labels).inc();
-        let rdns = match state.get_ips_for_hostname(&workload.hostname) {
+        let rdns = match state.dns_resolver.get_ips_for_hostname(&hostname) {


Can this entire method (load_balance_for_hostname) just be moved to DnsResolver? All of this should be internal details of it, I think.

Hi @nmittler, as you know DnsResolver is part of DemandProxyState, and it needs to modify itself on its 2 HashMaps, so it's mutable reference. If moving load_balance_for_hostname into DnsResolver, it would impact that the whole DemandProxyState need to be a mutable reference as well. So hope it can be as it is now. Does it make sense?

I was actually concerned when I saw that DemandProxyState.resolve_on_demand_dns and DemandProxyState.load_balance_for_hostname both working around &self to get &mut self. Not sure when that was introduced, but it seems a bit hacky and should probably be fixed if possible. Perhaps the issue is that we have some methods that needlessly require &mut self(e.g. if you lock an object that you write to, you don't need mut)?

@howardjohn I may be off here, since my rust is a little rusty at this point :) ... thoughts?

zhlsunshine · 2023-11-03T15:44:09Z

Hi @nmittler, as we discussed, it becomes more simplicity.

nmittler · 2023-11-03T15:45:47Z

src/state.rs

    // by_hostname is a map from hostname to resolved IP addresses for now.
    //
    // in a future with support for per-pod DNS resolv.conf settings we may need
    // to change this to a map from source workload uid to resolved IP addresses.
+    #[serde(skip_serializing)]
    by_hostname: HashMap<String, ResolvedDns>,


Maybe just call this field resolved?

I can change it, how about resolved_hostname?

nmittler · 2023-11-03T15:52:08Z

src/state.rs

+    ) -> Option<ResolvedDns> {
+        // optimize so that if multiple requests to the same hostname come in at the same time,
+        // we don't start more than one background on-demand DNS task
+        let is_the_first_req = self.get_singleflight_by_hostname(hostname.to_owned());


I don't think this is quite right. This code should be something like this:

async fn resolve_host( &self, hostname: &String, workload: &Workload, ) -> Option<ResolvedDns> { // Look up the host in the local cache. If it's there, // just return it now. if let Some(result) = self.get_ips_for_hostname(hostname) { return Some(result) } // Perform the resolution for the host and block until it // completes. Take care to only allow a single request to be // in-flight at a time. Other threads will just wait for the first // request to complete. match self.get_or_create_notify(hostname) { (notify, true) => { // First request, perform the resolution and notify // all other threads when resolution is complete. self.resolve_on_demand_dns(workload).await; notify.notify_waiters(); // Now that we've notified all waiters, we can safely // remove the notify for this host. Any future requests // Should just get it from the local cache. self.remove_notify(hostname) } (notify, false) => { // Already in progress, just wait for the result. notify.notified().await; } } // At this point, the local cache has been populated. // Just return the cached contents. return self.get_ips_for_hostname(hostname) }

Hi @nmittler, there are 2 mutable borrows occurs in both self.get_or_create_notify (first mutable borrow) and self.resolve_on_demand_dns (second mutable borrow), so I have to use this let element = self.get_cached_resolve_dns_for_hostname(hostname); to avoid this problem by getting the element from HashMap via read lock.
However, I can change get_singleflight_by_hostname into get_or_create_notify. Does it make sense?

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler · 2023-11-03T21:04:20Z

@zhlsunshine I created #722, which re-works this a bit based on some things we discussed. PTAL.

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler · 2023-11-06T13:48:14Z

@zhlsunshine could we step back for a moment and discuss the motivation for this PR? Are you seeing a real issue with DNS performance, specifically due to concurrent requests for the same host? I just want to make sure we're solving a real problem here.

Co-authored-by: nmittler <nmittler@gmail.com>

zhlsunshine · 2023-11-07T06:09:29Z

@zhlsunshine could we step back for a moment and discuss the motivation for this PR? Are you seeing a real issue with DNS performance, specifically due to concurrent requests for the same host? I just want to make sure we're solving a real problem here.

Hi @nmittler, frankly, I'm focusing on some validations of ambient mesh in CSP environment in recent, what I'm sure about is that the original code has the problem: there may be multiple DNS resolving for the same hostname in concurrent requests.
BTW, I have no more comment for your rework PR#722. And I also insists on that you should be a co-author of this PR thanks to your help and comments. ^_^

nmittler · 2023-11-09T16:13:49Z

@zhlsunshine just to be clear, you haven't actually seen performance problems in a deployed system? My concern is fundamentally about the complexity of the solution. I'd like to see that the problem is real before we optimize for it.

@howardjohn @hzxuzhonghu any thoughts?

howardjohn · 2023-11-10T01:44:14Z

I agree with Nate

…

On Thu, Nov 9, 2023 at 10:14 AM Nathan Mittler ***@***.***> wrote: @zhlsunshine <https://github.com/zhlsunshine> just to be clear, you haven't actually seen performance problems in a deployed system? My concern is fundamentally about the complexity of the solution. I'd like to see that the problem is real before we optimize for it. @howardjohn <https://github.com/howardjohn> @hzxuzhonghu <https://github.com/hzxuzhonghu> any thoughts? — Reply to this email directly, view it on GitHub <#672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXKQHA3WWYB256NJ5ATYDT6MTAVCNFSM6AAAAAA4EL76HWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBUGEZTANZTGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

zhlsunshine · 2023-11-10T02:43:52Z

@zhlsunshine just to be clear, you haven't actually seen performance problems in a deployed system? My concern is fundamentally about the complexity of the solution. I'd like to see that the problem is real before we optimize for it.

@howardjohn @hzxuzhonghu any thoughts?

Hi @nmittler, yes, I haven't. But multiple requests with the same hostname given in concurrency and whose resolved IP addresses are not in the DNS resolved cache, then this situation leads to the same number of times of DNS resolving operations for the same hostname, it's really waste, so this optimization should be valid even we do not test the performance for it.

nmittler · 2023-11-10T17:42:19Z

@zhlsunshine

Hi @nmittler, yes, I haven't. But multiple requests with the same hostname given in concurrency and whose resolved IP addresses are not in the DNS resolved cache, then this situation leads to the same number of times of DNS resolving operations for the same hostname, it's really waste, so this optimization should be valid even we do not test the performance for it.

I understand what you're trying to optimize for. What I'm suggesting is that it may not be a huge issue in practice. I'd rather not add this type of complexity until we actually have a customer that is being impacted.

howardjohn · 2023-11-10T19:54:43Z

One thing to note is the behavior currently is equal to without DNS proxy. So it's not like this is fixing a regression, purely a performance benefit , and the intent of the proxy is not performance.

…

On Fri, Nov 10, 2023, 9:42 AM Nathan Mittler ***@***.***> wrote: @zhlsunshine <https://github.com/zhlsunshine> Hi @nmittler <https://github.com/nmittler>, yes, I haven't. But multiple requests with the same hostname given in concurrency and whose resolved IP addresses are not in the DNS resolved cache, then this situation leads to the same number of times of DNS resolving operations for the same hostname, it's really waste, so this optimization should be valid even we do not test the performance for it. I understand what you're trying to optimize for. What I'm suggesting is that it may not be a huge issue in practice. I'd rather not add this type of complexity until we actually have a customer that is being impacted. — Reply to this email directly, view it on GitHub <#672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEYGXM3PZ3N2OV4FSFUXPDYDZRQLAVCNFSM6AAAAAA4EL76HWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBWGE2TONBQGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

zhlsunshine · 2023-11-24T03:04:15Z

Hi @howardjohn, besides the performance change, we also did some code refactor for the DNS resolver to make it more clear, does it make sense?

istio-testing · 2023-12-19T22:04:11Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

howardjohn · 2024-05-28T23:08:01Z

#1098

optimize the load_balance_for_hostname to make sure that do not start…

d8b9c9f

… more than one background on-demand DNS task

zhlsunshine requested a review from a team as a code owner August 30, 2023 11:38

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 30, 2023

zhlsunshine added 2 commits August 30, 2023 20:26

add AtomicBool operation for async fn load_balance_for_hostname

91130c5

make gen for change

5ad4095

hzxuzhonghu reviewed Aug 31, 2023

View reviewed changes

src/state.rs Outdated Show resolved Hide resolved

add some comments for optimization parts

a1a59e9

zhlsunshine changed the title ~~optimize the function load_balance_for_hostname to avoid starting multiple DNS task~~ optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks Aug 31, 2023

improve the logic for resolving DNS tasks in load_balance_for_hostname

1c1bc97

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 1, 2023

zhlsunshine added 2 commits September 1, 2023 18:42

set more longer timeout

a4808d6

fix issue for other concurrency requests get into the same branch as …

a109fa4

…the first request

hzxuzhonghu reviewed Sep 4, 2023

View reviewed changes

src/state.rs Outdated Show resolved Hide resolved

optimization to aviod the CPU wasting when waiting

014e4ff

hzxuzhonghu reviewed Sep 7, 2023

View reviewed changes

src/state.rs Outdated Show resolved Hide resolved

src/state.rs Outdated Show resolved Hide resolved

src/state.rs Outdated Show resolved Hide resolved

change the map element insert way by using or_insert

0d862ff

howardjohn assigned nmittler Sep 11, 2023

hzxuzhonghu reviewed Sep 19, 2023

View reviewed changes

src/state.rs Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Sep 19, 2023

View reviewed changes

zhlsunshine added 2 commits September 19, 2023 09:47

Merge branch 'master' of https://github.com/istio/ztunnel into reslovDNS

29c3c36

change the fields name for SingleFlight and add test case

de37627

zhlsunshine added 2 commits September 19, 2023 12:53

make gen for code changes

1149b83

Merge branch 'master' of https://github.com/zhlsunshine/ztunnel into …

66bfa6b

…reslovDNS

zhlsunshine added 2 commits November 3, 2023 16:02

re-struct the DnsResolver

657a4a2

make gen to fix test error

c67bbd5

nmittler reviewed Nov 3, 2023

View reviewed changes

zhlsunshine added 2 commits November 3, 2023 21:48

remove pub key words for some methods

77cc7ce

go on the simplification by remove SingleFlight sturct based on comments

0654230

nmittler reviewed Nov 3, 2023

View reviewed changes

zhlsunshine added 2 commits November 4, 2023 00:33

change the function name for checking Notify hashmap

d15363d

change hashmap by_hostname into resolved_hostname

ee6d91a

nmittler added a commit to nmittler/ztunnel that referenced this pull request Nov 3, 2023

Avoid redundant DNS requests

bc859dd

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler mentioned this pull request Nov 3, 2023

Avoid redundant DNS requests #722

Closed

nmittler added a commit to nmittler/ztunnel that referenced this pull request Nov 4, 2023

Avoid redundant DNS requests

52aa0fa

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler added a commit to nmittler/ztunnel that referenced this pull request Nov 4, 2023

Avoid redundant DNS requests

326b42c

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler added a commit to nmittler/ztunnel that referenced this pull request Nov 4, 2023

Avoid redundant DNS requests

bda68d5

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

nmittler added a commit to nmittler/ztunnel that referenced this pull request Nov 4, 2023

Avoid redundant DNS requests

272c7b6

This modifies the approach described in istio#672. Attempting to clarify some things we had discussed offline.

further simplication for DNS resolving.

a1b5eff

Co-authored-by: nmittler <nmittler@gmail.com>

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 19, 2023

howardjohn closed this May 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks #672

optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks #672

zhlsunshine commented Aug 30, 2023

istio-policy-bot commented Aug 30, 2023

zhlsunshine commented Sep 7, 2023

hzxuzhonghu commented Sep 7, 2023

hzxuzhonghu left a comment

zhlsunshine commented Sep 8, 2023

zhlsunshine commented Sep 18, 2023

hzxuzhonghu left a comment

zhlsunshine commented Sep 19, 2023

zhlsunshine commented Nov 3, 2023

nmittler Nov 3, 2023

zhlsunshine Nov 3, 2023

nmittler Nov 3, 2023 •

edited

zhlsunshine commented Nov 3, 2023

nmittler Nov 3, 2023

zhlsunshine Nov 3, 2023

nmittler Nov 3, 2023 •

edited

zhlsunshine Nov 3, 2023 •

edited

nmittler commented Nov 3, 2023

nmittler commented Nov 6, 2023

zhlsunshine commented Nov 7, 2023

nmittler commented Nov 9, 2023

howardjohn commented Nov 10, 2023 via email

zhlsunshine commented Nov 10, 2023

nmittler commented Nov 10, 2023

howardjohn commented Nov 10, 2023 via email

zhlsunshine commented Nov 24, 2023

istio-testing commented Dec 19, 2023

howardjohn commented May 28, 2024

optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks #672

optimize the function load_balance_for_hostname to avoid starting multiple DNS tasks #672

Conversation

zhlsunshine commented Aug 30, 2023

istio-policy-bot commented Aug 30, 2023

zhlsunshine commented Sep 7, 2023

hzxuzhonghu commented Sep 7, 2023

hzxuzhonghu left a comment

Choose a reason for hiding this comment

zhlsunshine commented Sep 8, 2023

zhlsunshine commented Sep 18, 2023

hzxuzhonghu left a comment

Choose a reason for hiding this comment

zhlsunshine commented Sep 19, 2023

zhlsunshine commented Nov 3, 2023

nmittler Nov 3, 2023

Choose a reason for hiding this comment

zhlsunshine Nov 3, 2023

Choose a reason for hiding this comment

nmittler Nov 3, 2023 • edited

Choose a reason for hiding this comment

zhlsunshine commented Nov 3, 2023

nmittler Nov 3, 2023

Choose a reason for hiding this comment

zhlsunshine Nov 3, 2023

Choose a reason for hiding this comment

nmittler Nov 3, 2023 • edited

Choose a reason for hiding this comment

zhlsunshine Nov 3, 2023 • edited

Choose a reason for hiding this comment

nmittler commented Nov 3, 2023

nmittler commented Nov 6, 2023

zhlsunshine commented Nov 7, 2023

nmittler commented Nov 9, 2023

howardjohn commented Nov 10, 2023 via email

zhlsunshine commented Nov 10, 2023

nmittler commented Nov 10, 2023

howardjohn commented Nov 10, 2023 via email

zhlsunshine commented Nov 24, 2023

istio-testing commented Dec 19, 2023

howardjohn commented May 28, 2024

nmittler Nov 3, 2023 •

edited

nmittler Nov 3, 2023 •

edited

zhlsunshine Nov 3, 2023 •

edited