Skip to content

hubble: Remove support for Service{Upsert,Delete}Notification #36391

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

hubble: Remove support for Service{Upsert,Delete}Notification #36391

wants to merge 1 commit into from

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Dec 5, 2024

These events were not used by Hubble and on large clusters with lots of backends used up significant amounts of memory in the hubble ring buffer.

Service upsert and delete events are no longer emitted by Hubble as they were unused and used up significant amount of memory for services with many backends.

@joamaki
hubble: Remove support for Service{Upsert,Delete}Notification
cbe878e 
These events were not used by Hubble and on large clusters with lots of
backends used up significant amounts of memory in the hubble ring buffer.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 5, 2024
@github-actions github-actions bot added the hubble-cli PRs or GitHub issues related with hubble-cli label Dec 5, 2024
@joamaki joamaki added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Dec 5, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 5, 2024
@joamaki joamaki mentioned this pull request Dec 5, 2024
Merged
@joamaki joamaki marked this pull request as ready for review December 6, 2024 12:50
@joamaki joamaki requested review from a team as code owners December 6, 2024 12:50
@joamaki joamaki requested a review from rolinh December 6, 2024 12:50
@joestringer
Copy link
Member

@cilium/sig-hubble does it make sense to stage this for deprecation in v1.17 and removal in v1.18?

@joestringer joestringer added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Dec 9, 2024
@michi-covalent
Copy link
Contributor

does it make sense to stage this for deprecation in v1.17 and removal in v1.18?

yeah i think this makes sense. might be better to:

  • only mark it deprecated in .proto file without removing the functionality in v1.17
  • remove proto fields and the functionality in v1.18

but i'll make hubble folks make the call 🚀🙏

@glibsm
Copy link
Member

glibsm commented Dec 9, 2024

According to the notes from last week when we met to discuss this, we decided to mark it deprecated in the API to signal our intention to potentially remove, but instead of deleting the functionality outright we were going to turn it off by default.

Perhaps in a future release we can remove it altogether, but we probably want to be more cautious from the beginning.

Thank you @joamaki for putting up the PR. So we don't keep you on the hook, what do you think about one of the Hubble team members commandeering this PR and getting it merged in a slightly different state?

@joamaki
Copy link
Contributor Author

joamaki commented Dec 10, 2024

According to the notes from last week when we met to discuss this, we decided to mark it deprecated in the API to signal our intention to potentially remove, but instead of deleting the functionality outright we were going to turn it off by default.

Perhaps in a future release we can remove it altogether, but we probably want to be more cautious from the beginning.

Thank you @joamaki for putting up the PR. So we don't keep you on the hook, what do you think about one of the Hubble team members commandeering this PR and getting it merged in a slightly different state?

Well that of course sounds great to me!

@aanm aanm removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Dec 18, 2024
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 18, 2025
@glibsm glibsm removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 21, 2025
@joamaki joamaki closed this Jan 22, 2025
@joamaki joamaki mentioned this pull request May 12, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolinh rolinh Awaiting requested review from rolinh rolinh is a code owner automatically assigned from cilium/sig-hubble

Assignees
No one assigned
Labels
hubble-cli PRs or GitHub issues related with hubble-cli release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joamaki @joestringer @michi-covalent @glibsm @aanm