improve validation of cpu-shares, and migrate TestRunInvalidCPUShares

This test was testing errors produced by runc; both the "maximum" and
"minimum" values originate from the OCI runtime;
d48d9cfefc/libcontainer/cgroups/fs/cpu.go (L66-L83)

    docker run --cpu-shares=1 alpine
    docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error setting cgroup config for procHooks process: the minimum allowed cpu-shares is 2: unknown.

Happy path for this setting is covered by TestRunWithCPUShares, and
various other tests, so we validate that the options take effect;
f5af46d4d5/integration-cli/docker_cli_run_unix_test.go (L494-L503)

This patch:

- removes the test and migrates it to an integration test
- removes the checks for errors that might be produced by runc
- updates our validation for invalid (negative) values to happen
  when creating the contaienr; the existing check that happened when
  creating the OCI spec is preserved, so that configs of existing containers
  are still validated.
- updates validateResources to return the correct error-type
- updated unit-test to validate

With this patch:

    make TEST_FILTER='TestCreateInvalidHostConfig' TEST_SKIP_INTEGRATION_CLI=1 test-integration
    --- PASS: TestCreateInvalidHostConfig (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_IpcMode (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_CPUShares (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_PidMode (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_PidMode_without_container_ID (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_Annotations (0.00s)
        --- PASS: TestCreateInvalidHostConfig/invalid_UTSMode (0.00s)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

daemon/daemon_unix.go

@@ -137,10 +137,10 @@ func getPidsLimit(config containertypes.Resources) *specs.LinuxPids {
 func getCPUResources(config containertypes.Resources) (*specs.LinuxCPU, error) {
 	cpu := specs.LinuxCPU{}
 
-	if config.CPUShares < 0 {
-		return nil, fmt.Errorf("shares: invalid argument")
-	}
-	if config.CPUShares > 0 {
+	if config.CPUShares != 0 {
+		if config.CPUShares < 0 {
+			return nil, fmt.Errorf("invalid CPU shares (%d): value must be a positive integer", config.CPUShares)
+		}
 		shares := uint64(config.CPUShares)
 		cpu.Shares = &shares
 	}

integration-cli/docker_cli_run_unix_test.go

@@ -724,24 +724,6 @@ func (s *DockerCLIRunSuite) TestRunInvalidCpusetMemsFlagValue(c *testing.T) {
 	assert.Assert(c, is.Contains(out, expected))
 }
 
-func (s *DockerCLIRunSuite) TestRunInvalidCPUShares(c *testing.T) {
-	testRequires(c, cpuShare, DaemonIsLinux)
-	out, _, err := dockerCmdWithError("run", "--cpu-shares", "1", "busybox", "echo", "test")
-	assert.ErrorContains(c, err, "", out)
-	expected := "minimum allowed cpu-shares is 2"
-	assert.Assert(c, is.Contains(out, expected))
-
-	out, _, err = dockerCmdWithError("run", "--cpu-shares", "-1", "busybox", "echo", "test")
-	assert.ErrorContains(c, err, "", out)
-	expected = "shares: invalid argument"
-	assert.Assert(c, is.Contains(out, expected))
-
-	out, _, err = dockerCmdWithError("run", "--cpu-shares", "99999999", "busybox", "echo", "test")
-	assert.ErrorContains(c, err, "", out)
-	expected = "maximum allowed cpu-shares is"
-	assert.Assert(c, is.Contains(out, expected))
-}
-
 func (s *DockerCLIRunSuite) TestRunWithDefaultShmSize(c *testing.T) {
 	testRequires(c, DaemonIsLinux)
 

integration/container/create_test.go

@@ -644,6 +644,11 @@ func TestCreateInvalidHostConfig(t *testing.T) {
 			hc:          container.HostConfig{Annotations: map[string]string{"": "a"}},
 			expectedErr: "Error response from daemon: invalid Annotations: the empty string is not permitted as an annotation key",
 		},
+		{
+			doc:         "invalid CPUShares",
+			hc:          container.HostConfig{Resources: container.Resources{CPUShares: -1}},
+			expectedErr: "Error response from daemon: invalid CPU shares (-1): value must be a positive integer",
+		},
 	}
 
 	for _, tc := range testCases {

runconfig/hostconfig_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/sysinfo"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -16,6 +17,7 @@ func TestValidateResources(t *testing.T) {
 		doc                string
 		resources          container.Resources
 		sysInfoCPURealtime bool
+		sysInfoCPUShares   bool
 		expectedError      string
 	}
 
@@ -54,6 +56,14 @@ func TestValidateResources(t *testing.T) {
 			sysInfoCPURealtime: true,
 			expectedError:      "cpu real-time runtime cannot be higher than cpu real-time period",
 		},
+		{
+			doc: "negative CPU shares",
+			resources: container.Resources{
+				CPUShares: -1,
+			},
+			sysInfoCPUShares: true,
+			expectedError:    "invalid CPU shares (-1): value must be a positive integer",
+		},
 	}
 
 	for _, tc := range tests {
@@ -63,9 +73,11 @@ func TestValidateResources(t *testing.T) {
 
 			var si sysinfo.SysInfo
 			si.CPURealtime = tc.sysInfoCPURealtime
+			si.CPUShares = tc.sysInfoCPUShares
 
 			err := validateResources(&hc, &si)
 			if tc.expectedError != "" {
+				assert.Check(t, is.ErrorType(err, errdefs.IsInvalidParameter))
 				assert.Check(t, is.Error(err, tc.expectedError))
 			} else {
 				assert.NilError(t, err)

runconfig/hostconfig_unix.go

@@ -56,6 +56,16 @@ func validateResources(hc *container.HostConfig, si *sysinfo.SysInfo) error {
 	if hc.Resources.CPURealtimePeriod != 0 && hc.Resources.CPURealtimeRuntime != 0 && hc.Resources.CPURealtimeRuntime > hc.Resources.CPURealtimePeriod {
 		return validationError("cpu real-time runtime cannot be higher than cpu real-time period")
 	}
+	if si.CPUShares {
+		// We're only producing an error if CPU-shares are supported to preserve
+		// existing behavior. The OCI runtime may still reject the config though.
+		// We should consider making this an error-condition when trying to set
+		// CPU-shares on a system that doesn't support it instead of silently
+		// ignoring.
+		if hc.Resources.CPUShares < 0 {
+			return validationError(fmt.Sprintf("invalid CPU shares (%d): value must be a positive integer", hc.Resources.CPUShares))
+		}
+	}
 	return nil
 }