Merge pull request #45704 from corhere/fix-zeroes-in-linux-resources

daemon: stop setting container resources to zero

daemon/daemon_unix.go

@@ -104,7 +104,10 @@ func getMemoryResources(config containertypes.Resources) *specs.LinuxMemory {
 		memory.KernelTCP = &config.KernelMemoryTCP
 	}
 
-	return &memory
+	if memory != (specs.LinuxMemory{}) {
+		return &memory
+	}
+	return nil
 }
 
 func getPidsLimit(config containertypes.Resources) *specs.LinuxPids {
@@ -126,7 +129,7 @@ func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) {
 	if config.CPUShares < 0 {
 		return nil, fmt.Errorf("shares: invalid argument")
 	}
-	if config.CPUShares >= 0 {
+	if config.CPUShares > 0 {
 		shares := uint64(config.CPUShares)
 		cpu.Shares = &shares
 	}
@@ -167,7 +170,10 @@ func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) {
 		cpu.RealtimeRuntime = &c
 	}
 
-	return &cpu, nil
+	if cpu != (specs.LinuxCPU{}) {
+		return &cpu, nil
+	}
+	return nil, nil
 }
 
 func getBlkioWeightDevices(config containertypes.Resources) ([]specs.LinuxWeightDevice, error) {

daemon/oci_linux.go

@@ -53,6 +53,9 @@ func withRlimits(daemon *Daemon, daemonCfg *dconfig.Config, c *container.Contain
 			})
 		}
 
+		if s.Process == nil {
+			s.Process = &specs.Process{}
+		}
 		s.Process.Rlimits = rlimits
 		return nil
 	}
@@ -113,6 +116,9 @@ func withRootless(daemon *Daemon, daemonCfg *dconfig.Config) coci.SpecOpts {
 // WithOOMScore sets the oom score
 func WithOOMScore(score *int) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error {
+		if s.Process == nil {
+			s.Process = &specs.Process{}
+		}
 		s.Process.OOMScoreAdj = score
 		return nil
 	}
@@ -121,6 +127,12 @@ func WithOOMScore(score *int) coci.SpecOpts {
 // WithSelinux sets the selinux labels
 func WithSelinux(c *container.Container) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error {
+		if s.Process == nil {
+			s.Process = &specs.Process{}
+		}
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
+		}
 		s.Process.SelinuxLabel = c.GetProcessLabel()
 		s.Linux.MountLabel = c.MountLabel
 		return nil
@@ -151,6 +163,9 @@ func WithApparmor(c *container.Container) coci.SpecOpts {
 					return err
 				}
 			}
+			if s.Process == nil {
+				s.Process = &specs.Process{}
+			}
 			s.Process.ApparmorProfile = appArmorProfile
 		}
 		return nil
@@ -213,6 +228,10 @@ func getUser(c *container.Container, username string) (specs.User, error) {
 }
 
 func setNamespace(s *specs.Spec, ns specs.LinuxNamespace) {
+	if s.Linux == nil {
+		s.Linux = &specs.Linux{}
+	}
+
 	for i, n := range s.Linux.Namespaces {
 		if n.Type == ns.Type {
 			s.Linux.Namespaces[i] = ns
@@ -606,6 +625,9 @@ func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container)
 				}
 				rootpg := mountPropagationMap[s.Linux.RootfsPropagation]
 				if rootpg != mount.SHARED && rootpg != mount.RSHARED {
+					if s.Linux == nil {
+						s.Linux = &specs.Linux{}
+					}
 					s.Linux.RootfsPropagation = mountPropagationReverseMap[mount.SHARED]
 				}
 			case mount.SLAVE, mount.RSLAVE:
@@ -634,6 +656,9 @@ func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container)
 				if !fallback {
 					rootpg := mountPropagationMap[s.Linux.RootfsPropagation]
 					if rootpg != mount.SHARED && rootpg != mount.RSHARED && rootpg != mount.SLAVE && rootpg != mount.RSLAVE {
+						if s.Linux == nil {
+							s.Linux = &specs.Linux{}
+						}
 						s.Linux.RootfsPropagation = mountPropagationReverseMap[mount.RSLAVE]
 					}
 				}
