Browse code

daemon: stop setting container resources to zero

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>

Cory Snider authored on 2023/06/06 07:44:51
Showing 7 changed files
... ...
@@ -104,7 +104,10 @@ func getMemoryResources(config containertypes.Resources) *specs.LinuxMemory {
104 104
 		memory.KernelTCP = &config.KernelMemoryTCP
105 105
 	}
106 106
 
107
-	return &memory
107
+	if memory != (specs.LinuxMemory{}) {
108
+		return &memory
109
+	}
110
+	return nil
108 111
 }
109 112
 
110 113
 func getPidsLimit(config containertypes.Resources) *specs.LinuxPids {
... ...
@@ -126,7 +129,7 @@ func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) {
126 126
 	if config.CPUShares < 0 {
127 127
 		return nil, fmt.Errorf("shares: invalid argument")
128 128
 	}
129
-	if config.CPUShares >= 0 {
129
+	if config.CPUShares > 0 {
130 130
 		shares := uint64(config.CPUShares)
131 131
 		cpu.Shares = &shares
132 132
 	}
... ...
@@ -167,7 +170,10 @@ func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) {
167 167
 		cpu.RealtimeRuntime = &c
168 168
 	}
169 169
 
170
-	return &cpu, nil
170
+	if cpu != (specs.LinuxCPU{}) {
171
+		return &cpu, nil
172
+	}
173
+	return nil, nil
171 174
 }
172 175
 
