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

Revert "Merge pull request #133 from danielfoehrKn/fix/set-systemd-cgroup-driver" #144

Merged
merged 1 commit into from Feb 16, 2024

Conversation

danatsap
Copy link
Contributor

This reverts commit 3725157, reversing changes made to 1d7c3bd.

How to categorize this PR?

/area os
/kind bug
/os garden-linux

What this PR does / why we need it:

#133 seems to cause problems that were not caught during testing the changes.

  • sometimes the cgroup driver is changed on a host with already running containers --> prevents the node from joining the cluster.

I would suggest to revert this commit and avoid any more changes to the cgroup setup until the gardenlet is fixed using cgroup v2. Most, if not all, supported Operating Systems run an image with cgroupsv2 mounted by default, so it should not be long until the gardenlet can centrally enforce cgroup v2 without each extension having to care for legacy nodes that still run cgroup v1 (and introducing potential bugs in the process)

Which issue(s) this PR fixes:
Fixes #141

Special notes for your reviewer:

Release note:

reverts commit 37251573e5225c2f4ed6afa4a61f674a8efec245 

…stemd-cgroup-driver"

This reverts commit 3725157, reversing
changes made to 1d7c3bd.
@danatsap danatsap requested review from a team as code owners January 31, 2024 12:38
@gardener-robot gardener-robot added needs/review Needs review area/os Operation system related kind/bug Bug os/garden-linux Related to Garden Linux OS labels Jan 31, 2024
@gardener-robot
Copy link

@danatsap Thank you for your contribution.

@gardener-robot gardener-robot added the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Jan 31, 2024
@gardener-robot-ci-1
Copy link
Contributor

Thank you @danatsap for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@MrBatschner
Copy link
Contributor

MrBatschner commented Jan 31, 2024

Some problems with that...

Most, if not all, supported Operating Systems run an image with cgroupsv2 mounted by default, so it should not be long until the gardenlet can centrally enforce cgroup v2 without each extension having to care for legacy nodes that still run cgroup v1 (and introducing potential bugs in the process)

One very important OS we support - SUSE cHost - is still on cgroups v1 which prevents us from setting anything globally. Garden Linux is on cgroups v2 and you are right, this initial commit by @danielfoehrKn still has some problems. Problem is: even if we reverted his change, the problems you are experiencing will not go away - we had them even before that.

Last but not least: the offending commit you are trying to revert is not part of release 0.22.0 of this extension but only of 0.23.0. Gardener landscapes at SAP are still running with 0.22.0 because we figured out that this commit is not helping and we needed additional time to investigate why we are still running into these nasty problems (this problem is not the easiest to debug).

Therefore, I would prefer to
/do-not-merge
for the moment.

@gardener-robot gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jan 31, 2024
@MrBatschner
Copy link
Contributor

/unhold

@gardener-robot gardener-robot removed the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Jan 31, 2024
@MrBatschner
Copy link
Contributor

/ok-to-test

@gardener-robot gardener-robot added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 31, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 31, 2024
Copy link
Contributor

@MrBatschner MrBatschner left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Feb 16, 2024
@MrBatschner MrBatschner merged commit c02041d into gardener:master Feb 16, 2024
5 checks passed
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Feb 16, 2024
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/os Operation system related kind/bug Bug needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) os/garden-linux Related to Garden Linux OS reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Existing nodes get cgroup driver configured inconsistently on extension upgrade to 0.23.0
5 participants