gardener / gardener-extension-os-gardenlinux

Gardener extension controller for the Garden Linux operating system
Apache License 2.0
9 stars 30 forks source link

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

Closed danatsap closed 8 months ago

danatsap commented 8 months ago

This reverts commit 37251573e5225c2f4ed6afa4a61f674a8efec245, reversing changes made to 1d7c3bd244e7abfc4dd3928db939956253248db7.

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.

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 
gardener-robot commented 8 months ago

@danatsap Thank you for your contribution.

gardener-robot-ci-1 commented 8 months ago

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 commented 8 months ago

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.

MrBatschner commented 8 months ago

/unhold

MrBatschner commented 8 months ago

/ok-to-test