173 176
 func getBlkioWeightDevices(config containertypes.Resources) ([]specs.LinuxWeightDevice, error) {
... ...
@@ -973,13 +973,11 @@ func WithResources(c *container.Container) coci.SpecOpts {
973 973
 		if err != nil {
974 974
 			return err
975 975
 		}
976
-		blkioWeight := r.BlkioWeight
977 976
 
978 977
 		specResources := &specs.LinuxResources{
979 978
 			Memory: memoryRes,
980 979
 			CPU:    cpuRes,
981 980
 			BlockIO: &specs.LinuxBlockIO{
982
-				Weight:                  &blkioWeight,
983 981
 				WeightDevice:            weightDevices,
984 982
 				ThrottleReadBpsDevice:   readBpsDevice,
985 983
 				ThrottleWriteBpsDevice:  writeBpsDevice,
... ...
@@ -988,6 +986,10 @@ func WithResources(c *container.Container) coci.SpecOpts {
988 988
 			},
989 989
 			Pids: getPidsLimit(r),
990 990
 		}
991
+		if r.BlkioWeight != 0 {
992
+			w := r.BlkioWeight
993
+			specResources.BlockIO.Weight = &w
994
+		}
991 995
 
992 996
 		if s.Linux.Resources != nil && len(s.Linux.Resources.Devices) > 0 {
993 997
 			specResources.Devices = s.Linux.Resources.Devices
... ...
@@ -11,6 +11,8 @@ import (
11 11
 	"github.com/docker/docker/daemon/config"
12 12
 	"github.com/docker/docker/daemon/network"
13 13
 	"github.com/docker/docker/libnetwork"
14
+	"github.com/google/go-cmp/cmp/cmpopts"
15
+	"github.com/opencontainers/runtime-spec/specs-go"
14 16
 	"golang.org/x/sys/unix"
15 17
 	"gotest.tools/v3/assert"
16 18
 	is "gotest.tools/v3/assert/cmp"
... ...
@@ -212,3 +214,38 @@ func TestGetSourceMount(t *testing.T) {
212 212
 	_, _, err = getSourceMount(cwd)
213 213
 	assert.NilError(t, err)
214 214
 }
215
+
216
+func TestDefaultResources(t *testing.T) {
217
+	skip.If(t, os.Getuid() != 0, "skipping test that requires root") // TODO: is this actually true? I'm guilty of following the cargo cult here.
218
+
219
+	c := &container.Container{
220
+		HostConfig: &containertypes.HostConfig{
221
+			IpcMode: containertypes.IPCModeNone,
222
+		},
223
+	}
224
+	d := setupFakeDaemon(t, c)
225
+
226
+	s, err := d.createSpec(context.Background(), &configStore{}, c)
227
+	assert.NilError(t, err)
228
+	checkResourcesAreUnset(t, s.Linux.Resources)
229
+}
230
+
231
+func checkResourcesAreUnset(t *testing.T, r *specs.LinuxResources) {
232
+	t.Helper()
233
+
234
+	if r != nil {
235
+		if r.Memory != nil {
236
+			assert.Check(t, is.DeepEqual(r.Memory, &specs.LinuxMemory{}))
237
+		}
238
+		if r.CPU != nil {
239
+			assert.Check(t, is.DeepEqual(r.CPU, &specs.LinuxCPU{}))
240
+		}
241
+		assert.Check(t, is.Nil(r.Pids))
242
+		if r.BlockIO != nil {
243
+			assert.Check(t, is.DeepEqual(r.BlockIO, &specs.LinuxBlockIO{}, cmpopts.EquateEmpty()))
244
+		}
245
+		if r.Network != nil {
246
+			assert.Check(t, is.DeepEqual(r.Network, &specs.LinuxNetwork{}, cmpopts.EquateEmpty()))
247
+		}
248
+	}
249
+}
... ...
@@ -11,15 +11,19 @@ import (
11 11
 func toContainerdResources(resources container.Resources) *libcontainerdtypes.Resources {
12 12
 	var r libcontainerdtypes.Resources
13 13
 
14
-	r.BlockIO = &specs.LinuxBlockIO{
15
-		Weight: &resources.BlkioWeight,
14
+	if resources.BlkioWeight != 0 {
15
+		r.BlockIO = &specs.LinuxBlockIO{
16
+			Weight: &resources.BlkioWeight,
17
+		}
16 18
 	}
17 19
 
18
-	shares := uint64(resources.CPUShares)
19
-	r.CPU = &specs.LinuxCPU{
20
-		Shares: &shares,
21
-		Cpus:   resources.CpusetCpus,
22
-		Mems:   resources.CpusetMems,
20
+	cpu := specs.LinuxCPU{
21
+		Cpus: resources.CpusetCpus,
22
+		Mems: resources.CpusetMems,
23
+	}
24
+	if resources.CPUShares != 0 {
25
+		shares := uint64(resources.CPUShares)
26
+		cpu.Shares = &shares
23 27
 	}
24 28
 
25 29
 	var (
... ...
@@ -37,17 +41,33 @@ func toContainerdResources(resources container.Resources) *libcontainerdtypes.Re
37 37
 		period = uint64(resources.CPUPeriod)
38 38
 	}
39 39
 
40
-	r.CPU.Period = &period
41
-	r.CPU.Quota = &quota
40
+	if period != 0 {
41
+		cpu.Period = &period
42
+	}
43
+	if quota != 0 {
44
+		cpu.Quota = &quota
45
+	}
42 46
 
43
-	r.Memory = &specs.LinuxMemory{
44
-		Limit:       &resources.Memory,
45
-		Reservation: &resources.MemoryReservation,
46
-		Kernel:      &resources.KernelMemory,
47
+	if cpu != (specs.LinuxCPU{}) {
48
+		r.CPU = &cpu
47 49
 	}
48 50
 
51
+	var memory specs.LinuxMemory
52
+	if resources.Memory != 0 {
53
+		memory.Limit = &resources.Memory
54
+	}
55
+	if resources.MemoryReservation != 0 {
56
+		memory.Reservation = &resources.MemoryReservation
57
+	}
58
+	if resources.KernelMemory != 0 {
59
+		memory.Kernel = &resources.KernelMemory
60
+	}
49 61
 	if resources.MemorySwap > 0 {
50
-		r.Memory.Swap = &resources.MemorySwap
62
+		memory.Swap = &resources.MemorySwap
63
+	}
64
+
65
+	if memory != (specs.LinuxMemory{}) {
66
+		r.Memory = &memory
51 67
 	}
52 68
 
53 69
 	r.Pids = getPidsLimit(resources)
54 70
new file mode 100644
... ...
@@ -0,0 +1,11 @@
0
+package daemon // import "github.com/docker/docker/daemon"
1
+
2
+import (
3
+	"testing"
4
+
5
+	"github.com/docker/docker/api/types/container"
6
+)
7
+
8
+func TestToContainerdResources_Defaults(t *testing.T) {
9
+	checkResourcesAreUnset(t, toContainerdResources(container.Resources{}))
10
+}
... ...
@@ -21,9 +21,7 @@ func summaryFromInterface(i interface{}) (*libcontainerdtypes.Summary, error) {
21 21
 }
22 22
 
23 23
 func (t *task) UpdateResources(ctx context.Context, resources *libcontainerdtypes.Resources) error {
24
-	// go doesn't like the alias in 1.8, this means this need to be
25
-	// platform specific
26
-	return t.Update(ctx, containerd.WithResources((*specs.LinuxResources)(resources)))
24
+	return t.Update(ctx, containerd.WithResources(resources))
27 25
 }
28 26
 
29 27
 func hostIDFromMap(id uint32, mp []specs.LinuxIDMapping) int {
... ...
@@ -27,7 +27,7 @@ func InterfaceToStats(read time.Time, v interface{}) *Stats {
27 27
 }
28 28
 
29 29
 // Resources defines updatable container resource values. TODO: it must match containerd upcoming API
30
-type Resources specs.LinuxResources
30
+type Resources = specs.LinuxResources
31 31
 
32 32
 // Checkpoints contains the details of a checkpoint
33 33
 type Checkpoints struct{}