daemon: stop setting container resources to zero #45704

corhere · 2023-06-06T00:13:49Z

Many of the fields in LinuxResources struct are pointers to scalars for some reason, presumably to differentiate between set-to-zero and unset when unmarshaling from JSON, despite zero being outside the acceptable range for the corresponding kernel tunables. When creating the OCI spec for a container, the daemon sets the container's OCI spec CPUShares and BlkioWeight parameters to zero when the corresponding Docker container configuration values are zero, signifying unset, despite the minimum acceptable value for CPUShares being two, and BlkioWeight ten. This has gone unnoticed as runC does not distingiush set-to-zero from unset as it also uses zero internally to represent unset for those fields. However, kata-containers v3.2.0-alpha.3 tries to apply the explicit-zero resource parameters to the container, exactly as instructed, and fails loudly. The OCI runtime-spec is silent on how the runtime should handle the case when those parameters are explicitly set to out-of-range values and kata's behaviour is not unreasonable, so the daemon must therefore be in the wrong.

Translate unset values in the Docker container's resources HostConfig to omit the corresponding fields in the container's OCI spec when starting and updating a container in order to maximize compatibility with runtimes.

- What I did

- How I did it

- How to verify it

- Description for the changelog

Improved compatibility with OCI runtimes which are more strict than runC about resource constraint parameter values

- A picture of a cute animal (not mandatory but encouraged)

Switch to using t.TempDir() instead of rolling our own. Clean up mounts leaked by the tests as otherwise the tests fail due to the leaked mounts because unlike the old cleanup code, t.TempDir() cleanup does not ignore errors from os.RemoveAll. Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere · 2023-06-06T00:20:06Z

daemon/oci_linux_test.go

+	// HORRIBLE HACK: clean up shm mounts leaked by some tests. Otherwise the
+	// offending tests would fail due to the mounts blocking the temporary
+	// directory from being cleaned up.


I checked: only the tests are impacted. The shm mounts are not leaked on container exit—though I'm not sure where in the code they get unmounted.

I think I was running into this one as well in #45474 (comment)
and https://github.com/moby/moby/pull/43346/files#diff-7044cd74671fe1aa629c3374b3b65fb57e7d47c6a9a20a23b539ee1425a88f2eL19-L52

