libct/cg: make Set accept configs.Resources by kolyshkin · Pull Request #2906 · opencontainers/runc

kolyshkin · 2021-04-13T18:23:56Z

This is mostly refactoring that lays the foundation for better resource updates.

libct/cg/systemd/v2: fix Set to use resources from the argument.
libct/cg: make Set accept configs.Resources (rather that the whole configs.Cgroups).
libct/cg: allow Set(nil) (to distinguish between set and update).
libc/cg: improve Manager docs.

See individual commits for more details as to why and how.

Proposed changelog entry

cgroup manager's Set now accepts configs.Resources rather than configs.Cgroups (libct/cg: make Set accept configs.Resources #2906)

kolyshkin · 2021-04-15T18:11:35Z

This helps to fix Kubernetes/CAdvisor after PR #2814 (see kubernetes/kubernetes#100631), and this is a change in the public API (of a cgroup controller) so I'd like to to go in in rc94.

h-vetinari · 2021-04-15T21:55:07Z

Typo in 499fb99 msg:

[...] as the method can not change cgroup ~~contfuration~~ configuration [...]

[...] as now Set ~~have~~ has two sets of resources [...]

kolyshkin · 2021-04-15T22:02:31Z

Typo in 499fb99 msg

@h-vetinari thank you, fixed

kolyshkin · 2021-04-15T22:24:36Z

ci hit a snag, restarted

odinuge

Overall positive with the change, just some thoughts when we are rewriting the API.

libcontainer/cgroups/cgroups.go

libcontainer/cgroups/fs/fs.go

cyphar · 2021-04-20T10:18:10Z

LGTM, aside from the nit -- though I will admit I'm not sure I quite understand what the need for Set(nil) is (then again, the old behaviour was also kinda weird too -- my guess is it got inherited by the behaviour of runc update where having resources unset leads to them not being set at all).

AkihiroSuda · 2021-04-22T09:32:09Z

needs rebase

kolyshkin · 2021-04-23T03:18:43Z

Rebased.

though I will admit I'm not sure I quite understand what the need for Set(nil) is

Some libcontainer users (notably kubernetes) previously relied on the fact that Apply() sets the parameters (specified during manager creation) -- which was true for systemd before #2814. The idea here is those users who relied on the functionality can call Set(nil) after Apply() to retain the old behavior.

Anyway, maybe this is not necessary -- I will re-check one more time tomorrow.

cyphar · 2021-04-28T00:11:50Z

@kolyshkin

Anyway, maybe this is not necessary -- I will re-check one more time tomorrow.

Yeah, the reason I asked was that I read through the linked k8s thread and it seems as though k8s doesn't need this (though I could be wrong). But as I said, I don't really mind if we have it -- it will make Set(nil) slightly strange but only direct users of libcontainer would care about it.

cyphar · 2021-04-28T03:06:34Z

With #2923 merged, this needs a rebase.

For some reason, systemd cgroup v2 driver's Set is not using its container argument when generating systemd unit properties. This bug is not detected by our update tests as we run a new binary every time and thus a new instance of a cgroup manager. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

A cgroup manager's Set method sets cgroup resources, but historically it was accepting configs.Cgroups. Refactor it to accept resources only. This is an improvement from the API point of view, as the method can not change cgroup configuration (such as path to the cgroup etc), it can only set (modify) its resources/limits. This also lays the foundation for complicated resource updates, as now Set has two sets of resources -- the one that was previously specified during cgroup manager creation (or the previous Set), and the one passed in the argument, so it could deduce the difference between these. This is a long term goal though. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

One notable change is cgroup manager's Set now accept Resources rather than Cgroup (see opencontainers/runc#2906). Modify the code accordingly. Also update runc dependencies (as hinted by hack/lint-depdendencies.sh): github.com/cilium/ebpf v0.5.0 github.com/containerd/console v1.0.2 github.com/coreos/go-systemd/v22 v22.3.1 github.com/godbus/dbus/v5 v5.0.4 github.com/moby/sys/mountinfo v0.4.1 golang.org/x/sys v0.0.0-20210426230700-d19ff857e887 github.com/google/go-cmp v0.5.4 github.com/kr/pretty v0.2.1 github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Kubernetes-commit: b49744f177087173d43d7771588d83b9df5a3c0b

kolyshkin force-pushed the set-allow-nil branch 2 times, most recently from 0fc0cc4 to dc35884 Compare April 13, 2021 22:08

kolyshkin marked this pull request as ready for review April 14, 2021 00:12

kolyshkin added this to the 1.0.0-rc94 milestone Apr 15, 2021

kolyshkin force-pushed the set-allow-nil branch from dc35884 to b022cff Compare April 15, 2021 22:02

kolyshkin changed the title ~~libct/cg: allow Set(nil)~~ libct/cg: make Set accept configs.Resources, allow Set(nil) Apr 15, 2021

odinuge reviewed Apr 16, 2021

View reviewed changes

libcontainer/cgroups/cgroups.go Show resolved Hide resolved

libcontainer/cgroups/fs/fs.go Outdated Show resolved Hide resolved

kolyshkin added impact/changelog kind/refactor refactoring labels Apr 19, 2021

AkihiroSuda previously approved these changes Apr 20, 2021

View reviewed changes

AkihiroSuda added the status/needs-rebase label Apr 22, 2021

kolyshkin dismissed AkihiroSuda’s stale review via ec4714c April 23, 2021 02:57

kolyshkin force-pushed the set-allow-nil branch 2 times, most recently from ec4714c to e0133d6 Compare April 23, 2021 03:08

AkihiroSuda removed the status/needs-rebase label Apr 23, 2021

tao12345666333 mentioned this pull request Apr 27, 2021

rc94 discussion (mid-April 2021?) #2790

Closed

AkihiroSuda added the status/needs-rebase label Apr 28, 2021

kolyshkin added 3 commits April 29, 2021 15:12

libc/cg: improve Manager docs

abf12ce

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the set-allow-nil branch from e0133d6 to abf12ce Compare April 29, 2021 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct/cg: make Set accept configs.Resources#2906

libct/cg: make Set accept configs.Resources#2906
AkihiroSuda merged 3 commits intoopencontainers:masterfrom
kolyshkin:set-allow-nil

kolyshkin commented Apr 13, 2021 •

edited

Loading

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

h-vetinari commented Apr 15, 2021

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

odinuge left a comment

Uh oh!

Uh oh!

Uh oh!

cyphar commented Apr 20, 2021

Uh oh!

AkihiroSuda commented Apr 22, 2021

Uh oh!

kolyshkin commented Apr 23, 2021

Uh oh!

cyphar commented Apr 28, 2021

Uh oh!

cyphar commented Apr 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

kolyshkin commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entry

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

h-vetinari commented Apr 15, 2021

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

kolyshkin commented Apr 15, 2021

Uh oh!

odinuge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cyphar commented Apr 20, 2021

Uh oh!

AkihiroSuda commented Apr 22, 2021

Uh oh!

kolyshkin commented Apr 23, 2021

Uh oh!

cyphar commented Apr 28, 2021

Uh oh!

cyphar commented Apr 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Apr 13, 2021 •

edited

Loading