From 94464e3a5e1dce0f6b3e821f79fe193278f67dba Mon Sep 17 00:00:00 2001 From: Antonio Murdaca Date: Tue, 8 Sep 2015 20:40:55 +0200 Subject: [PATCH] Validate --cpuset-cpus, --cpuset-mems Before this patch libcontainer badly errored out with `invalid argument` or `numerical result out of range` while trying to write to cpuset.cpus or cpuset.mems with an invalid value provided. This patch adds validation to --cpuset-cpus and --cpuset-mems flag along with validation based on system's available cpus/mems before starting a container. Signed-off-by: Antonio Murdaca --- daemon/daemon_unix.go | 20 +++++-- errors/daemon.go | 36 +++++++++++++ integration-cli/docker_api_containers_test.go | 26 ++++++++++ integration-cli/docker_cli_run_test.go | 14 +++++ integration-cli/docker_cli_run_unix_test.go | 41 +++++++++++++++ pkg/parsers/parsers.go | 52 ++++++++++++++++++- pkg/parsers/parsers_test.go | 38 ++++++++++++++ pkg/sysinfo/sysinfo.go | 39 ++++++++++++++ pkg/sysinfo/sysinfo_linux.go | 18 ++++++- pkg/sysinfo/sysinfo_test.go | 26 ++++++++++ 10 files changed, 303 insertions(+), 7 deletions(-) create mode 100644 pkg/sysinfo/sysinfo_test.go diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go index f0d68e1303..0cf425c1c4 100644 --- a/daemon/daemon_unix.go +++ b/daemon/daemon_unix.go @@ -15,6 +15,7 @@ import ( "github.com/docker/docker/autogen/dockerversion" "github.com/docker/docker/context" "github.com/docker/docker/daemon/graphdriver" + derr "github.com/docker/docker/errors" "github.com/docker/docker/pkg/fileutils" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/parsers/kernel" @@ -197,6 +198,20 @@ func verifyPlatformContainerSettings(ctx context.Context, daemon *Daemon, hostCo hostConfig.CpusetCpus = "" hostConfig.CpusetMems = "" } + cpusAvailable, err := sysInfo.IsCpusetCpusAvailable(hostConfig.CpusetCpus) + if err != nil { + return warnings, derr.ErrorCodeInvalidCpusetCpus.WithArgs(hostConfig.CpusetCpus) + } + if !cpusAvailable { + return warnings, derr.ErrorCodeNotAvailableCpusetCpus.WithArgs(hostConfig.CpusetCpus, sysInfo.Cpus) + } + memsAvailable, err := sysInfo.IsCpusetMemsAvailable(hostConfig.CpusetMems) + if err != nil { + return warnings, derr.ErrorCodeInvalidCpusetMems.WithArgs(hostConfig.CpusetMems) + } + if !memsAvailable { + return warnings, derr.ErrorCodeNotAvailableCpusetMems.WithArgs(hostConfig.CpusetMems, sysInfo.Mems) + } if hostConfig.BlkioWeight > 0 && !sysInfo.BlkioWeight { warnings = append(warnings, "Your kernel does not support Block I/O weight. Weight discarded.") logrus.Warnf("Your kernel does not support Block I/O weight. Weight discarded.") @@ -237,10 +252,7 @@ func checkSystem() error { if os.Geteuid() != 0 { return fmt.Errorf("The Docker daemon needs to be run as root") } - if err := checkKernel(); err != nil { - return err - } - return nil + return checkKernel() } // configureKernelSecuritySupport configures and validate security support for the kernel diff --git a/errors/daemon.go b/errors/daemon.go index 11d6d0b59a..07187d8bca 100644 --- a/errors/daemon.go +++ b/errors/daemon.go @@ -827,4 +827,40 @@ var ( Description: "While trying to delete a container, there was an error trying to delete one of its volumes", HTTPStatusCode: http.StatusInternalServerError, }) + + // ErrorCodeInvalidCpusetCpus is generated when user provided cpuset CPUs + // are invalid. + ErrorCodeInvalidCpusetCpus = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "INVALIDCPUSETCPUS", + Message: "Invalid value %s for cpuset cpus.", + Description: "While verifying the container's 'HostConfig', CpusetCpus value was in an incorrect format", + HTTPStatusCode: http.StatusInternalServerError, + }) + + // ErrorCodeInvalidCpusetMems is generated when user provided cpuset mems + // are invalid. + ErrorCodeInvalidCpusetMems = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "INVALIDCPUSETMEMS", + Message: "Invalid value %s for cpuset mems.", + Description: "While verifying the container's 'HostConfig', CpusetMems value was in an incorrect format", + HTTPStatusCode: http.StatusInternalServerError, + }) + + // ErrorCodeNotAvailableCpusetCpus is generated when user provided cpuset + // CPUs aren't available in the container's cgroup. + ErrorCodeNotAvailableCpusetCpus = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "NOTAVAILABLECPUSETCPUS", + Message: "Requested CPUs are not available - requested %s, available: %s.", + Description: "While verifying the container's 'HostConfig', cpuset CPUs provided aren't available in the container's cgroup available set", + HTTPStatusCode: http.StatusInternalServerError, + }) + + // ErrorCodeNotAvailableCpusetMems is generated when user provided cpuset + // memory nodes aren't available in the container's cgroup. + ErrorCodeNotAvailableCpusetMems = errcode.Register(errGroup, errcode.ErrorDescriptor{ + Value: "NOTAVAILABLECPUSETMEMS", + Message: "Requested memory nodes are not available - requested %s, available: %s.", + Description: "While verifying the container's 'HostConfig', cpuset memory nodes provided aren't available in the container's cgroup available set", + HTTPStatusCode: http.StatusInternalServerError, + }) ) diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index 07267c288c..2a5b2a3769 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -1525,3 +1525,29 @@ func (s *DockerSuite) TestContainersApiGetContainersJSONEmpty(c *check.C) { c.Fatalf("Expected empty response to be `[]`, got %q", string(body)) } } + +func (s *DockerSuite) TestPostContainersCreateWithWrongCpusetValues(c *check.C) { + testRequires(c, DaemonIsLinux) + + c1 := struct { + Image string + CpusetCpus string + }{"busybox", "1-42,,"} + name := "wrong-cpuset-cpus" + status, body, err := sockRequest("POST", "/containers/create?name="+name, c1) + c.Assert(err, check.IsNil) + c.Assert(status, check.Equals, http.StatusInternalServerError) + expected := "Invalid value 1-42,, for cpuset cpus.\n" + c.Assert(string(body), check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, string(body))) + + c2 := struct { + Image string + CpusetMems string + }{"busybox", "42-3,1--"} + name = "wrong-cpuset-mems" + status, body, err = sockRequest("POST", "/containers/create?name="+name, c2) + c.Assert(err, check.IsNil) + c.Assert(status, check.Equals, http.StatusInternalServerError) + expected = "Invalid value 42-3,1-- for cpuset mems.\n" + c.Assert(string(body), check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, string(body))) +} diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index dca1c0e2c5..f998e9561b 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -3407,3 +3407,17 @@ func (s *DockerSuite) TestRunStdinBlockedAfterContainerExit(c *check.C) { c.Fatal("timeout waiting for command to exit") } } + +func (s *DockerSuite) TestRunWrongCpusetCpusFlagValue(c *check.C) { + out, _, err := dockerCmdWithError("run", "--cpuset-cpus", "1-10,11--", "busybox", "true") + c.Assert(err, check.NotNil) + expected := "Error response from daemon: Invalid value 1-10,11-- for cpuset cpus.\n" + c.Assert(out, check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, out)) +} + +func (s *DockerSuite) TestRunWrongCpusetMemsFlagValue(c *check.C) { + out, _, err := dockerCmdWithError("run", "--cpuset-mems", "1-42--", "busybox", "true") + c.Assert(err, check.NotNil) + expected := "Error response from daemon: Invalid value 1-42-- for cpuset mems.\n" + c.Assert(out, check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, out)) +} diff --git a/integration-cli/docker_cli_run_unix_test.go b/integration-cli/docker_cli_run_unix_test.go index 84a5134078..4c2a8ebaf1 100644 --- a/integration-cli/docker_cli_run_unix_test.go +++ b/integration-cli/docker_cli_run_unix_test.go @@ -9,11 +9,14 @@ import ( "os" "os/exec" "path/filepath" + "strconv" "strings" "time" "github.com/docker/docker/pkg/integration/checker" "github.com/docker/docker/pkg/mount" + "github.com/docker/docker/pkg/parsers" + "github.com/docker/docker/pkg/sysinfo" "github.com/go-check/check" "github.com/kr/pty" ) @@ -370,3 +373,41 @@ func (s *DockerSuite) TestRunSwapLessThanMemoryLimit(c *check.C) { c.Fatalf("Expected output to contain %q, not %q", out, expected) } } + +func (s *DockerSuite) TestRunInvalidCpusetCpusFlagValue(c *check.C) { + testRequires(c, cgroupCpuset) + + sysInfo := sysinfo.New(true) + cpus, err := parsers.ParseUintList(sysInfo.Cpus) + c.Assert(err, check.IsNil) + var invalid int + for i := 0; i <= len(cpus)+1; i++ { + if !cpus[i] { + invalid = i + break + } + } + out, _, err := dockerCmdWithError("run", "--cpuset-cpus", strconv.Itoa(invalid), "busybox", "true") + c.Assert(err, check.NotNil) + expected := fmt.Sprintf("Error response from daemon: Requested CPUs are not available - requested %s, available: %s.\n", strconv.Itoa(invalid), sysInfo.Cpus) + c.Assert(out, check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, out)) +} + +func (s *DockerSuite) TestRunInvalidCpusetMemsFlagValue(c *check.C) { + testRequires(c, cgroupCpuset) + + sysInfo := sysinfo.New(true) + mems, err := parsers.ParseUintList(sysInfo.Mems) + c.Assert(err, check.IsNil) + var invalid int + for i := 0; i <= len(mems)+1; i++ { + if !mems[i] { + invalid = i + break + } + } + out, _, err := dockerCmdWithError("run", "--cpuset-mems", strconv.Itoa(invalid), "busybox", "true") + c.Assert(err, check.NotNil) + expected := fmt.Sprintf("Error response from daemon: Requested memory nodes are not available - requested %s, available: %s.\n", strconv.Itoa(invalid), sysInfo.Mems) + c.Assert(out, check.Equals, expected, check.Commentf("Expected output to contain %q, got %q", expected, out)) +} diff --git a/pkg/parsers/parsers.go b/pkg/parsers/parsers.go index 30b1932914..75d54c54c5 100644 --- a/pkg/parsers/parsers.go +++ b/pkg/parsers/parsers.go @@ -127,7 +127,7 @@ func PartParser(template, data string) (map[string]string, error) { out = make(map[string]string, len(templateParts)) ) if len(parts) != len(templateParts) { - return nil, fmt.Errorf("Invalid format to parse. %s should match template %s", data, template) + return nil, fmt.Errorf("Invalid format to parse. %s should match template %s", data, template) } for i, t := range templateParts { @@ -196,3 +196,53 @@ func ParseLink(val string) (string, string, error) { } return arr[0], arr[1], nil } + +// ParseUintList parses and validates the specified string as the value +// found in some cgroup file (e.g. `cpuset.cpus`, `cpuset.mems`), which could be +// one of the formats below. Note that duplicates are actually allowed in the +// input string. It returns a `map[int]bool` with available elements from `val` +// set to `true`. +// Supported formats: +// 7 +// 1-6 +// 0,3-4,7,8-10 +// 0-0,0,1-7 +// 03,1-3 <- this is gonna get parsed as [1,2,3] +// 3,2,1 +// 0-2,3,1 +func ParseUintList(val string) (map[int]bool, error) { + if val == "" { + return map[int]bool{}, nil + } + + availableInts := make(map[int]bool) + split := strings.Split(val, ",") + errInvalidFormat := fmt.Errorf("invalid format: %s", val) + + for _, r := range split { + if !strings.Contains(r, "-") { + v, err := strconv.Atoi(r) + if err != nil { + return nil, errInvalidFormat + } + availableInts[v] = true + } else { + split := strings.SplitN(r, "-", 2) + min, err := strconv.Atoi(split[0]) + if err != nil { + return nil, errInvalidFormat + } + max, err := strconv.Atoi(split[1]) + if err != nil { + return nil, errInvalidFormat + } + if max < min { + return nil, errInvalidFormat + } + for i := min; i <= max; i++ { + availableInts[i] = true + } + } + } + return availableInts, nil +} diff --git a/pkg/parsers/parsers_test.go b/pkg/parsers/parsers_test.go index d83722e8d5..49cbdb1634 100644 --- a/pkg/parsers/parsers_test.go +++ b/pkg/parsers/parsers_test.go @@ -1,6 +1,7 @@ package parsers import ( + "reflect" "runtime" "strings" "testing" @@ -238,3 +239,40 @@ func TestParseLink(t *testing.T) { t.Fatalf("Expected error 'bad format for links: link:alias:wrong' but got: %v", err) } } + +func TestParseUintList(t *testing.T) { + valids := map[string]map[int]bool{ + "": {}, + "7": {7: true}, + "1-6": {1: true, 2: true, 3: true, 4: true, 5: true, 6: true}, + "0-7": {0: true, 1: true, 2: true, 3: true, 4: true, 5: true, 6: true, 7: true}, + "0,3-4,7,8-10": {0: true, 3: true, 4: true, 7: true, 8: true, 9: true, 10: true}, + "0-0,0,1-4": {0: true, 1: true, 2: true, 3: true, 4: true}, + "03,1-3": {1: true, 2: true, 3: true}, + "3,2,1": {1: true, 2: true, 3: true}, + "0-2,3,1": {0: true, 1: true, 2: true, 3: true}, + } + for k, v := range valids { + out, err := ParseUintList(k) + if err != nil { + t.Fatalf("Expected not to fail, got %v", err) + } + if !reflect.DeepEqual(out, v) { + t.Fatalf("Expected %v, got %v", v, out) + } + } + + invalids := []string{ + "this", + "1--", + "1-10,,10", + "10-1", + "-1", + "-1,0", + } + for _, v := range invalids { + if out, err := ParseUintList(v); err == nil { + t.Fatalf("Expected failure with %s but got %v", v, out) + } + } +} diff --git a/pkg/sysinfo/sysinfo.go b/pkg/sysinfo/sysinfo.go index e62eadfcf4..851d176e4d 100644 --- a/pkg/sysinfo/sysinfo.go +++ b/pkg/sysinfo/sysinfo.go @@ -1,5 +1,7 @@ package sysinfo +import "github.com/docker/docker/pkg/parsers" + // SysInfo stores information about which features a kernel supports. // TODO Windows: Factor out platform specific capabilities. type SysInfo struct { @@ -63,4 +65,41 @@ type cgroupBlkioInfo struct { type cgroupCpusetInfo struct { // Whether Cpuset is supported or not Cpuset bool + + // Available Cpuset's cpus + Cpus string + + // Available Cpuset's memory nodes + Mems string +} + +// IsCpusetCpusAvailable returns `true` if the provided string set is contained +// in cgroup's cpuset.cpus set, `false` otherwise. +// If error is not nil a parsing error occurred. +func (c cgroupCpusetInfo) IsCpusetCpusAvailable(provided string) (bool, error) { + return isCpusetListAvailable(provided, c.Cpus) +} + +// IsCpusetMemsAvailable returns `true` if the provided string set is contained +// in cgroup's cpuset.mems set, `false` otherwise. +// If error is not nil a parsing error occurred. +func (c cgroupCpusetInfo) IsCpusetMemsAvailable(provided string) (bool, error) { + return isCpusetListAvailable(provided, c.Mems) +} + +func isCpusetListAvailable(provided, available string) (bool, error) { + parsedProvided, err := parsers.ParseUintList(provided) + if err != nil { + return false, err + } + parsedAvailable, err := parsers.ParseUintList(available) + if err != nil { + return false, err + } + for k := range parsedProvided { + if !parsedAvailable[k] { + return false, nil + } + } + return true, nil } diff --git a/pkg/sysinfo/sysinfo_linux.go b/pkg/sysinfo/sysinfo_linux.go index 44f10fae40..0ef3fbd858 100644 --- a/pkg/sysinfo/sysinfo_linux.go +++ b/pkg/sysinfo/sysinfo_linux.go @@ -126,7 +126,7 @@ func checkCgroupBlkioInfo(quiet bool) cgroupBlkioInfo { // checkCgroupCpusetInfo reads the cpuset information from the cpuset cgroup mount point. func checkCgroupCpusetInfo(quiet bool) cgroupCpusetInfo { - _, err := cgroups.FindCgroupMountpoint("cpuset") + mountPoint, err := cgroups.FindCgroupMountpoint("cpuset") if err != nil { if !quiet { logrus.Warn(err) @@ -134,7 +134,21 @@ func checkCgroupCpusetInfo(quiet bool) cgroupCpusetInfo { return cgroupCpusetInfo{} } - return cgroupCpusetInfo{Cpuset: true} + cpus, err := ioutil.ReadFile(path.Join(mountPoint, "cpuset.cpus")) + if err != nil { + return cgroupCpusetInfo{} + } + + mems, err := ioutil.ReadFile(path.Join(mountPoint, "cpuset.mems")) + if err != nil { + return cgroupCpusetInfo{} + } + + return cgroupCpusetInfo{ + Cpuset: true, + Cpus: strings.TrimSpace(string(cpus)), + Mems: strings.TrimSpace(string(mems)), + } } func cgroupEnabled(mountPoint, name string) bool { diff --git a/pkg/sysinfo/sysinfo_test.go b/pkg/sysinfo/sysinfo_test.go new file mode 100644 index 0000000000..b61fbcf541 --- /dev/null +++ b/pkg/sysinfo/sysinfo_test.go @@ -0,0 +1,26 @@ +package sysinfo + +import "testing" + +func TestIsCpusetListAvailable(t *testing.T) { + cases := []struct { + provided string + available string + res bool + err bool + }{ + {"1", "0-4", true, false}, + {"01,3", "0-4", true, false}, + {"", "0-7", true, false}, + {"1--42", "0-7", false, true}, + {"1-42", "00-1,8,,9", false, true}, + {"1,41-42", "43,45", false, false}, + {"0-3", "", false, false}, + } + for _, c := range cases { + r, err := isCpusetListAvailable(c.provided, c.available) + if (c.err && err == nil) && r != c.res { + t.Fatalf("Expected pair: %v, %v for %s, %s. Got %v, %v instead", c.res, c.err, c.provided, c.available, (c.err && err == nil), r) + } + } +}