From 6f229a54d291d19d705f19ef7f1baf02dd1491a5 Mon Sep 17 00:00:00 2001 From: Ace-Tang Date: Fri, 15 Jun 2018 15:15:32 +0800 Subject: [PATCH] feature: add container setting check add container config settings check, include container config and host config in create container, update container. fix memory_swappiness initial value to zero, since -1 is invalid. Signed-off-by: Ace-Tang --- apis/opts/memory_swappiness.go | 4 +- apis/opts/memory_swappiness_test.go | 6 +- apis/swagger.yml | 2 +- apis/types/resources.go | 4 +- cli/common_flags.go | 2 +- daemon/mgr/container.go | 13 +++- daemon/mgr/container_types.go | 53 +++++++++++++ daemon/mgr/container_utils.go | 114 ++++++++++++++-------------- test/cli_update_test.go | 20 +++++ 9 files changed, 151 insertions(+), 67 deletions(-) diff --git a/apis/opts/memory_swappiness.go b/apis/opts/memory_swappiness.go index 719aea3c1..a086590c1 100644 --- a/apis/opts/memory_swappiness.go +++ b/apis/opts/memory_swappiness.go @@ -6,8 +6,8 @@ import "fmt" // ValidateMemorySwappiness verifies the correctness of memory-swappiness. func ValidateMemorySwappiness(memorySwappiness int64) error { - if memorySwappiness != -1 && (memorySwappiness < 0 || memorySwappiness > 100) { - return fmt.Errorf("invalid memory swappiness: %d (its range is -1 or 0-100)", memorySwappiness) + if memorySwappiness < 0 || memorySwappiness > 100 { + return fmt.Errorf("invalid memory swappiness: %d (its range is 0-100)", memorySwappiness) } return nil } diff --git a/apis/opts/memory_swappiness_test.go b/apis/opts/memory_swappiness_test.go index 2bf5e5024..78155c759 100644 --- a/apis/opts/memory_swappiness_test.go +++ b/apis/opts/memory_swappiness_test.go @@ -16,7 +16,7 @@ func TestValidateMemorySwappiness(t *testing.T) { testCases := []TestCase{ { input: -1, - expected: nil, + expected: fmt.Errorf("invalid memory swappiness: %d (its range is 0-100)", -1), }, { input: 0, @@ -32,11 +32,11 @@ func TestValidateMemorySwappiness(t *testing.T) { }, { input: -5, - expected: fmt.Errorf("invalid memory swappiness: %d (its range is -1 or 0-100)", -5), + expected: fmt.Errorf("invalid memory swappiness: %d (its range is 0-100)", -5), }, { input: 200, - expected: fmt.Errorf("invalid memory swappiness: %d (its range is -1 or 0-100)", 200), + expected: fmt.Errorf("invalid memory swappiness: %d (its range is 0-100)", 200), }, } diff --git a/apis/swagger.yml b/apis/swagger.yml index a963253f1..8788b3983 100644 --- a/apis/swagger.yml +++ b/apis/swagger.yml @@ -2349,7 +2349,7 @@ definitions: description: "Tune a container's memory swappiness behavior. Accepts an integer between 0 and 100." type: "integer" format: "int64" - minimum: -1 + minimum: 0 maximum: 100 NanoCPUs: description: "CPU quota in units of 10-9 CPUs." diff --git a/apis/types/resources.go b/apis/types/resources.go index b60852534..2877d6fb3 100644 --- a/apis/types/resources.go +++ b/apis/types/resources.go @@ -127,7 +127,7 @@ type Resources struct { // Tune a container's memory swappiness behavior. Accepts an integer between 0 and 100. // Maximum: 100 - // Minimum: -1 + // Minimum: 0 MemorySwappiness *int64 `json:"MemorySwappiness,omitempty"` // MemoryWmarkRatio is an integer value representing this container's memory low water mark percentage. @@ -573,7 +573,7 @@ func (m *Resources) validateMemorySwappiness(formats strfmt.Registry) error { return nil } - if err := validate.MinimumInt("MemorySwappiness", "body", int64(*m.MemorySwappiness), -1, false); err != nil { + if err := validate.MinimumInt("MemorySwappiness", "body", int64(*m.MemorySwappiness), 0, false); err != nil { return err } diff --git a/cli/common_flags.go b/cli/common_flags.go index ee86b655e..9932f7d2b 100644 --- a/cli/common_flags.go +++ b/cli/common_flags.go @@ -50,7 +50,7 @@ func addCommonFlags(flagSet *pflag.FlagSet) *container { flagSet.StringVarP(&c.memory, "memory", "m", "", "Memory limit") flagSet.StringVar(&c.memorySwap, "memory-swap", "", "Swap limit equal to memory + swap, '-1' to enable unlimited swap") - flagSet.Int64Var(&c.memorySwappiness, "memory-swappiness", -1, "Container memory swappiness [0, 100]") + flagSet.Int64Var(&c.memorySwappiness, "memory-swappiness", 0, "Container memory swappiness [0, 100]") // for alikernel isolation options flagSet.Int64Var(&c.memoryWmarkRatio, "memory-wmark-ratio", 0, "Represent this container's memory low water mark percentage, range in [0, 100]. The value of memory low water mark is memory.limit_in_bytes * MemoryWmarkRatio") flagSet.Int64Var(&c.memoryExtra, "memory-extra", 0, "Represent container's memory high water mark percentage, range in [0, 100]") diff --git a/daemon/mgr/container.go b/daemon/mgr/container.go index 428b5b70e..f1c8be471 100644 --- a/daemon/mgr/container.go +++ b/daemon/mgr/container.go @@ -386,11 +386,14 @@ func (mgr *ContainerManager) Create(ctx context.Context, name string, config *ty } // validate container Config - warnings, err := validateConfig(config) + warnings, err := validateConfig(&config.ContainerConfig, config.HostConfig, false) if err != nil { return nil, err } + // amendContainerSettings modify container config settings to wanted + amendContainerSettings(&config.ContainerConfig, config.HostConfig) + // store disk if err := container.Write(mgr.Store); err != nil { logrus.Errorf("failed to update meta: %v", err) @@ -851,6 +854,14 @@ func (mgr *ContainerManager) Update(ctx context.Context, name string, config *ty return err } + warnings, err := validateResource(&config.Resources, true) + if err != nil { + return err + } + if len(warnings) != 0 { + logrus.Warnf("warnings update %s: %v", name, warnings) + } + restore := false configBack := *c.Config hostconfigBack := *c.HostConfig diff --git a/daemon/mgr/container_types.go b/daemon/mgr/container_types.go index b490f6c2e..9cd546c7f 100644 --- a/daemon/mgr/container_types.go +++ b/daemon/mgr/container_types.go @@ -20,6 +20,59 @@ var ( // GCExecProcessTick is the time interval to trigger gc unused exec config, // time unit is minute. GCExecProcessTick = 5 + + // MinMemory is minimal memory container should has + MinMemory int64 = 4194304 +) + +var ( + // MemoryWarn is warning for flag --memory + MemoryWarn = "Current Kernel does not support memory limit, discard --memory" + + // MemorySwapWarn is warning for flag --memory-swap + MemorySwapWarn = "Current Kernel does not support memory swap, discard --memory-swap" + + // MemorySwappinessWarn is warning for flag --memory-swappiness + MemorySwappinessWarn = "Current Kernel does not support memory swappiness , discard --memory-swappiness" + + //OOMKillWarn is warning for flag --oom-kill-disable + OOMKillWarn = "Current Kernel does not support disable oom kill, discard --oom-kill-disable" + + // CpusetCpusWarn is warning for flag --cpuset-cpus + CpusetCpusWarn = "Current Kernel does not support cpuset cpus, discard --cpuset-cpus" + + // CpusetMemsWarn is warning for flag --cpuset-mems + CpusetMemsWarn = "Current Kernel does not support cpuset mems, discard --cpuset-mems" + + // CPUSharesWarn is warning for flag --cpu-shares + CPUSharesWarn = "Current Kernel does not support cpu shares, discard --cpu-shares" + + // CPUQuotaWarn is warning for flag --cpu-quota + CPUQuotaWarn = "Current Kernel does not support cpu quota, discard --cpu-quota" + + // CPUPeriodWarn is warning for flag --cpu-period + CPUPeriodWarn = "Current Kernel does not support cpu period, discard --cpu-period" + + // BlkioWeightWarn is warning for flag --blkio-weight + BlkioWeightWarn = "Current Kernel does not support blkio weight, discard --blkio-weight" + + // BlkioWeightDeviceWarn is warning for flag --blkio-weight-device + BlkioWeightDeviceWarn = "Current Kernel does not support blkio weight device, discard --blkio-weight-device" + + // BlkioDeviceReadBpsWarn is warning for flag --device-read-bps + BlkioDeviceReadBpsWarn = "Current Kernel does not support blkio device throttle read bps, discard --device-read-bps" + + // BlkioDeviceWriteBpsWarn is warning for flag --device-write-bps + BlkioDeviceWriteBpsWarn = "Current Kernel does not support blkio device throttle write bps, discard --device-write-bps" + + // BlkioDeviceReadIOpsWarn is warning for flag --device-read-iops + BlkioDeviceReadIOpsWarn = "Current Kernel does not support blkio device throttle read iops, discard --device-read-iops" + + // BlkioDeviceWriteIOpsWarn is warning for flag --device-write-iops + BlkioDeviceWriteIOpsWarn = "Current Kernel does not support blkio device throttle, discard --device-write-iops" + + // PidsLimitWarn is warning for flag --pids-limit + PidsLimitWarn = "Current Kernel does not support pids cgroup, discard --pids-limit" ) const ( diff --git a/daemon/mgr/container_utils.go b/daemon/mgr/container_utils.go index d6671ca80..18f81fb34 100644 --- a/daemon/mgr/container_utils.go +++ b/daemon/mgr/container_utils.go @@ -227,22 +227,23 @@ func parsePSOutput(output []byte, pids []int) (*types.ContainerProcessList, erro } // validateConfig validates container config -func validateConfig(config *types.ContainerCreateConfig) ([]string, error) { - amendResource(&config.HostConfig.Resources) - +func validateConfig(config *types.ContainerConfig, hostConfig *types.HostConfig, update bool) ([]string, error) { // validates container hostconfig warnings := make([]string, 0) - warns, err := validateResource(&config.HostConfig.Resources) + warns, err := validateResource(&hostConfig.Resources, update) if err != nil { return nil, err } warnings = append(warnings, warns...) + if hostConfig.OomScoreAdj < -1000 || hostConfig.OomScoreAdj > 1000 { + return warnings, fmt.Errorf("oom score should be in range [-1000, 1000]") + } // TODO: add more validate here return warnings, nil } -func validateResource(r *types.Resources) ([]string, error) { +func validateResource(r *types.Resources, update bool) ([]string, error) { cgroupInfo := system.NewCgroupInfo() if cgroupInfo == nil { return nil, nil @@ -251,32 +252,34 @@ func validateResource(r *types.Resources) ([]string, error) { // validates memory cgroup value if cgroupInfo.Memory != nil { - if r.Memory != 0 && !cgroupInfo.Memory.MemoryLimit { - warn := "Current Kernel does not support memory limit, discard --memory" - logrus.Warn(warn) - warnings = append(warnings, warn) + if r.Memory > 0 && !cgroupInfo.Memory.MemoryLimit { + logrus.Warn(MemoryWarn) + warnings = append(warnings, MemoryWarn) r.Memory = 0 r.MemorySwap = 0 } - if r.MemorySwap != 0 && !cgroupInfo.Memory.MemorySwap { - warn := "Current Kernel does not support memory swap, discard --memory-swap" - logrus.Warn(warn) - warnings = append(warnings, warn) + if r.MemorySwap > 0 && !cgroupInfo.Memory.MemorySwap { + logrus.Warn(MemorySwapWarn) + warnings = append(warnings, MemorySwapWarn) r.MemorySwap = 0 } + if r.Memory != 0 && r.Memory < MinMemory { + return warnings, fmt.Errorf("Minimal memory should greater than 4M") + } if r.Memory > 0 && r.MemorySwap > 0 && r.MemorySwap < 2*r.Memory { warnings = append(warnings, "You should typically size your swap space to approximately 2x main memory for systems with less than 2GB of RAM") } if r.MemorySwappiness != nil && !cgroupInfo.Memory.MemorySwappiness { - warn := "Current Kernel does not support memory swappiness , discard --memory-swappiness" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(MemorySwappinessWarn) + warnings = append(warnings, MemorySwappinessWarn) r.MemorySwappiness = nil } + if r.MemorySwappiness != nil && (*r.MemorySwappiness < 0 || *r.MemorySwappiness > 100) { + return warnings, fmt.Errorf("MemorySwappiness should in range [-1, 100]") + } if r.OomKillDisable != nil && !cgroupInfo.Memory.OOMKillDisable { - warn := "Current Kernel does not support disable oom kill, discard --oom-kill-disable" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(OOMKillWarn) + warnings = append(warnings, OOMKillWarn) r.OomKillDisable = nil } } @@ -284,73 +287,69 @@ func validateResource(r *types.Resources) ([]string, error) { // validates cpu cgroup value if cgroupInfo.CPU != nil { if r.CpusetCpus != "" && !cgroupInfo.CPU.CpusetCpus { - warn := "Current Kernel does not support cpuset cpus, discard --cpuset-cpus" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(CpusetCpusWarn) + warnings = append(warnings, CpusetCpusWarn) r.CpusetCpus = "" } if r.CpusetMems != "" && !cgroupInfo.CPU.CpusetMems { - warn := "Current Kernel does not support cpuset cpus, discard --cpuset-mems" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(CpusetMemsWarn) + warnings = append(warnings, CpusetMemsWarn) r.CpusetMems = "" } if r.CPUShares > 0 && !cgroupInfo.CPU.CPUShares { - warn := "Current Kernel does not support cpu shares, discard --cpu-shares" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(CPUSharesWarn) + warnings = append(warnings, CPUSharesWarn) r.CPUShares = 0 } if r.CPUQuota > 0 && !cgroupInfo.CPU.CPUQuota { - warn := "Current Kernel does not support cpu quota, discard --cpu-quota" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(CPUQuotaWarn) + warnings = append(warnings, CPUQuotaWarn) r.CPUQuota = 0 } + // cpu.cfs_quota_us can accept value less than 0, we allow -1 and > 1000 + if r.CPUQuota > 0 && r.CPUQuota < 1000 { + return warnings, fmt.Errorf("CPU cfs quota should be greater than 1ms(1000)") + } if r.CPUPeriod > 0 && !cgroupInfo.CPU.CPUPeriod { - warn := "Current Kernel does not support cpu period, discard --cpu-period" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(CPUPeriodWarn) + warnings = append(warnings, CPUPeriodWarn) r.CPUPeriod = 0 } + if r.CPUPeriod != 0 && (r.CPUPeriod < 1000 || r.CPUPeriod > 1000000) { + return warnings, fmt.Errorf("CPU cfs period should be in range [1000, 1000000](1ms, 1s)") + } } // validates blkio cgroup value if cgroupInfo.Blkio != nil { if r.BlkioWeight > 0 && !cgroupInfo.Blkio.BlkioWeight { - warn := "Current Kernel does not support blkio weight, discard --blkio-weight" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioWeightWarn) + warnings = append(warnings, BlkioWeightWarn) r.BlkioWeight = 0 } if len(r.BlkioWeightDevice) > 0 && !cgroupInfo.Blkio.BlkioWeightDevice { - warn := "Current Kernel does not support blkio weight device, discard --blkio-weight-device" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioWeightDeviceWarn) + warnings = append(warnings, BlkioWeightDeviceWarn) r.BlkioWeightDevice = []*types.WeightDevice{} } if len(r.BlkioDeviceReadBps) > 0 && !cgroupInfo.Blkio.BlkioDeviceReadBps { - warn := "Current Kernel does not support blkio device throttle read bps, discard --device-read-bps" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioDeviceReadBpsWarn) + warnings = append(warnings, BlkioDeviceReadBpsWarn) r.BlkioDeviceReadBps = []*types.ThrottleDevice{} } if len(r.BlkioDeviceWriteBps) > 0 && !cgroupInfo.Blkio.BlkioDeviceWriteBps { - warn := "Current Kernel does not support blkio device throttle write bps, discard --device-write-bps" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioDeviceWriteBpsWarn) + warnings = append(warnings, BlkioDeviceWriteBpsWarn) r.BlkioDeviceWriteBps = []*types.ThrottleDevice{} } if len(r.BlkioDeviceReadIOps) > 0 && !cgroupInfo.Blkio.BlkioDeviceReadIOps { - warn := "Current Kernel does not support blkio device throttle read iops, discard --device-read-iops" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioDeviceReadIOpsWarn) + warnings = append(warnings, BlkioDeviceReadIOpsWarn) r.BlkioDeviceReadIOps = []*types.ThrottleDevice{} } if len(r.BlkioDeviceWriteIOps) > 0 && !cgroupInfo.Blkio.BlkioDeviceWriteIOps { - warn := "Current Kernel does not support blkio device throttle, discard --device-write-iops" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(BlkioDeviceWriteIOpsWarn) + warnings = append(warnings, BlkioDeviceWriteIOpsWarn) r.BlkioDeviceWriteIOps = []*types.ThrottleDevice{} } } @@ -358,9 +357,8 @@ func validateResource(r *types.Resources) ([]string, error) { // validates pid cgroup value if cgroupInfo.Pids != nil { if r.PidsLimit != 0 && !cgroupInfo.Pids.Pids { - warn := "Current Kernel does not support pids cgroup, discard --pids-limit" - logrus.Warn(warn) - warnings = append(warnings, warn) + logrus.Warn(PidsLimitWarn) + warnings = append(warnings, PidsLimitWarn) r.PidsLimit = 0 } } @@ -368,8 +366,10 @@ func validateResource(r *types.Resources) ([]string, error) { return warnings, nil } -// amendResource modify resource to correct setting. -func amendResource(r *types.Resources) { +// amendContainerSettings modify config settings to wanted, +// it will be call before container created. +func amendContainerSettings(config *types.ContainerConfig, hostConfig *types.HostConfig) { + r := &hostConfig.Resources if r.Memory > 0 && r.MemorySwap == 0 { r.MemorySwap = 2 * r.Memory } diff --git a/test/cli_update_test.go b/test/cli_update_test.go index 1723c3b20..a9ca24b13 100644 --- a/test/cli_update_test.go +++ b/test/cli_update_test.go @@ -115,6 +115,26 @@ func (suite *PouchUpdateSuite) TestUpdateCpuPeriod(c *check.C) { c.Assert(metaJSON[0].HostConfig.CPUPeriod, check.Equals, int64(2000)) } +// TestUpdateCpuMemoryFail is to verify the invalid value of updating container cpu and memory related flags will fail. +func (suite *PouchUpdateSuite) TestUpdateCpuMemoryFail(c *check.C) { + name := "update-container-cpu-memory-period-fail" + + res := command.PouchRun("run", "-d", "--name", name, busyboxImage, "top") + defer DelContainerForceMultyTime(c, name) + res.Assert(c, icmd.Success) + + res = command.PouchRun("update", "--cpu-period", "10", name) + c.Assert(res.Stderr(), check.NotNil) + res = command.PouchRun("update", "--cpu-period", "100000000", name) + c.Assert(res.Stderr(), check.NotNil) + res = command.PouchRun("update", "--cpu-period", "-1", name) + c.Assert(res.Stderr(), check.NotNil) + res = command.PouchRun("update", "--cpu-quota", "1", name) + c.Assert(res.Stderr(), check.NotNil) + res = command.PouchRun("update", "-m", "10000", name) + c.Assert(res.Stderr(), check.NotNil) +} + // TestUpdateRunningContainer is to verify the correctness of updating a running container. func (suite *PouchUpdateSuite) TestUpdateRunningContainer(c *check.C) { name := "update-running-container"