libct: trim and de-mock container state unit tests - #5382
Conversation
40e58f7 to
3ecafb6
Compare
There was a problem hiding this comment.
Pull request overview
This PR simplifies libcontainer’s container state unit tests by removing mock-based and redundant coverage, and rewriting the remaining meaningful test to use a real cgroup v2 (fs2) manager operating against a temporary directory (via cgroups.TestMode).
Changes:
- Removed
mockCgroupManagerand two low-value tests (TestGetContainerPids,TestGetContainerState). - Added
newFakeCgroupManagerto create a realfs2cgroup manager backed by a temp directory. - Updated
TestGetContainerStateAfterUpdateto validate persisted state and confirm the updated memory limit is written tomemory.max.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Helper() | ||
| cgroups.TestMode = true | ||
| t.Cleanup(func() { cgroups.TestMode = false }) |
932697f to
765b657
Compare
rata
left a comment
rata
left a comment
There was a problem hiding this comment.
Sorry for the quick review. I think the remaining test is not really doing much. But not 100% sure, I'm in a rush now :D
| func newFakeCgroupManager(t *testing.T, config *cgroups.Cgroup) (cgroups.Manager, string) { | ||
| t.Helper() | ||
| cgroups.TestMode = true | ||
| t.Cleanup(func() { cgroups.TestMode = false }) |
There was a problem hiding this comment.
Should we store what it was set to, and restore that on defer? To be good neighbors.
This cgroup global thing makes me think that this can never run in parallel. Let's add a comment to the function doc, just in case?
There was a problem hiding this comment.
Should we store what it was set to, and restore that on defer? To be good neighbors.
Fixed (was already reported by copilot, #5382 (comment))
This cgroup global thing makes me think that this can never run in parallel. Let's add a comment to the function doc, just in case?
Ideally there should be a way to mark a test case as being unable to run in parallel. The only way I know of lives inside the testing package (for example when you use t.Setenv() it internally marks the test case and then t.Parallel panics). I wish there was a way to do so but currently it's just a proposal (golang/go#78614).
As a kludge we could use t.Setenv("USES_cgroups.TestMode", "") or somesuch, but this is quite ugly.
Added a note to newFakeCgroupManager docstring. @rata let me know if you prefer using the kludge.
There was a problem hiding this comment.
The remaining test I'm not sure how much is really testing, I guess we need to improve it here.
I don't have a lot of time to see this now. But it seems this is sharing pointers and then not testing much?
This makes the test not really testing much, IIUC. Because we do a shallow copy in newConfig := ..., then newConfig.Cgroups is a pointer that is shared with the same pointer the internal libct is using. Then we are just modifying the internal struct of libct.
Then all reads will pass just fine. I think we should assign a new struct here for the test to be more meaningful.
But not sure if I'm missing something
There was a problem hiding this comment.
To add some context, the test was added by commit e6e1c34. And I guess you're right -- in its current form it's useless, too :(
Fixed this (in a separate commit on top so you don't have to re-review).
Alternatively, we can just drop all this useless-in-its-current-form-nonsense tests altogether.
Removal of mockCgroupManager is needed so that oc/cgroups can add more methods without breaking runc compatibility. Remove the useless tests: - TestGetContainerPids merely asserted that Container.Processes returns what the mock cgroup manager was configured to return; - TestGetContainerState reimplemented the namespace type to /proc file name mapping it was meant to verify, so it could only detect drift between the two copies. Keep the useful one: - TestGetContainerStateAfterUpdate (added by commit e6e1c34). The test round-trips the container state through a real state directory and is the only unit level coverage of Set, saveState, updateState, and currentState. Rewrite it to use real fs2 cgroup manager instead of a mock. Note that in its current form the test is not particularly sound; the next commit explains and fixes it. Package coverage goes from 7.4% to 7.3%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of what the test asserts after calling Set could not fail, because
the config it passes to Set is aliased to the one the container already
holds, in two independent ways:
1. Container.Config returns a shallow copy, so newConfig.Cgroups is the
very *cgroups.Cgroup the container config points to, and setting
Memory on it modified the container config before Set was even
called. This is as old as the test itself (commit e6e1c34).
2. fs2.Manager.Set ends with "m.config.Resources = r", and the manager
was handed the same *cgroups.Cgroup as the container config, so the
manager installed the new Resources into the container config as a
side effect. This one appeared once the test switched to a real
cgroup manager in the previous commit.
Fix both: give newConfig its own Cgroups/Resources, and construct the
manager from a separate (but equal) instance. In real life these are the
same object, so the split is only there to let the test tell whether Set
did its job.
Also, check that the new value made it to state.json, rather than merely
asserting that saveState did not return an error.
With this, removing "c.config = &config" from Set, or skipping the
saveState call in updateState, each make the test fail; before, both
mutations passed.
Reported-by: Rodrigo Campos <rodrigo@amutable.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
765b657 to
c437b9b
Compare
|
@rata sorry for the late reply; your comments are valuable and were addressed; I still think maybe we should just drop all this entirely as we have a bunch of integration tests that tests the same thing anyway. |
Related to opencontainers/cgroups#61.
The removal of mockCgroupManager is needed so oc/cgroups can add more methods without breaking runc.
Remove the useless tests:
TestGetContainerPidsmerely asserted that Container.Processes returns what the mock cgroup manager was configured to return;TestGetContainerStatereimplemented the namespace type to /proc file name mapping it was meant to verify, so it could only detect drift between the two copies.Keep the useful one:
TestGetContainerStateAfterUpdate(initially added by commit e6e1c34)The test round-trips the container state through a real state directory and is the only unit level coverage of Set, saveState, updateState, and currentState. Rewrite it to use real fs2 cgroup manager instead of a mock.
Note that in its current form the test is not particularly sound; the second commit explains and fixes it.
Package coverage goes from 7.4% to 7.3%.