(Not sure why I didn't use c.ShmPath though 🤔

thaJeztah

overall looks good; one comment on the type change for Resources

thaJeztah · 2023-06-06T07:06:08Z

daemon/oci_linux_test.go

+	// HORRIBLE HACK: clean up shm mounts leaked by some tests. Otherwise the
+	// offending tests would fail due to the mounts blocking the temporary
+	// directory from being cleaned up.


I think I was running into this one as well in #45474 (comment)
and https://github.com/moby/moby/pull/43346/files#diff-7044cd74671fe1aa629c3374b3b65fb57e7d47c6a9a20a23b539ee1425a88f2eL19-L52

(Not sure why I didn't use c.ShmPath though 🤔

thaJeztah · 2023-06-06T07:08:50Z

daemon/oci_linux.go

+		if r.BlkioWeight != 0 {
+			w := r.BlkioWeight
+			specResources.BlockIO.Weight = &w
+		}


Definitely not a blocker, but would it make sense to have a getBlkioWeight() func (like we do for the other options above?

No. The other options require non-trivial processing, unlike this one. It could make sense to factor out a getBlockIOResources() func like getMemoryResources(), getCPUResources() and getPidsLimit(), though.

thaJeztah · 2023-06-06T07:10:35Z

daemon/oci_linux.go

 		if s.Linux.Resources != nil && len(s.Linux.Resources.Devices) > 0 {
 			specResources.Devices = s.Linux.Resources.Devices


Looking at this, I feel like WithResources() is doing too much, or we should split out a WithBlkIO() - it looks like this part is here, to prevent WithResources() from overwriting Devices (which probably is set separately), so trying to merge multiple options.

No, ensuring that mutations to the spec by WithDevices() are not clobbered is not WithResources() doing too much. It's just WithResources() being a good(-ish) implementation of a functional option, and what allows the functional options pattern to be used to factor distinct concerns into distinct option functions. Factoring out a WithBlkIO() function would also have to ensure that it does not clobber changes to the spec applied by WithResources() and other options.

That all being said, the way that WithResources() goes about not clobbering existing LinuxResources fields it does not otherwise touch is not particularly forward-compatible as it will zero out all other fields aside from Devices. I'll fix that.

thaJeztah · 2023-06-06T07:21:21Z

libcontainerd/types/types_linux.go

-type Resources specs.LinuxResources
+type Resources = specs.LinuxResources


Was this needed for this PR? I feel like this should be separate (and possibly when doing so, mark Resources deprecated in favour of specs.LinuxResources

It avoids having to explicitly cast types.Resources values to specs.LinuxResources. And the type serves a purpose: it allows the libcontainerd Task interface to be have a single definition which is applicable for both Windows and Linux platforms.

moby/libcontainerd/types/types_windows.go

Lines 26 to 27 in 1aae940

// Resources defines updatable container resource values.

type Resources struct{}

moby/libcontainerd/types/types.go

Line 96 in 1aae940

UpdateResources(ctx context.Context, resources *Resources) error

I am sure it would have been written as a type alias originally, if it existed at the time. (Proper type aliases were added in Go 1.9.)

moby/libcontainerd/remote/client_linux.go

Lines 24 to 26 in 1aae940

// go doesn't like the alias in 1.8, this means this need to be

// platform specific

return t.Update(ctx, containerd.WithResources((*specs.LinuxResources)(resources)))

I can split this out into its own commit if you feel strongly about it, but I don't think it's a significant enough change to warrant a separate PR. While technically a breaking change in the strictest sense, even Go's own apidiff tooling does not consider merging types to be worth flagging.

thaJeztah · 2023-06-06T07:23:31Z

Actually; looks like there's some test-failures that may be related 😅

Many of the fields in LinuxResources struct are pointers to scalars for some reason, presumably to differentiate between set-to-zero and unset when unmarshaling from JSON, despite zero being outside the acceptable range for the corresponding kernel tunables. When creating the OCI spec for a container, the daemon sets the container's OCI spec CPUShares and BlkioWeight parameters to zero when the corresponding Docker container configuration values are zero, signifying unset, despite the minimum acceptable value for CPUShares being two, and BlkioWeight ten. This has gone unnoticed as runC does not distingiush set-to-zero from unset as it also uses zero internally to represent unset for those fields. However, kata-containers v3.2.0-alpha.3 tries to apply the explicit-zero resource parameters to the container, exactly as instructed, and fails loudly. The OCI runtime-spec is silent on how the runtime should handle the case when those parameters are explicitly set to out-of-range values and kata's behaviour is not unreasonable, so the daemon must therefore be in the wrong. Translate unset values in the Docker container's resources HostConfig to omit the corresponding fields in the container's OCI spec when starting and updating a container in order to maximize compatibility with runtimes. Signed-off-by: Cory Snider <csnider@mirantis.com>

Audit the OCI spec options used for Linux containers to ensure they are less order-dependent. Ensure they don't assume that any pointer fields are non-nil and that they don't unintentionally clobber mutations to the spec applied by other options. Signed-off-by: Cory Snider <csnider@mirantis.com>

cpuguy83

LGTM

thaJeztah

LGTM

corhere added area/runtime process/cherry-pick kind/bugfix PR's that fix bugs labels Jun 6, 2023

corhere added this to the 25.0.0 milestone Jun 6, 2023

corhere requested review from tianon, cpuguy83, neersighted and thaJeztah June 6, 2023 00:13

corhere commented Jun 6, 2023

View reviewed changes

thaJeztah requested changes Jun 6, 2023

View reviewed changes

corhere added 2 commits June 6, 2023 12:13

corhere force-pushed the fix-zeroes-in-linux-resources branch from 3c52bbe to 8a094fe Compare June 6, 2023 17:10

tianon approved these changes Jun 8, 2023

View reviewed changes

tianon requested a review from thaJeztah June 8, 2023 22:48

cpuguy83 approved these changes Jun 10, 2023

View reviewed changes

cpuguy83 added the status/4-merge label Jun 10, 2023

neersighted approved these changes Jun 10, 2023

View reviewed changes

thaJeztah approved these changes Jun 12, 2023

View reviewed changes

thaJeztah merged commit ed798d6 into moby:master Jun 12, 2023
102 checks passed

thaJeztah mentioned this pull request Jun 14, 2023

[24.0 backport] daemon: stop setting container resources to zero #45746

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jun 14, 2023

thaJeztah mentioned this pull request Aug 13, 2023

[23.0 backport] Do not drop effective&permitted set #46222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: stop setting container resources to zero #45704

daemon: stop setting container resources to zero #45704

corhere commented Jun 6, 2023

corhere Jun 6, 2023

thaJeztah Jun 6, 2023

thaJeztah left a comment

thaJeztah Jun 6, 2023

thaJeztah Jun 6, 2023

corhere Jun 6, 2023

thaJeztah Jun 6, 2023

corhere Jun 6, 2023

thaJeztah Jun 6, 2023

corhere Jun 6, 2023

thaJeztah commented Jun 6, 2023

cpuguy83 left a comment

thaJeztah left a comment

		if s.Linux.Resources != nil && len(s.Linux.Resources.Devices) > 0 {
		specResources.Devices = s.Linux.Resources.Devices

		type Resources specs.LinuxResources
		type Resources = specs.LinuxResources

	// Resources defines updatable container resource values.
	type Resources struct{}

	// go doesn't like the alias in 1.8, this means this need to be
	// platform specific
	return t.Update(ctx, containerd.WithResources((*specs.LinuxResources)(resources)))

daemon: stop setting container resources to zero #45704

daemon: stop setting container resources to zero #45704

Conversation

corhere commented Jun 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah commented Jun 6, 2023

cpuguy83 left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment