Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fqdn: Fix GC for dead IPs on live names over limit #22510

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Dec 2, 2022

A user observed that Cilium's FQDN name cache can accumulate many IP
addresses over time for DNS names that have frequent IP recycling (such as
Amazon S3). In particular, the --tofqdns-endpoint-max-ip-per-hostname
maximum did not seem to apply to such names as long as there is an
active connection for one of the IPs associated with the name. This
could cause an increase in memory usage and identity allocations in the
cluster, which consumes policymap entries as well.

The problem was that the FQDN garbage collection routines would apply
the limits to active IPs for active names and clean up inactive IPs and
inactive names, but it would not apply to the inactive IPs that
correspond to names with other active IPs. These would hence slip
through the garbage collection selection and remain in the cache
indefinitely.

This patch fixes it by including the inactive IPs along with active IPs
in the list of live names to evaluate, which then ensures they are
considered for deletion if they exceed the max-ip-per-hostname limits.
Those limits are then enforced based on how recently the IPs have been
used, favouring IPs with more recently active connections over IPs that
have not been recently used by applications on the node.

Related: #13914
Related: #19452

Improve garbage collection for FQDNs particularly with high-churn IP names such as Amazon S3.

@joestringer joestringer added affects/v1.10 This issue affects v1.10 branch affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch needs-backport/1.12 release-note/bug This PR fixes an issue in a previous release of Cilium. labels Dec 2, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 2, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.5 Dec 2, 2022
@joestringer joestringer added the sig/agent Cilium agent related. label Dec 2, 2022
A user observed that Cilium's FQDN name cache can accumulate many IP
addresses over time for DNS names that have frequent IP recycling (such as
Amazon S3). In particular, the "--tofqdns-endpoint-max-ip-per-hostname"
maximum did not seem to apply to such names as long as there is an
active connection for one of the IPs associated with the name. This
could cause an increase in memory usage and identity allocations in the
cluster, which consumes policymap entries as well.

The problem was that the FQDN garbage collection routines would apply
the limits to active IPs for active names and clean up inactive IPs and
inactive names, but it would not apply to the inactive IPs that
correspond to names with other active IPs. These would hence slip
through the garbage collection selection and remain in the cache
indefinitely.

This patch fixes it by including the inactive IPs along with active IPs
in the list of live names to evaluate, which then ensures they are
considered for deletion if they exceed the max-ip-per-hostname limits.
Those limits are then enforced based on how recently the IPs have been
used, favouring IPs with more recently active connections over IPs that
have not been recently used by applications on the node.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer force-pushed the submit/zombie-limits-for-inactive-ips-of-active-names branch from 12c4be6 to dcd64c1 Compare December 2, 2022 05:43
@joestringer
Copy link
Member Author

/test

@joestringer
Copy link
Member Author

joestringer commented Dec 2, 2022

Not sure what happened with test-runtime not-failing failure, maybe I triggered the build too soon. Will re-kick it.

@joestringer
Copy link
Member Author

/test-runtime

@joestringer joestringer marked this pull request as ready for review December 2, 2022 09:12
@joestringer joestringer requested review from a team as code owners December 2, 2022 09:12
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix makes sense to me, thanks. There seems to be a pattern popping up with these bugs. It seems we are missing a sort of "matrix" / "permutations" testing, like how do we handle GC when there's a mix alive & dead IPs associated with names or a mix of lookups & zombies. I see in this PR we are adding that sort of test, but I wonder if we have this coverage in other tests.

@joestringer
Copy link
Member Author

There is a bit of that yeah. There's not so many permutations though and it seemed like the rest was already covered. Particularly when it comes to enforcing the limit, it's more just about "alive" names and how the limits get enforced against those. Once we do the GC() call we just get back the lists of alive + dead names and clean up the dead ones, so I think at this point we probably have all the coverage we need for this? I'm open to looking into some other pattern if there's a particular test you have in mind.

@joestringer
Copy link
Member Author

joestringer commented Dec 4, 2022

Copy link
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just adding another data point, i was hitting this issue with connections to S3 in one of my clusters. i applied this patch, and the number of connection entries in cilium fqdn cache list has been kept under tofqdns-endpoint-max-ip-per-hostname for the past 3 days 🥳

@joestringer joestringer merged commit 335b0ae into cilium:master Dec 5, 2022
@joestringer joestringer deleted the submit/zombie-limits-for-inactive-ips-of-active-names branch December 5, 2022 21:20
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.12 in 1.12.5 Dec 14, 2022
@joestringer joestringer moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.5 Dec 15, 2022
@joestringer joestringer added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/v1.10 This issue affects v1.10 branch affects/v1.11 This issue affects v1.11 branch affects/v1.12 This issue affects v1.12 branch backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/agent Cilium agent related.
Projects
No open projects
1.12.5
Backport done to v1.12
Development

Successfully merging this pull request may close these issues.

None yet

3 participants