@@ -706,8 +731,10 @@ func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container)
 					clearReadOnly(&s.Mounts[i])
 				}
 			}
-			s.Linux.ReadonlyPaths = nil
-			s.Linux.MaskedPaths = nil
+			if s.Linux != nil {
+				s.Linux.ReadonlyPaths = nil
+				s.Linux.MaskedPaths = nil
+			}
 		}
 
 		// TODO: until a kernel/mount solution exists for handling remount in a user namespace,
@@ -753,6 +780,9 @@ func withCommonOptions(daemon *Daemon, daemonCfg *dconfig.Config, c *container.C
 		if len(cwd) == 0 {
 			cwd = "/"
 		}
+		if s.Process == nil {
+			s.Process = &specs.Process{}
+		}
 		s.Process.Args = append([]string{c.Path}, c.Args...)
 
 		// only add the custom init if it is specified and the container is running in its
@@ -829,6 +859,9 @@ func withCgroups(daemon *Daemon, daemonCfg *dconfig.Config, c *container.Contain
 		} else {
 			cgroupsPath = filepath.Join(parent, c.ID)
 		}
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
+		}
 		s.Linux.CgroupsPath = cgroupsPath
 
 		// the rest is only needed for CPU RT controller
@@ -929,8 +962,14 @@ func WithDevices(daemon *Daemon, c *container.Container) coci.SpecOpts {
 			}
 		}
 
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
+		}
+		if s.Linux.Resources == nil {
+			s.Linux.Resources = &specs.LinuxResources{}
+		}
 		s.Linux.Devices = append(s.Linux.Devices, devs...)
