Skip to content

cgroup manager: introduce hive cell #32799

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
Merged
merged 4 commits into from
Jun 4, 2024
Merged

cgroup manager: introduce hive cell #32799

merged 4 commits into from
Jun 4, 2024

Conversation

mhofstetter
Copy link
Member

@mhofstetter mhofstetter commented May 30, 2024 •
edited
Loading

This PR introduces a Hive Cell for the cgroup manager.

Please review the individual commits.

  • introduce hive cell for cgroup manager
  • use injected logger in cgroup manager
  • remove unused method GetParentPodMetadata
  • introduce noop cgroup manager implementation

@mhofstetter mhofstetter added kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. area/modularization Relates to code modularization and maintenance. labels May 30, 2024
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/cgroup-manager-cell branch 2 times, most recently from 7d469d3 to 710887d Compare May 30, 2024 15:54
@mhofstetter
Copy link
Member Author

/test

@mhofstetter mhofstetter marked this pull request as ready for review May 30, 2024 17:30
@mhofstetter mhofstetter requested review from a team as code owners May 30, 2024 17:30
kaworu
kaworu approved these changes May 31, 2024
View reviewed changes
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.

Hubble changes LGTM

@mhofstetter mhofstetter force-pushed the pr/mhofstetter/cgroup-manager-cell branch from 710887d to 8a056e6 Compare May 31, 2024 15:29
@mhofstetter
Copy link
Member Author

rebased to main to resolve conflicts

@mhofstetter
Copy link
Member Author

/test

@mhofstetter
cgroup: introduce hive cell for cgroup manager
6bae613 
This commit introduces a hive cell for the cgroup manager and replaces
the explicit initialization (and shutdown) in the daemon.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
cgroup: use injected logger in cgroup manager
b4b5825 
This commit introduces an Hive Dependency from the cgroup manager to
a logger and replaces the static logger.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
daemon: remove unused method GetParentPodMetadata
e04ec81 
This commit removes the unused daemon method `GetParentPodMetadata` that
introduced an unnecessary dependency to the CGroupManager.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
cgroup: introduce noop cgroup manager implementation
fe2dd05 
Currently, the implementation of the CGroupManager also handles the cases
if socket lb tracing isn't enabled or the setup failed to lookup the
cgroup path provider.

To ease the understanding of the actual implementation (aside from testing
and the mentioned cases where the feature is disabled), this commit
introduces a `noopCgroupManager` that is provided as implementation in cases
where the feature isn't enabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter force-pushed the pr/mhofstetter/cgroup-manager-cell branch from 8a056e6 to fe2dd05 Compare June 3, 2024 07:27
@mhofstetter
Copy link
Member Author

rebased to main to resolve conflicts

dylandreimerink
dylandreimerink approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

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

Nice work. Thanks!

@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

Removing review-request for Louis as we have everything covered - thanks 🙏

@mhofstetter mhofstetter removed the request for review from ldelossa June 4, 2024 09:59
@mhofstetter mhofstetter added dont-merge/blocked Another PR must be merged before this one. and removed dont-merge/blocked Another PR must be merged before this one. labels Jun 4, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 4, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Jun 4, 2024
Merged via the queue into cilium:main with commit 20c1f36 Jun 4, 2024
@mhofstetter mhofstetter deleted the pr/mhofstetter/cgroup-manager-cell branch June 4, 2024 10:16
@mhofstetter mhofstetter mentioned this pull request Jun 5, 2024
Merged
julianwiedmann added a commit to julianwiedmann/cilium that referenced this pull request Oct 10, 2024
@julianwiedmann
docs: kpr: update error message regarding SocketLB tracing
de4b6f7 
cilium#32799 changed the error message,
adjust the documentation.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@julianwiedmann julianwiedmann mentioned this pull request Oct 10, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 10, 2024
@julianwiedmann @qmonnet
docs: kpr: update error message regarding SocketLB tracing
ea9d3c6 
#32799 changed the error message,
adjust the documentation.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
tklauser pushed a commit that referenced this pull request Oct 22, 2024
@julianwiedmann @tklauser
docs: kpr: update error message regarding SocketLB tracing
dcf7ce3 
[ upstream commit ea9d3c6 ]

#32799 changed the error message,
adjust the documentation.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2024
@julianwiedmann @tklauser
docs: kpr: update error message regarding SocketLB tracing
09b949f 
[ upstream commit ea9d3c6 ]

#32799 changed the error message,
adjust the documentation.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: Tobias Klauser <tobias@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaworu kaworu kaworu approved these changes

@joamaki joamaki joamaki approved these changes

@dylandreimerink dylandreimerink dylandreimerink approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

+1 more reviewer

@doniacld doniacld doniacld approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/modularization Relates to code modularization and maintenance. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mhofstetter @kaworu @joamaki @dylandreimerink @ysksuzuki @doniacld @julianwiedmann