fix memory leaking in dfp by doujiang24 · Pull Request #31433 · envoyproxy/envoy

doujiang24 · 2023-12-18T16:08:05Z

Commit Message:
fix #30999

when host is resolved to IP A, A will be added into mutable_cross_priority_host_map_
when host is resolved to IP B, the address saved in logical_host_ will be updated to IP B.
when host is TTL expired, only IP B will be removed in mutable_cross_priority_host_map_, IP A is leaking in mutable_cross_priority_host_map_.

this PR do fix the leaking issue, but I'm not sure if it is a proper fix.

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: doujiang24 <doujiang24@gmail.com>

adisuissa · 2024-01-02T13:55:23Z

Assigning Alyssa as codeowner
/assign @alyssawilk

alyssawilk

thanks for tackling this. I'm not terribly familiar with this area of code so tagging @zuercher as well. My Thoughts below
/wait

alyssawilk · 2024-01-02T18:15:10Z

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

-    std::unique_ptr<Upstream::HostVector>& hosts_added) {
+    const Extensions::Common::DynamicForwardProxy::DnsHostInfoSharedPtr& host_info) {
  Upstream::LogicalHostSharedPtr emplaced_host;
+  bool founded = false;


host_found?

okay, will fix it later :)

alyssawilk · 2024-01-02T18:34:02Z

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

+                                   dummy_lb_endpoint_);
+    // will add host into priorityState again, with the new address.
+  }
+  updatePriorityState(hosts, {});


do we actually need to do the add/remove here? I don't think we really use priority for DFP, and the address update grabs a lock and updates (I think) globally so I'm not sure if the update is necessary. strict DNS appears to simply do the priority update on host addition and skip on resolution change.
One nice way to test e2e would be to do an e2e test where we start with a bad resolved IP via cache and a short DNS refresh timeout, and verify that the update makes it to the worker threads (crib off of test/extensions/filters/http/dynamic_forward_proxy/proxy_filter_integration_test.cc tests for cache files)

I don't think we really use priority for DFP

yep, I do think so too. can we remove the priority entirely? if yes, it would be much simpler.

do we actually need to do the add/remove here

yes, I do think so, we have to keep tracking the correct address in priority, otherwise,
when host is TTL expired, the old(before change) address will leaking in priority.

One nice way to test e2e would be to do an e2e test

okay, I will have a try later, after we figure out the proper way to fix it.

the different between logical DNS and strict DNS is that - strict DNS could remove all of the IPs from priority, but logic DNS could not.
since there is only the latest IP in logic DNS when removing the expired host:

envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

Line 268 in 7a9543d

runRemoveCallbacks(host);

I worry a little about the double-call to updatePriorityState -- I think it means theres's a brief period with no host in the priority set. Perhaps that's ok.

An alternative might be to check for the existing host but always create a new one for the updated IP and then make a single call to updatePriorityState with the new/old host.

yeah if it's not super complicated it'd be nice to fix updatePriortySet properly so it handles a remove/add of a single host.

okay, already updated to a single call to updatePriorityState.

@zuercher can you do first pass here? I didn't expect the single call to updatePriorityState to require an add/remove of host instead of updateAddress and I'm not really sure which is better

hm, not sure what happened to @zuercher on this review, but I'm wondering if it's possible to do an update while not replacing the host, still doing the address resolution change option
/wait-any

Oh, nope, seems hard to reach it, it needs two logicalHost, sine we want a single call to updatePriorityState

yanjunxiang-google · 2024-01-02T20:37:42Z

