Skip to content

service: Cap number of backends included in monitor message #36394

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

Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Dec 5, 2024

Limit the number of backends included in the ServiceUpsertMessage to avoid performing significant amount of allocations for services with lots of backends and churn. The number of omitted backends is included in the monitor message.

This is the second part of addressing a significant memory "leak" issue with services that have lots of backends. The root problem was the Hubble ring buffer holding onto lots of ServiceUpsertNotifications, which is fixed in #36391. After 36391 the remaining use-case for the service messages is just cilium-dbg monitor, so there isn't too much of a point to doing lots of work to generate the message.

@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Dec 5, 2024
Limit the number of backends included in the ServiceUpsertMessage to avoid
performing significant amount of allocations for services with lots of backends
and churn. The number of omitted backends is included in the monitor message.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/service-monitor-event-cap-backends branch from f6d7073 to 08a343e Compare December 10, 2024 11:40
@joamaki joamaki marked this pull request as ready for review December 11, 2024 10:17
@joamaki joamaki requested review from a team as code owners December 11, 2024 10:17
@joamaki
Copy link
Contributor Author

joamaki commented Dec 11, 2024

/test

@joamaki joamaki added the needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch label Dec 12, 2024
@joamaki joamaki enabled auto-merge December 12, 2024 14:37
Copy link
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

Makes sense. Ty!

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks @joamaki !

@joamaki joamaki added this pull request to the merge queue Dec 13, 2024
Merged via the queue into cilium:main with commit 60f639c Dec 13, 2024
80 checks passed
@joamaki joamaki deleted the pr/joamaki/service-monitor-event-cap-backends branch December 13, 2024 09:50
@joestringer joestringer added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Dec 13, 2024
@pippolo84 pippolo84 mentioned this pull request Dec 16, 2024
9 tasks
@pippolo84 pippolo84 added backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. and removed needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Dec 16, 2024
@github-actions github-actions bot added backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. and removed backport-pending/1.16 The backport for Cilium 1.16.x for this PR is in progress. labels Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.16 The backport for Cilium 1.16.x for this PR is done. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants