cgroups/systemd: add setting CPUQuotaPeriod prop #2466
Conversation
|
On Ubuntu 18.04 we have this:
Apparently systemd in ubuntu-18.04 did not have support for this parameter. Looking at the systemd git, this was added in systemd v242 (systemd/systemd#9594). I think we can check systemd version during controller initialization and set this conditionally. |
|
#2458 is merged, please rebase |
671a84a to
7b5efe9
Compare
|
Added systemd version detection and conditional setting. I'm not quite sure why this one fails. @AkihiroSuda can you please take a look? From https://travis-ci.org/github/opencontainers/runc/jobs/698672952#L4689:
|
|
Added a commit to temporarily disable setting cpu quota period to check that the error above is caused by it. |
|
Still have the same error after commenting out setting
|
21f1c1f to
a235fa9
Compare
| return nil | ||
| } | ||
|
|
||
| func systemdVersion() (int, error) { |
There was a problem hiding this comment.
This seems heavy to call per runc invocation. Should we think about using build tags instead, so users can compile it for their version of systemd as needed? Another option may be a command-line flag. This applies to any other systemd feature detection as well.
There was a problem hiding this comment.
Let's just always assume systemd >= 242, and fall back to 241-compatible mode on error
There was a problem hiding this comment.
This seems heavy to call per runc invocation
Does not look like heavy to me, and it's not per runc invocation, but only upon container creation or update (and in this case we talk to dbus already to create/update a unit, so there's no such thing as creating and new connection just to get the version).
Or am I missing something here?
The alternative is to exec systemctl and parse its output. I haven't checked which way is faster, but can look into it.
Let's just always assume systemd >= 242, and fall back to 241-compatible mode on error
I was thinking about it, but the logic would look really ugly (got EPERM? remove cpu quota period and retry). I would consider it if there is no other way to know if systemd supports the feature or not, but it seems
For some reason, runc systemd drivers (both v1 and v2) never set systemd unit property named `CPUQuotaPeriod` (known as `CPUQuotaPeriodUSec` on dbus and in `systemctl show` output). Set it, and add a check to all the integration tests. The check is less than trivial because, when not set, the value is shown as "infinity" but when set to the same (default) value, shown as "100ms", so in case we expect 100ms (period = 100000 us), we have to _also_ check for "infinity". [v2: add systemd version checks since CPUQuotaPeriod requires v242+] Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
a235fa9 to
e751a16
Compare
|
Modified the patch to explicitly reuse dbus connection (which was implicitly reused before). This also fixed the rootless failure. |
| connDbus *systemdDbus.Conn | ||
| connErr error | ||
|
|
||
| versionOnce sync.Once |
There was a problem hiding this comment.
Note that all this sync.Once stuff is currently redundant, as we only call systemdVersion() once. It was added in anticipation there will be other systemd parameters like this that need a version guard, and since we call systemdVersion() when and where needed (right from addCpuQuota) rather than in advance, the future calls might come from a similar "when and where needed" places.
This is currently based on and includes #2458 and will be rebased once that one is merged. Please only review the last commit.
For some reason, runc systemd drivers (both v1 and v2) never set
systemd unit property named
CPUQuotaPeriod(known asCPUQuotaPeriodUSecon dbus and insystemctl showoutput).Set it, and add a check to all the integration tests. The check is less
than trivial because, when not set, the value is shown as "infinity" but
when set to the same (default) value, shown as "100ms", so in case we
expect 100ms (period = 100000 us), we have to also check for
"infinity".
Fixes #2465