source/common/upstream/upstream_impl.cc


  for (const auto& host : hosts_added) {
    mutable_cross_priority_host_map_->insert({addressToString(host->address()), host});
+    ENVOY_LOG(debug, "cross_priority host map, adding: {}", addressToString(host->address()));


Based on your problem description, it looks like the problem is that when the host is initially resolved into IP_A, IP_A is inserted into the mutable_cross_priority_host_map_ here. And later on if the host is resolved into IP_B, then IP_B is inserted here. However, the above (Line 841) mutable_cross_priority_host_map_->erase(addressToString(host->address())) for IP_A is somehow not called.

Intuitively, I would think when the DNS resolving returns a different address, MainPrioritySetImpl::updateCrossPriorityHostMap() will be called with both host_added and host_removed set. But, it looks like host_removed is empty here. Could you please share which code path is causing that?

Based on your problem description, it looks like the problem is that when the host is initially resolved into IP_A, IP_A is inserted into the mutable_cross_priority_host_map_ here. And later on if the host is resolved into IP_B, then IP_B is inserted here. However, the above (Line 841) mutable_cross_priority_host_map_->erase(addressToString(host->address())) for IP_A is somehow not called.

yep, that's it.

Intuitively, I would think when the DNS resolving returns a different address, MainPrioritySetImpl::updateCrossPriorityHostMap() will be called with both host_added and host_removed set. But, it looks like host_removed is empty here. Could you please share which code path is causing that?

Nope, in the old implementation, it simply do the priority update on host addition and skip on host subtraction, similar to strict DNS, as @alyssawilk said.
And I think that't the problem.
since it only remove the latest IP in logic DNS when removing an expired host:

envoy/source/extensions/common/dynamic_forward_proxy/dns_cache_impl.cc

Line 268 in 7a9543d

runRemoveCallbacks(host);

really I don't think having an IP based host map makes sense for DFP hosts, as we can have many "unique" hosts refered to by the same address. Pinging @wbpcode for thoughts - if it still lived in the LB classes I'd nix it for the DFP cluster but that's not an easy option nowadays.

Signed-off-by: doujiang24 <doujiang24@gmail.com>

doujiang24 · 2024-01-09T01:31:49Z

/retest

doujiang24 · 2024-01-11T11:22:21Z

/retest

phlax · 2024-01-11T11:24:30Z

the mobile (compile-time-options) job is passing - there was a ci glitch (since fixed) that meant it didnt always complete

alyssawilk · 2024-01-11T14:20:50Z

ping @zuercher

zuercher

I think this seems reasonable. I suppose the old version had an efficiency when many hosts were added or removed at once. If that's a concern we could lift the loop in startPreInit to happen under lock.

doujiang24 · 2024-01-17T01:57:48Z

Thanks @zuercher

If that's a concern

I think it might be fine, since it only happens while cluster initize, means low rate usually.

alyssawilk · 2024-02-13T19:52:55Z

source/extensions/clusters/dynamic_forward_proxy/cluster.cc

-  }
-  hosts_added->emplace_back(emplaced_host);
+  ASSERT(hosts_added.size() > 0);
+  updatePriorityState(hosts_added, hosts_removed);


Actually looking through this code belatedly, doesn't updatePriorityState cause updateHosts cause drainOrCloseConnPools(host, ConnectionPool::DrainBehavior::DrainAndDelete) on the "old host"? I think this could be a pretty bad latency hit where we want to lame-duck drain rather than force-remove drain.

Oh, I see the problem.
But, I think it might be reasonable to force-remove drain.
When dns resolve result changes, force-remove drain the old host should be expected?

The root cause is the dfp cluster use logical DNS mode instead of strict dns mode, it may lead to host change even when dns resolve result not changes.

I haven't found a simple solution yet.
Maybe we can recommend people to use the new sub_cluster_config?
Or, any suggestions?

DrainExistingConnections would maybe be OK, but DrainAndDelete is too aggressive IMO as it will force-close connections.

Either way I'm thinking we should probably roll this back as I don't think the prior leak had caused many problems in production and (I think) this ended up being an unexpected non-flag-guarded behavioral change.

I am also OK writing a test to verify if this resulted in the behavioral change I think it did before we go that route but @yanavlasov @ggreenway for their thoughts.

This bug is first reported in #30999, and it leads to a large leak, according to the profile data.
But, according @rbtz-openai's update, brotli decompression is the root cause for the large leak.
I also agree that it just leaking in extremely case, with very low rate.
Revert this patch might be a proper choice, since it's hard to keep the old behaviour to fix this extremely low rate leak.
Maybe @rbtz-openai could reconfirm that this dfp leak could be ignored in your production case?

For "DrainAndDelete will force-close connections":
After reading the comments of DrainExistingConnections/DrainAndDelete, and the code of drainConnections(drain_behavior), I didn't find force-close connections.
I think I missed it somewhere. Would you please give a guideline? i.e. some keywords of the code.
Thanks very much~

