opencontainers
Add memory policy support
Implement support for Linux memory policy in OCI spec PR: https://github.com/opencontainers/runtime-spec/pull/1282
TODO:
- remove the replace from go.mod when OCI spec is merged
@AkihiroSuda, @giuseppe, do you think it would be good to keep this PR as Draft until https://github.com/opencontainers/runtime-spec/pull/1282 is merged? Or should I make it Ready for review (despite the "replace" in go.mod) in order to get reviews and possibly get this tagged into v1.3?
@AkihiroSuda, as the memory policy field is now in place in the runtime-spec (https://github.com/opencontainers/runtime-spec/pull/1282 merged), I updated this PR to use the latest runtime-spec and removed the Draft status. Rebased and fixed lint issues. Good for review now. :)
Careful with the milestones. According to the new release policy, features should by merged by rc1, which is due by the end of the month.
This is marked for 1.4.0 but I plan to cut -rc1 this week and there are still some outstanding points. Do we want to merge this as-is and fix it in -rc2 or punt it for 1.5.0 (to be released in April 2026)?
IMHO let's continue with the plans to release rc.1. If this is ready before that, we include it, otherwise we then decide if we want to merge and polish during the rc phase or just wait for 1.5.
@lifubang, CI reports lint errors none of which are not related to this PR. I hope it wouldn't block approving this.
@lifubang, CI reports lint errors none of which are not related to this PR. I hope it wouldn't block approving this.
This is a bug in golangci-lint https://github.com/golangci/golangci-lint/issues/5979. We'll re-run the linters.
@kolyshkin, sorry to bother you folks with this, but I'd have a question on staticcheck lint in the CI. It runs twice, but with different set of checks. That causes an issue: one cannot disable staticcheck for a line in the case that the line passes one check but not the other. Disabling a check that does not cause an error is itself an error, which makes sense.
In order to workaround this issue, I added commit https://github.com/opencontainers/runc/pull/4726/commits/1b32673b29f29cc4f9e818a4946498c7aa340d71 in this PR, so that we can see all checks pass. But I don't know what is the best way forward. Should I remove the commit before this PR gets merged?
Should I remove the commit before this PR gets merged?
Yes, please remove this commit. The CI is fully passing in #4900.
Should I remove the commit before this PR gets merged?
Yes, please remove this commit. The CI is fully passing in #4900.
Removed.
@askervin somewhat related to this PR; can you please review https://go-review.googlesource.com/c/sys/+/706917/2?
Should I remove the commit before this PR gets merged?
Yes, please remove this commit. The CI is fully passing in #4900.
Removed.
Could you please help fix the lint errors?
Error: libcontainer/configs/memorypolicy.go:7:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck)
MPOL_DEFAULT = iota
^
Error: libcontainer/configs/memorypolicy.go:8:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck)
MPOL_PREFERRED
^
Error: libcontainer/configs/memorypolicy.go:9:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck)
MPOL_BIND
^
Should I remove the commit before this PR gets merged?
Yes, please remove this commit. The CI is fully passing in #4900.
Removed.
Could you please help fix the lint errors?
Error: libcontainer/configs/memorypolicy.go:7:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck) MPOL_DEFAULT = iota ^ Error: libcontainer/configs/memorypolicy.go:8:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck) MPOL_PREFERRED ^ Error: libcontainer/configs/memorypolicy.go:9:2: ST1003: should not use ALL_CAPS in Go names; use CamelCase instead (staticcheck) MPOL_BIND ^
@lifubang, I fixed these errors with this commit: https://github.com/opencontainers/runc/commit/1b32673b29f29cc4f9e818a4946498c7aa340d71
The root cause is that CI runs the staticcheck lint twice, first without and then with ST1003. The first run checks all the code, the latter check only changed parts. If I disable this in code (nolint), I'll get an error from the first run: you're disabling a check without a reason. If I don't disable it, I'll get the errors quoted above. Options that I see are:
- modify both CI staticchecks always run without ST1003 (implemented in the commit).
- modify both CI staticchecks always run with ST1003. This requires many changes in the code base to fix existing errors.
- stop using ALL_CAPS in these constants. This would be against practices in this code base and in the go standard library (sys/unix) where all C flags are use ALL_CAPS without exception. So I wouldn't really prefer this option, as we are working around CI lint issues by breaking the code.
- merge the commit even if CI lint gives these warnings and fix CI lint later.
Of course there may be more options that I just fail to see. What would you suggest?
What would you suggest?
In fact, we always use CamelCase for const names in runc, but as you say, it will be in go standard library (sys/unix), so I suggest:
//nolint:revive // this will matches the unix.* name in the future
What would you suggest?
In fact, we always use
CamelCasefor const names in runc, but as you say, it will be in go standard library (sys/unix), so I suggest://nolint:revive // this will matches the unix.* name in the future
@lifubang, //nolint:revive does not silence staticcheck error. And //nolint:revive,staticcheck does not work because nolintlint complains about disabling staticcheck without effect as it does not produce an error in the first pass (when ST1003 is disabled in .golangci.yml). However, it turned out that disabling all three works. That's why you will see //nolint:revive,staticcheck,nolintlint. Included your suggestion to the comment in disabling these lint modules. Thanks for pointing me back to this, happy to get it fixed without tweaking CI lint configs.
A side note: ALL_CAPS C header constants in the runc code base do exist, like in libcontainer/configs/{mount.go,namespaces_linux.go}. And if we'd like to enable ST1003 in the first pass, we would also need to fix ALL_CAPS internal constants in utils_linux.go:
CT_ACT_CREATE CtAct = iota + 1
CT_ACT_RUN
CT_ACT_RESTORE
However, fixing (or nolinting) ALL_CAPS cases would not suffice, as ST1003 wants few other naming changes, too. Like "id"s capitalized (like ttyUid -> ttyUID), but that's another story.
The nolintlint issue is quite annoying, I've hit it before as well. Adding nolintlint is the only way I found to work around it as well. The only real long-term solution is to go through and fix all of the lint errors in a mega-PR so we don't have these two separate runs of golangci-lint -- I started this with #4904 but plan to eventually do all of the lint failures once I have some more time.
This has 3 reviews, so I'm happy to merge it.
@askervin Did you still want this in runc 1.4?
@kolyshkin @AkihiroSuda @lifubang Would you be happy for us to add this in rc2, or do you want to push this for 1.5 in April?
I think having it in v1.4 is fine since it's a new functionality and the chance it will bring a regression is minimal.
@askervin Did you still want this in runc 1.4?
Sure, @cyphar! Glad to see the functionality already in v1.4.
Thanks for all the great comments, @AkihiroSuda, @lifubang, @cyphar and @kolyshkin!
@askervin I noticed there's a recent commit (golang/sys@28c5bda) titled "unix: add SetMemPolicy and its mode/flag values" referenced in #4993. Would you be interested in opening a PR to update your current implementation accordingly?
@lifubang I already did that in a0e809a8ba6a61e3f4490132ef169ac991281e0b (I pushed it to #4993).