-		s.Linux.Resources.Devices = devPermissions
+		s.Linux.Resources.Devices = append(s.Linux.Resources.Devices, devPermissions...)
 
 		for _, req := range c.HostConfig.DeviceRequests {
 			if err := daemon.handleDevice(req, s); err != nil {
@@ -971,27 +1010,28 @@ func WithResources(c *container.Container) coci.SpecOpts {
 		if err != nil {
 			return err
 		}
-		blkioWeight := r.BlkioWeight
 
-		specResources := &specs.LinuxResources{
-			Memory: memoryRes,
-			CPU:    cpuRes,
-			BlockIO: &specs.LinuxBlockIO{
-				Weight:                  &blkioWeight,
-				WeightDevice:            weightDevices,
-				ThrottleReadBpsDevice:   readBpsDevice,
-				ThrottleWriteBpsDevice:  writeBpsDevice,
-				ThrottleReadIOPSDevice:  readIOpsDevice,
-				ThrottleWriteIOPSDevice: writeIOpsDevice,
-			},
-			Pids: getPidsLimit(r),
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
 		}
-
-		if s.Linux.Resources != nil && len(s.Linux.Resources.Devices) > 0 {
-			specResources.Devices = s.Linux.Resources.Devices
+		if s.Linux.Resources == nil {
+			s.Linux.Resources = &specs.LinuxResources{}
+		}
+		s.Linux.Resources.Memory = memoryRes
+		s.Linux.Resources.CPU = cpuRes
+		s.Linux.Resources.BlockIO = &specs.LinuxBlockIO{
+			WeightDevice:            weightDevices,
+			ThrottleReadBpsDevice:   readBpsDevice,
+			ThrottleWriteBpsDevice:  writeBpsDevice,
+			ThrottleReadIOPSDevice:  readIOpsDevice,
+			ThrottleWriteIOPSDevice: writeIOpsDevice,
+		}
+		if r.BlkioWeight != 0 {
+			w := r.BlkioWeight
+			s.Linux.Resources.BlockIO.Weight = &w
 		}
+		s.Linux.Resources.Pids = getPidsLimit(r)
 
-		s.Linux.Resources = specResources
 		return nil
 	}
 }
@@ -999,6 +1039,15 @@ func WithResources(c *container.Container) coci.SpecOpts {
 // WithSysctls sets the container's sysctls
 func WithSysctls(c *container.Container) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error {
+		if len(c.HostConfig.Sysctls) == 0 {
+			return nil
+		}
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
+		}
+		if s.Linux.Sysctl == nil {
+			s.Linux.Sysctl = make(map[string]string)
+		}
 		// We merge the sysctls injected above with the HostConfig (latter takes
 		// precedence for backwards-compatibility reasons).
 		for k, v := range c.HostConfig.Sysctls {
@@ -1011,6 +1060,9 @@ func WithSysctls(c *container.Container) coci.SpecOpts {
 // WithUser sets the container's user
 func WithUser(c *container.Container) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error {
+		if s.Process == nil {
+			s.Process = &specs.Process{}
+		}
 		var err error
 		s.Process.User, err = getUser(c, c.Config.User)
 		return err

daemon/oci_linux_test.go

@@ -11,17 +11,20 @@ import (
 	"github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/daemon/network"
 	"github.com/docker/docker/libnetwork"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"golang.org/x/sys/unix"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/skip"
 )
 
 func setupFakeDaemon(t *testing.T, c *container.Container) *Daemon {
-	root, err := os.MkdirTemp("", "oci_linux_test-root")
-	assert.NilError(t, err)
+	t.Helper()
+	root := t.TempDir()
 
 	rootfs := filepath.Join(root, "rootfs")
-	err = os.MkdirAll(rootfs, 0755)
+	err := os.MkdirAll(rootfs, 0755)
 	assert.NilError(t, err)
 
 	netController, err := libnetwork.New()
@@ -48,6 +51,18 @@ func setupFakeDaemon(t *testing.T, c *container.Container) *Daemon {
 		c.NetworkSettings = &network.Settings{Networks: make(map[string]*network.EndpointSettings)}
 	}
 
+	// 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.
+	t.Cleanup(func() {
+		if c.ShmPath != "" {
+			var err error
+			for err == nil { // Some tests over-mount over the same path multiple times.
+				err = unix.Unmount(c.ShmPath, unix.MNT_DETACH)
+			}
+		}
+	})
+
 	return d
 }
 
@@ -59,10 +74,6 @@ func (i *fakeImageService) StorageDriver() string {
 	return "overlay"
 }
 
-func cleanupFakeContainer(c *container.Container) {
-	_ = os.RemoveAll(c.Root)
-}
-
 // TestTmpfsDevShmNoDupMount checks that a user-specified /dev/shm tmpfs
 // mount (as in "docker run --tmpfs /dev/shm:rw,size=NNN") does not result
 // in "Duplicate mount point" error from the engine.
@@ -80,7 +91,6 @@ func TestTmpfsDevShmNoDupMount(t *testing.T) {
 		},
 	}
 	d := setupFakeDaemon(t, c)
-	defer cleanupFakeContainer(c)
 
 	_, err := d.createSpec(context.TODO(), &configStore{}, c)
 	assert.Check(t, err)
@@ -99,7 +109,6 @@ func TestIpcPrivateVsReadonly(t *testing.T) {
 		},
 	}
 	d := setupFakeDaemon(t, c)
-	defer cleanupFakeContainer(c)
 
 	s, err := d.createSpec(context.TODO(), &configStore{}, c)
 	assert.Check(t, err)
@@ -128,7 +137,6 @@ func TestSysctlOverride(t *testing.T) {
 		},
 	}
 	d := setupFakeDaemon(t, c)
-	defer cleanupFakeContainer(c)
 
 	// Ensure that the implicit sysctl is set correctly.
 	s, err := d.createSpec(context.TODO(), &configStore{}, c)
@@ -179,7 +187,6 @@ func TestSysctlOverrideHost(t *testing.T) {
 		},
 	}
 	d := setupFakeDaemon(t, c)
-	defer cleanupFakeContainer(c)
 
 	// Ensure that the implicit sysctl is not set
 	s, err := d.createSpec(context.TODO(), &configStore{}, c)
@@ -207,3 +214,38 @@ func TestGetSourceMount(t *testing.T) {
 	_, _, err = getSourceMount(cwd)
 	assert.NilError(t, err)
 }
+
+func TestDefaultResources(t *testing.T) {
+	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.
+
+	c := &container.Container{
+		HostConfig: &containertypes.HostConfig{
+			IpcMode: containertypes.IPCModeNone,
+		},
+	}
+	d := setupFakeDaemon(t, c)
+
+	s, err := d.createSpec(context.Background(), &configStore{}, c)
+	assert.NilError(t, err)
+	checkResourcesAreUnset(t, s.Linux.Resources)
+}
+
+func checkResourcesAreUnset(t *testing.T, r *specs.LinuxResources) {
+	t.Helper()
+
+	if r != nil {
+		if r.Memory != nil {
+			assert.Check(t, is.DeepEqual(r.Memory, &specs.LinuxMemory{}))
+		}
+		if r.CPU != nil {
+			assert.Check(t, is.DeepEqual(r.CPU, &specs.LinuxCPU{}))
+		}
+		assert.Check(t, is.Nil(r.Pids))
+		if r.BlockIO != nil {
+			assert.Check(t, is.DeepEqual(r.BlockIO, &specs.LinuxBlockIO{}, cmpopts.EquateEmpty()))
+		}
+		if r.Network != nil {
+			assert.Check(t, is.DeepEqual(r.Network, &specs.LinuxNetwork{}, cmpopts.EquateEmpty()))
+		}
+	}
+}

daemon/oci_opts.go

@@ -13,6 +13,9 @@ import (
 func WithConsoleSize(c *container.Container) coci.SpecOpts {
 	return func(ctx context.Context, _ coci.Client, _ *containers.Container, s *coci.Spec) error {
 		if c.HostConfig.ConsoleSize[0] > 0 || c.HostConfig.ConsoleSize[1] > 0 {
+			if s.Process == nil {
+				s.Process = &specs.Process{}
+			}
 			s.Process.ConsoleSize = &specs.Box{
 				Height: c.HostConfig.ConsoleSize[0],
 				Width:  c.HostConfig.ConsoleSize[1],

daemon/oci_utils.go

@@ -9,7 +9,12 @@ func setLinuxDomainname(c *container.Container, s *specs.Spec) {
 	// There isn't a field in the OCI for the NIS domainname, but luckily there
 	// is a sysctl which has an identical effect to setdomainname(2) so there's
 	// no explicit need for runtime support.
-	s.Linux.Sysctl = make(map[string]string)
+	if s.Linux == nil {
+		s.Linux = &specs.Linux{}
+	}
+	if s.Linux.Sysctl == nil {
+		s.Linux.Sysctl = make(map[string]string)
+	}
 	if c.Config.Domainname != "" {
 		s.Linux.Sysctl["kernel.domainname"] = c.Config.Domainname
 	}

daemon/seccomp_linux.go

@@ -9,6 +9,7 @@ import (
 	"github.com/docker/docker/container"
 	dconfig "github.com/docker/docker/daemon/config"
 	"github.com/docker/docker/profiles/seccomp"
+	specs "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 )
 
@@ -31,6 +32,9 @@ func WithSeccomp(daemon *Daemon, c *container.Container) coci.SpecOpts {
 			c.SeccompProfile = dconfig.SeccompProfileUnconfined
 			return nil
 		}
+		if s.Linux == nil {
+			s.Linux = &specs.Linux{}
+		}
 		var err error
 		switch {
 		case c.SeccompProfile == dconfig.SeccompProfileDefault:

daemon/update_linux.go

@@ -11,15 +11,19 @@ import (
 func toContainerdResources(resources container.Resources) *libcontainerdtypes.Resources {
 	var r libcontainerdtypes.Resources
 
-	r.BlockIO = &specs.LinuxBlockIO{
-		Weight: &resources.BlkioWeight,
+	if resources.BlkioWeight != 0 {
+		r.BlockIO = &specs.LinuxBlockIO{
+			Weight: &resources.BlkioWeight,
+		}
 	}
 
-	shares := uint64(resources.CPUShares)
-	r.CPU = &specs.LinuxCPU{
-		Shares: &shares,
-		Cpus:   resources.CpusetCpus,
-		Mems:   resources.CpusetMems,
+	cpu := specs.LinuxCPU{
+		Cpus: resources.CpusetCpus,
+		Mems: resources.CpusetMems,
+	}
+	if resources.CPUShares != 0 {
+		shares := uint64(resources.CPUShares)
+		cpu.Shares = &shares
 	}
 
 	var (
@@ -37,17 +41,33 @@ func toContainerdResources(resources container.Resources) *libcontainerdtypes.Re
 		period = uint64(resources.CPUPeriod)
 	}
 
-	r.CPU.Period = &period
-	r.CPU.Quota = &quota
+	if period != 0 {
+		cpu.Period = &period
+	}
+	if quota != 0 {
+		cpu.Quota = &quota
+	}
 
-	r.Memory = &specs.LinuxMemory{
-		Limit:       &resources.Memory,
-		Reservation: &resources.MemoryReservation,
-		Kernel:      &resources.KernelMemory,
+	if cpu != (specs.LinuxCPU{}) {
+		r.CPU = &cpu
 	}
 
+	var memory specs.LinuxMemory
+	if resources.Memory != 0 {
+		memory.Limit = &resources.Memory
+	}
+	if resources.MemoryReservation != 0 {
+		memory.Reservation = &resources.MemoryReservation
+	}
+	if resources.KernelMemory != 0 {
+		memory.Kernel = &resources.KernelMemory
+	}
 	if resources.MemorySwap > 0 {
-		r.Memory.Swap = &resources.MemorySwap
+		memory.Swap = &resources.MemorySwap
+	}
+
+	if memory != (specs.LinuxMemory{}) {
+		r.Memory = &memory
 	}
 
 	r.Pids = getPidsLimit(resources)

daemon/update_linux_test.go

@@ -0,0 +1,11 @@
+package daemon // import "github.com/docker/docker/daemon"
+
+import (
+	"testing"
+
+	"github.com/docker/docker/api/types/container"
+)
+
+func TestToContainerdResources_Defaults(t *testing.T) {
+	checkResourcesAreUnset(t, toContainerdResources(container.Resources{}))
+}

libcontainerd/remote/client_linux.go

@@ -21,9 +21,7 @@ func summaryFromInterface(i interface{}) (*libcontainerdtypes.Summary, error) {
 }
 
 func (t *task) UpdateResources(ctx context.Context, resources *libcontainerdtypes.Resources) error {
-	// 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)))
+	return t.Update(ctx, containerd.WithResources(resources))
 }
 
 func hostIDFromMap(id uint32, mp []specs.LinuxIDMapping) int {

libcontainerd/types/types_linux.go

@@ -27,7 +27,7 @@ func InterfaceToStats(read time.Time, v interface{}) *Stats {
 }
 
 // Resources defines updatable container resource values. TODO: it must match containerd upcoming API
-type Resources specs.LinuxResources
+type Resources = specs.LinuxResources
 
 // Checkpoints contains the details of a checkpoint
 type Checkpoints struct{}

oci/namespaces.go

@@ -4,6 +4,9 @@ import specs "github.com/opencontainers/runtime-spec/specs-go"
 
 // RemoveNamespace removes the `nsType` namespace from OCI spec `s`
 func RemoveNamespace(s *specs.Spec, nsType specs.LinuxNamespaceType) {
+	if s.Linux == nil {
+		return
+	}
 	for i, n := range s.Linux.Namespaces {
 		if n.Type == nsType {
 			s.Linux.Namespaces = append(s.Linux.Namespaces[:i], s.Linux.Namespaces[i+1:]...)

oci/oci.go

@@ -20,6 +20,9 @@ var deviceCgroupRuleRegex = regexp.MustCompile("^([acb]) ([0-9]+|\\*):([0-9]+|\\
 // SetCapabilities sets the provided capabilities on the spec
 // All capabilities are added if privileged is true.
 func SetCapabilities(s *specs.Spec, caplist []string) error {
+	if s.Process == nil {
+		s.Process = &specs.Process{}
+	}
 	// setUser has already been executed here
 	if s.Process.User.UID == 0 {
 		s.Process.Capabilities = &specs.LinuxCapabilities{

pkg/rootless/specconv/specconv_linux.go

@@ -40,11 +40,13 @@ func getCurrentOOMScoreAdj() int {
 
 func toRootless(spec *specs.Spec, v2Controllers []string, currentOOMScoreAdj int) error {
 	if len(v2Controllers) == 0 {
-		// Remove cgroup settings.
-		spec.Linux.Resources = nil
-		spec.Linux.CgroupsPath = ""
+		if spec.Linux != nil {
+			// Remove cgroup settings.
+			spec.Linux.Resources = nil
+			spec.Linux.CgroupsPath = ""
+		}
 	} else {
-		if spec.Linux.Resources != nil {
+		if spec.Linux != nil && spec.Linux.Resources != nil {
 			m := make(map[string]struct{})
 			for _, s := range v2Controllers {
 				m[s] = struct{}{}
@@ -77,7 +79,7 @@ func toRootless(spec *specs.Spec, v2Controllers []string, currentOOMScoreAdj int
 		}
 	}
 
-	if spec.Process.OOMScoreAdj != nil && *spec.Process.OOMScoreAdj < currentOOMScoreAdj {
+	if spec.Process != nil && spec.Process.OOMScoreAdj != nil && *spec.Process.OOMScoreAdj < currentOOMScoreAdj {
 		*spec.Process.OOMScoreAdj = currentOOMScoreAdj
 	}
 
@@ -110,6 +112,9 @@ func isHostNS(spec *specs.Spec, nsType specs.LinuxNamespaceType) (bool, error) {
 	if strings.Contains(string(nsType), string(os.PathSeparator)) {
 		return false, fmt.Errorf("unexpected namespace type %q", nsType)
 	}
+	if spec.Linux == nil {
+		return false, nil
+	}
 	for _, ns := range spec.Linux.Namespaces {
 		if ns.Type == nsType {
 			if ns.Path == "" {
@@ -144,15 +149,17 @@ func bindMountHostProcfs(spec *specs.Spec) error {
 		}
 	}
 
-	// Remove ReadonlyPaths for /proc/*
-	newROP := spec.Linux.ReadonlyPaths[:0]
-	for _, s := range spec.Linux.ReadonlyPaths {
-		s = path.Clean(s)
-		if !strings.HasPrefix(s, "/proc/") {
-			newROP = append(newROP, s)
+	if spec.Linux != nil {
+		// Remove ReadonlyPaths for /proc/*
+		newROP := spec.Linux.ReadonlyPaths[:0]
+		for _, s := range spec.Linux.ReadonlyPaths {
+			s = path.Clean(s)
+			if !strings.HasPrefix(s, "/proc/") {
+				newROP = append(newROP, s)
+			}
 		}
+		spec.Linux.ReadonlyPaths = newROP
 	}
-	spec.Linux.ReadonlyPaths = newROP
 
 	return nil
 }