Yeah so I added tests (#32710) and while I'm not sure why (having looked at the code) we're not force-closing, it does appear we're not force closing. I'll do a little more investigation before pushing for rollback. Thanks for looking into this as well!

Ah ok, figured out what's wrong with my test.

OK so this PR both did fix the memory leak described, but also result in a functional change to connection lifetimes.

By the Envoy change guidelines I think it should not have landed without flag protection (because data plane change). The particular change is problematic for Envoy Mobile. Given the memory leak is long standing and hadn't been largely I'm inclined to roll back to prior (buggy) state than keep this (also buggy for E-M) state. We could also flag guard the change if there's anyone who wants the memory leak fix and doesn't mind the connectivity change?

cc @yanavlasov @RyanTheOptimist for second opinions.

for context, this fixes the leak in the mutable_cross_priority_host_map_ as described but does so by updating the host in worker threads, which results in an (new, undesirable for E-M) connection pool drain and fresh non-warmed-CWND connection being used for subsequent streams.

Ideally we'd have a solution which handled both but until we have a solution the question is which bug to prefer. I'm inclined to stick with the old one but would also be amenable into making it a flag-guarded "behavioral change + fix". Unclear if the correct behavior for a server is to drain on DNS refresh - it does do a better job of respecting DNS TTL but can also result in a lot of connection churn and suboptimal bandwidth.

Yeah, I agree with @alyssawilk that since this included a non-flag protected behavior change (especially since it broke mobile), we should roll this back. We can go ahead and roll forward with flag protection, of course, but we should return to a stable state first, I think.

okay, I see. Rolling back does make sense. If someone really need this fix, we could create a new PR with a flag guard.

Adding some DFP tests of what happens on DNS resolution. Prior to #31433 both these tests passed, as we didn't drain the host when we re-resolved the IP. Now the second test fails, as the host is drained (and the newly resolved IP doesn't work by design). Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

doujiang24 added 2 commits December 16, 2023 22:29

debug log for dfp leaking

aede710

Signed-off-by: doujiang24 <doujiang24@gmail.com>

wip: fix leaking.

2524e3b

Signed-off-by: doujiang24 <doujiang24@gmail.com>

doujiang24 requested review from alyssawilk and mattklein123 as code owners December 18, 2023 16:08

doujiang24 added 3 commits December 19, 2023 09:29

fix lock.

5d557f6

Signed-off-by: doujiang24 <doujiang24@gmail.com>

fix updatePriorityState.

d1087b5

Signed-off-by: doujiang24 <doujiang24@gmail.com>

fix lock.

e199002

Signed-off-by: doujiang24 <doujiang24@gmail.com>

doujiang24 force-pushed the fix-dfp-leaking branch from 1be5a50 to e199002 Compare December 19, 2023 12:26

fix test.

7a9543d

Signed-off-by: doujiang24 <doujiang24@gmail.com>

doujiang24 mentioned this pull request Dec 20, 2023

Memory leak in dynamic forward proxy mode w/ DNS Cache #30999

Closed

KBaichoo assigned yanjunxiang-google Dec 20, 2023

repokitteh-read-only bot assigned alyssawilk Jan 2, 2024

alyssawilk reviewed Jan 2, 2024

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 2, 2024

yanjunxiang-google reviewed Jan 2, 2024

View reviewed changes

founded => host_found

1cd6039

Signed-off-by: doujiang24 <doujiang24@gmail.com>

repokitteh-read-only bot removed the waiting label Jan 4, 2024

doujiang24 added 2 commits January 5, 2024 14:02

remove & add priorityState in batch

5b2b92b

Signed-off-by: doujiang24 <doujiang24@gmail.com>

erase first

3f71e0f

Signed-off-by: doujiang24 <doujiang24@gmail.com>

alyssawilk assigned zuercher Jan 8, 2024

Merge branch 'main' into fix-dfp-leaking

702039c

phlax added this to the v1.29.0 milestone Jan 11, 2024

alyssawilk removed this from the v1.29.0 milestone Jan 16, 2024

repokitteh-read-only bot added the waiting:any label Jan 16, 2024

zuercher approved these changes Jan 16, 2024

View reviewed changes

repokitteh-read-only bot removed the waiting:any label Jan 16, 2024

alyssawilk merged commit f20b6ca into envoyproxy:main Jan 18, 2024

doujiang24 deleted the fix-dfp-leaking branch January 18, 2024 23:12

alyssawilk reviewed Feb 13, 2024

View reviewed changes

alyssawilk mentioned this pull request Mar 5, 2024

dfp: add e2e resolution tests #32710

Merged

Conversation

doujiang24 commented Dec 18, 2023

Uh oh!

adisuissa commented Jan 2, 2024

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doujiang24 commented Jan 9, 2024

Uh oh!

doujiang24 commented Jan 11, 2024

Uh oh!

phlax commented Jan 11, 2024

Uh oh!

alyssawilk commented Jan 11, 2024

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

doujiang24 commented Jan 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants