feature: add kernel-memory support in cli and ctrd #2717
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2717 +/- ##
=========================================
Coverage ? 69.29%
=========================================
Files ? 277
Lines ? 17433
Branches ? 0
=========================================
Hits ? 12081
Misses ? 4018
Partials ? 1334
|
allencloud
force-pushed
the
add-kmem
branch
2 times, most recently
from
834e504
to
405dfe9
Compare
| @@ -35,73 +35,88 @@ func (suite *PouchRunMemorySuite) TearDownTest(c *check.C) { | |||
|
|
|||
| // TestRunWithMemoryswap is to verify the valid running container | |||
| // with --memory-swap | |||
| func (suite *PouchRunMemorySuite) TestRunWithMemoryswap(c *check.C) { | |||
| func (suite *PouchRunMemorySuite) TestRunWithMemoryswapAndKernelMemory(c *check.C) { | |||
There was a problem hiding this comment.
could we add the case for default value of kernel memory?
| path = fmt.Sprintf("/sys/fs/cgroup/memory/default/%s/memory.memsw.limit_in_bytes", containerID) | ||
| checkFileContains(c, path, expectedMemSwap) | ||
| path = fmt.Sprintf("/sys/fs/cgroup/memory/default/%s/memory.kmem.usage_in_bytes", containerID) | ||
| checkFileContains(c, path, expectedKernelMem) |
There was a problem hiding this comment.
something wrong here
FAIL: cli_run_memory_test.go:38: PouchRunMemorySuite.TestRunWithMemoryswapAndKernelMemory
cli_run_memory_test.go:79:
checkFileContains(c, path, expectedKernelMem)
cli_run_test.go:248:
c.Assert(strings.Contains(string(cmdResult.Stdout()), expt),
check.Equals, true)
... obtained bool = false
... expected bool = true
There was a problem hiding this comment.
Oh, I should use memory.kmem.limit_in_bytes rather than memory.kmem.usage_in_bytes. Updated.
apis/opts/kernel_memory.go
Outdated
| import units "github.com/docker/go-units" | ||
|
|
||
| // ParseKernelMemory parses the kernel memory param of container. | ||
| func ParseKernelMemory(kmem string) (int64, error) { |
There was a problem hiding this comment.
Can we get all memory-related functions into one file, xxx/memory.go, there are too many file now
There was a problem hiding this comment.
good idea. Could we do that in a separate PR?
test/cli_run_memory_test.go
Outdated
| sleep := "10000" | ||
| kernelMemory := "100m" | ||
| expectedMem := strconv.Itoa(100 * 1024 * 1024) // 100 MB | ||
| expectedKernelMem := strconv.Itoa(1024 * 1024 * 100) // 100 MB |
There was a problem hiding this comment.
I think kernel memory test should be single test
There was a problem hiding this comment.
Actually both are OK. My concern is that if we combine the test case, we can reduce the CI time. It is a tradeoff.
|
@allencloud any update here? |
|
Could we kick the ball and make this move on? I think it will influence the @ireneYang 's scenario. |
Signed-off-by: Allen Sun <allensun.shl@alibaba-inc.com>
Signed-off-by: Allen Sun allensun.shl@alibaba-inc.com
Ⅰ. Describe what this PR did
This feature add kernel-memory support in cli part and containerd part.
Actually we have add KernelMemory in swagger.yml in https://github.com/alibaba/pouch/blob/master/apis/swagger.yml#L2676. Then in the API side, this field has been supported in the struct definition of
Resource.However in the daemon side, we have not set it in the LinuxResource of runc. Then there is no way to set kernel memory for PouchContainer.
This PR tries to fix that, and this pr did:
Ⅱ. Does this pull request fix one issue?
fix #2716
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
added
Ⅳ. Describe how to verify it
none
Ⅴ. Special notes for reviews
none