Update spec to master, switch to int64 for memory limits #1495
Conversation
| @@ -128,29 +126,29 @@ func setMemoryAndSwap(path string, cgroup *configs.Cgroup) error { | |||
| // When update memory limit, we should adapt the write sequence | |||
| // for memory and swap memory, so it won't fail because the new | |||
| // value and the old value don't fit kernel's validation. | |||
| if cgroup.Resources.MemorySwap == uint64(ulimited) || memoryUsage.Limit < cgroup.Resources.MemorySwap { | |||
| if err := writeFile(path, cgroupMemorySwapLimit, strconv.FormatUint(cgroup.Resources.MemorySwap, 10)); err != nil { | |||
| if cgroup.Resources.MemorySwap == -1 || memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) { | |||
There was a problem hiding this comment.
memoryUsage.Limit < uint64(cgroup.Resources.MemorySwap) is weird
There was a problem hiding this comment.
I think I understand what's going on: since memoryUsage.Limit is a uint64 (since read from fs), we have to choose between casting that to int64 (no problem since getMemory should not return numbers bigger than 2^63-1), or casting an int64 that's different from -1, to uint64. Would be great to add a comment about the casting.
Which brings up the question: have we ensured that the int64s are never < -1 ?
There was a problem hiding this comment.
-1 should be the only valid negative value, but I don't think we should ensure that in runc, cgroup API will return "invalid argument" in this case anyway.
|
@justincormack can you rebase this to pick up the CI fix? |
Updates memory limits to be int64, and removes Platform from spec. Signed-off-by: Justin Cormack <justin.cormack@docker.com>
This was never used, just validated, so was removed from spec. Signed-off-by: Justin Cormack <justin.cormack@docker.com>
replace opencontainers#1492 opencontainers#1494 fix opencontainers#1422 Since opencontainers/runtime-spec#876 the memory specifications are now `int64`, as that better matches the visible interface where `-1` is a valid value. Otherwise finding the correct value was difficult as it was kernel dependent. Signed-off-by: Justin Cormack <justin.cormack@docker.com>
justincormack
force-pushed
the
memory-int64
branch
from
48f04f8
to
3d9074e
Compare
|
rebased |
|
LGTM |
1 similar comment
|
LGTM |
Update memory specs to use int64 not uint64
replace #1492 #1494
fix #1422
Since opencontainers/runtime-spec#876 the memory
specifications are now
int64, as that better matches the visible interface where-1is a valid value. Otherwise finding the correct value was difficult as itwas kernel dependent.
Also in the update of the spec,
Platformwas removed, so remove some code.