Merge pull request #1537 from Ace-Tang/resource_check

feature: add container setting check

Author: allencloud

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
 }

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),
 		},
 	}
 

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<sup>-9</sup> CPUs."

apis/types/resources.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]")

cli/common_flags.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

daemon/mgr/container.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 (

daemon/mgr/container_types.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,125 +252,124 @@ 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
 		}
 	}
 
 	// 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{}
 		}
 	}
 
 	// 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
 		}
 	}
 
 	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
 	}

daemon/mgr/container_utils.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"