Skip to content

Don't freeze cgroup on update for systemd cgroup v2 #3067

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
cyphar merged 2 commits into from
Jul 13, 2021
Merged

Don't freeze cgroup on update for systemd cgroup v2 #3067

cyphar merged 2 commits into from
Jul 13, 2021

Conversation

@odinuge
Copy link
Contributor

@odinuge odinuge commented Jul 7, 2021 •
edited by kolyshkin
Loading

1.0 backport: #3092

@odinuge odinuge force-pushed the systemd-v2-freeze branch 2 times, most recently from 3f36eeb to 125f33b Compare July 7, 2021 11:50
odinuge
odinuge commented Jul 7, 2021
View reviewed changes
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 7, 2021

Indeed, I reinstated the freeze because of #3014, which is v1 only bug.

kolyshkin
kolyshkin reviewed Jul 7, 2021
View reviewed changes
kolyshkin
kolyshkin requested changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Second commit looks good; I don't like code duplication in the first one.

odinuge added 2 commits July 7, 2021 22:44
@odinuge
Update device update tests
d41a273 
Run device update tests on cgroup v2, and add a test verifying that we
don't allow access to devices when we don't intend to.

Signed-off-by: Odin Ugedal <odin@uged.al>
@odinuge
libct/cg/sd: Don't freeze cgroup on cgroup v2 Set
f33be7c 
Since device updates in cgroup v2 are atomic for systemd, there is no
need to freeze the processes before running the updates.

Signed-off-by: Odin Ugedal <odin@uged.al>
@odinuge odinuge force-pushed the systemd-v2-freeze branch from 125f33b to f33be7c Compare July 7, 2021 20:44
@odinuge odinuge changed the title [WIP] Don't freeze cgroup on update for systemd cgroup v2 Don't freeze cgroup on update for systemd cgroup v2 Jul 7, 2021
kolyshkin
kolyshkin approved these changes Jul 7, 2021
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 7, 2021

Thanks @odinuge for the update.

I treat this as an improvement (rather than a bug fix), and so this is not a candidate for 1.0 backport. Let me know if you disagree.

@kolyshkin kolyshkin requested review from AkihiroSuda and cyphar July 8, 2021 00:28
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 8, 2021

@cyphar @AkihiroSuda @mrunalp PTAL

@kolyshkin kolyshkin added this to the 1.1.0 milestone Jul 9, 2021
@kolyshkin
Copy link
Contributor

kolyshkin commented Jul 11, 2021

OK, I found out this is actually a bug fix -- it fixes the inability to freeze the container/cgroup via cgroup manager's Set method for cgroup v2.

While I find using Set for freezing cgroup questionable (as the freezer is kind of special -- and so we have Freeze and GetFreezerState cgroup manager methods, as well as Pause and Resume container methods), the functionality is there.

This PR fixes the ability to freeze systemd/v2 cgroup via Set(). I am adding a test case for that in #3082.

So, this is a bug, but I still don't think it calls for 1.0 backport.

kolyshkin
kolyshkin reviewed Jul 11, 2021
View reviewed changes
@kolyshkin kolyshkin mentioned this pull request Jul 11, 2021
Merged
cyphar
cyphar approved these changes Jul 13, 2021
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM.

@cyphar cyphar closed this in 2c01cec Jul 13, 2021
@cyphar cyphar merged commit 2c01cec into opencontainers:master Jul 13, 2021
This was referenced Jul 15, 2021
Closed
Closed
@kolyshkin kolyshkin added the backport/1.0-todo A PR in main branch which needs to be backported to release-1.0 label Jul 15, 2021
@kolyshkin kolyshkin mentioned this pull request Jul 15, 2021
Merged
@kolyshkin kolyshkin mentioned this pull request Jul 16, 2021
Closed
@kolyshkin kolyshkin added backport/1.0-done A PR in main branch which has been backported to release-1.0 and removed backport/1.0-todo A PR in main branch which needs to be backported to release-1.0 labels Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@mrunalp mrunalp mrunalp approved these changes

@cyphar cyphar cyphar approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

Assignees

No one assigned

Labels

area/cgroupv2 area/ci area/systemd backport/1.0-done A PR in main branch which has been backported to release-1.0

Projects

None yet

Milestone

1.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@odinuge @kolyshkin @mrunalp @cyphar