feature: add container setting check #1537
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1537 +/- ##
==========================================
+ Coverage 41.22% 41.29% +0.06%
==========================================
Files 266 266
Lines 17313 17313
==========================================
+ Hits 7138 7149 +11
+ Misses 9288 9275 -13
- Partials 887 889 +2
|
daemon/mgr/container_utils.go
Outdated
| if r.Memory != 0 && r.Memory < MinMemory { | ||
| return warnings, fmt.Errorf("Minimal memory should greater than 4M") | ||
| } | ||
| if r.Memory == 0 && r.MemorySwap > 0 && !update { |
There was a problem hiding this comment.
If it is the create action, the --memory-swap should also work with memory, right?
There was a problem hiding this comment.
Yes, we check only in create, and memory-swap should work with memory
daemon/mgr/container_utils.go
Outdated
| warn := "Current Kernel does not support memory limit, discard --memory" | ||
| logrus.Warn(warn) | ||
| warnings = append(warnings, warn) | ||
| r.Memory = 0 | ||
| r.MemorySwap = 0 | ||
| } | ||
| if r.MemorySwap != 0 && !cgroupInfo.Memory.MemorySwap { | ||
| if r.MemorySwap > 0 && !cgroupInfo.Memory.MemorySwap { | ||
| warn := "Current Kernel does not support memory swap, discard --memory-swap" |
There was a problem hiding this comment.
Could we put the warn as the const or package var? WDYT?
There was a problem hiding this comment.
Do you mean every warn make a const or warn ? will this too many?
daemon/mgr/container_utils.go
Outdated
| // 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" |
There was a problem hiding this comment.
Could we put the warn as the const or package var? WDYT?
daemon/mgr/container_utils.go
Outdated
| 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) | ||
| r.CPUPeriod = 0 | ||
| } | ||
| if r.CPUPeriod != 0 && (r.CPUPeriod < 1000 || r.CPUPeriod > 1000000) { |
There was a problem hiding this comment.
Since the number of if branch are too many, could we put the CPU related into one function? It can make the function readable....
test/cli_update_test.go
Outdated
| res.Assert(c, icmd.Success) | ||
|
|
||
| output := command.PouchRun("inspect", name).Stdout() | ||
| result := []types.ContainerJSON{} |
There was a problem hiding this comment.
I don't think we need result here. The inspect action should be tested by other suite.
There was a problem hiding this comment.
Yes, it can be removed.
|
PTAL, @fuweid |
|
LGTM. I think next step we should separate |
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 <aceapril@126.com>
|
LGTM |
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 aceapril@126.com
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews