Merge pull request moby#49180 from thaJeztah/improve_cpushares_valida…

…tion

improve validation of cpu-shares, and migrate TestRunInvalidCPUShares

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,59 +6,82 @@ 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"
 )
 
 func TestValidateResources(t *testing.T) {
 	type resourceTest struct {
-		ConfigCPURealtimePeriod  int64
-		ConfigCPURealtimeRuntime int64
-		SysInfoCPURealtime       bool
-		ErrorExpected            bool
-		FailureMsg               string
+		doc                string
+		resources          container.Resources
+		sysInfoCPURealtime bool
+		sysInfoCPUShares   bool
+		expectedError      string
 	}
 
 	tests := []resourceTest{
 		{
-			ConfigCPURealtimePeriod:  1000,
-			ConfigCPURealtimeRuntime: 1000,
-			SysInfoCPURealtime:       true,
-			ErrorExpected:            false,
-			FailureMsg:               "Expected valid configuration",
+			doc: "empty configuration",
 		},
 		{
-			ConfigCPURealtimePeriod:  5000,
-			ConfigCPURealtimeRuntime: 5000,
-			SysInfoCPURealtime:       false,
-			ErrorExpected:            true,
-			FailureMsg:               "Expected failure when cpu-rt-period is set but kernel doesn't support it",
+			doc: "valid configuration",
+			resources: container.Resources{
+				CPURealtimePeriod:  1000,
+				CPURealtimeRuntime: 1000,
+			},
+			sysInfoCPURealtime: true,
 		},
 		{
-			ConfigCPURealtimePeriod:  5000,
-			ConfigCPURealtimeRuntime: 5000,
-			SysInfoCPURealtime:       false,
-			ErrorExpected:            true,
-			FailureMsg:               "Expected failure when cpu-rt-runtime is set but kernel doesn't support it",
+			doc: "cpu-rt-period not supported",
+			resources: container.Resources{
+				CPURealtimePeriod: 5000,
+			},
+			expectedError: "kernel does not support CPU real-time scheduler",
 		},
 		{
-			ConfigCPURealtimePeriod:  5000,
-			ConfigCPURealtimeRuntime: 10000,
-			SysInfoCPURealtime:       true,
-			ErrorExpected:            true,
-			FailureMsg:               "Expected failure when cpu-rt-runtime is greater than cpu-rt-period",
+			doc: "cpu-rt-runtime not supported",
+			resources: container.Resources{
+				CPURealtimeRuntime: 5000,
+			},
+			expectedError: "kernel does not support CPU real-time scheduler",
+		},
+		{
+			doc: "cpu-rt-runtime greater than cpu-rt-period",
+			resources: container.Resources{
+				CPURealtimePeriod:  5000,
+				CPURealtimeRuntime: 10000,
+			},
+			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 _, rt := range tests {
-		var hc container.HostConfig
-		hc.Resources.CPURealtimePeriod = rt.ConfigCPURealtimePeriod
-		hc.Resources.CPURealtimeRuntime = rt.ConfigCPURealtimeRuntime
+	for _, tc := range tests {
+		t.Run(tc.doc, func(t *testing.T) {
+			var hc container.HostConfig
+			hc.Resources = tc.resources
 
-		var si sysinfo.SysInfo
-		si.CPURealtime = rt.SysInfoCPURealtime
+			var si sysinfo.SysInfo
+			si.CPURealtime = tc.sysInfoCPURealtime
+			si.CPUShares = tc.sysInfoCPUShares
 
-		if err := validateResources(&hc, &si); (err != nil) != rt.ErrorExpected {
-			t.Fatal(rt.FailureMsg, err)
-		}
+			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
 }