add memory cgroup controller #16
Conversation
|
@tsturzl |
|
Things should be working now. I think I'd like to wait until the hugetlb support is merged in since the subsystem changes that were made are more elegant than what I did. I'll just integrate with those changes once they're merged in. |
|
Thank you very much. The tests all passed 🎉 |
|
@tsturzl |
|
Should be good to go! If you wanna run the CI quick I confirmed that the CI is passing on my fork(https://github.com/tsturzl/youki/runs/2652443431). One change to note is my final commit. I noticed searching for the mounts and cgroup subsystems the filter->collect->pop routine was probably not needed. I might be wrong, but it seems like the vector that gets created by both is just going to be one element long. This probably creates some additional overhead and seems unneeded, so my last commit includes a change that simply replaces this with the |
|
That's right! It's nice to work!
|
| match Self::set_i64(val, &path) { | ||
| Ok(_) => Ok(()), | ||
| Err(e) => { | ||
| // we need to look into the raw OS error for an EBUSY status |
There was a problem hiding this comment.
This was a tough one. Thank you very much 👍
There was a problem hiding this comment.
@utam0k the challenge was fun! I tried to follow the runc code as cloesly as I could on this one since there were a few caveats, but it was a good learning experience.
There was a problem hiding this comment.
That was so good! I'm learning from your PR review now too.
src/cgroups/memory.rs
Outdated
| let limit = resource.limit.unwrap_or(-1); | ||
| let mut swap = resource.swap.unwrap_or(0); | ||
|
|
||
| if limit == -1 && swap == 0 { | ||
| if cgroup_root.join(CGROUP_MEMORY_SWAP_LIMIT).exists() { | ||
| swap = -1; | ||
| } | ||
| } | ||
|
|
||
| // According to runc we need to change the write sequence of | ||
| // limit and swap so it won't fail, because the new and old | ||
| // values don't fit the kernel's validation | ||
| // see: | ||
| // https://github.com/opencontainers/runc/blob/master/libcontainer/cgroups/fs/memory.go#L89 | ||
| if limit != 0 && swap != 0 { | ||
| let current_limit = Self::get_memory_limit(cgroup_root)?; | ||
|
|
||
| if swap == -1 || current_limit < swap { | ||
| Self::set_swap(swap, cgroup_root)?; | ||
| Self::set_memory(limit, cgroup_root)?; | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
I think it would be easier to use Option to make the code easier.
Also, swap can turn off mut by determining the value at the time of allocation.
|
@tsturzl |
|
@utam0k I think I can just give this a final pass and clean up the code in general. I'll make the changes soon and push. |
src/cgroups/memory.rs
Outdated
| // limit and swap so it won't fail, because the new and old | ||
| // values don't fit the kernel's validation | ||
| // see: | ||
| // https://github.com/opencontainers/runc/blob/master/libcontainer/cgroups/fs/memory.go#L89 |
There was a problem hiding this comment.
This is a very good comment, but it can be made even better by making it a permalink.
|
Ok, so a couple things. I replaced the 2 previous type specific I also cleaned up the mess of code that was looking for the EBUSY errno. Let me know what you think. |
|
@tsturzl I think this is good enough for this PR. |
Solves part of #9. Should be passing runtime-tools tests, as well as included unit tests.