Added unified cgroupv2 setting to updateContainerResources #9820
Added unified cgroupv2 setting to updateContainerResources #9820openshift-merge-bot[bot] merged 2 commits into
Conversation
|
Hi @PannagaRao. Thanks for your PR. I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds propagation of cgroup v2 "unified" resource entries into OCI/Linux resource structures during resource translation and container spec updates; introduces a CLI test binary and integration test to exercise dynamic updates of unified cgroup v2 settings. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "updateunified (CLI)"
participant CRI as "CRI gRPC (unix socket)"
participant Server as "container server"
participant OCI as "OCI runtime / cgroup v2"
CLI->>CRI: Connect + UpdateContainerResources(containerID, Linux.Unified map)
CRI->>Server: Forward UpdateContainerResources request
Server->>Server: toOCIResources -> allocate update.Unified and maps.Copy entries
Server->>OCI: Apply updated spec (including Unified entries)
OCI->>Server: Ack / updated cgroup v2 files
Server->>CRI: RPC response OK
CRI->>CLI: Print "OK"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9820 +/- ##
===========================================
+ Coverage 46.49% 67.46% +20.96%
===========================================
Files 180 212 +32
Lines 26670 29697 +3027
===========================================
+ Hits 12401 20035 +7634
+ Misses 13070 7953 -5117
- Partials 1199 1709 +510 🚀 New features to boost your workflow:
|
ad6220d to
1efe6bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/container_update_resources_test.go`:
- Around line 85-118: The test "should succeed with unified cgroup v2 resources"
assumes unified resources are applied only on cgroup v2 hosts; update the test
to detect the host cgroup mode and skip the test when running on cgroup v1:
before calling sut.UpdateContainerResources, add a guard that checks cgroup
version (e.g., via a helper like isCgroup2() or by probing /sys/fs/cgroup for
cgroup.controllers) and call Skip or t.Skip when not cgroup v2 so the assertions
on c.Spec().Linux.Resources.Unified only run on cgroup v2 systems; ensure you
reference the same test identifiers (sut.UpdateContainerResources,
types.UpdateContainerResourcesRequest, testContainer) when adding the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc3a8093-03d6-4057-bcdc-30f200c17772
📒 Files selected for processing (3)
internal/lib/container_server.goserver/container_update_resources.goserver/container_update_resources_test.go
Signed-off-by: Pannaga Rao Bhoja Ramamanohara
1efe6bc to
cd2b28f
Compare
|
/ok-to-test |
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/updateunified/updateunified.go (1)
37-44: Minor: Error message is slightly misleading.
grpc.NewClientcreates a client but doesn't establish a connection until the first RPC call. The error at line 42 relates to client creation failure (e.g., invalid target string), not a connection failure. Consider adjusting the message for clarity.Suggested fix
conn, err := grpc.NewClient( "unix://"+socket, grpc.WithTransportCredentials(insecure.NewCredentials()), ) if err != nil { - fmt.Fprintf(os.Stderr, "failed to connect: %v\n", err) + fmt.Fprintf(os.Stderr, "failed to create client: %v\n", err) os.Exit(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/updateunified/updateunified.go` around lines 37 - 44, The error message incorrectly says "failed to connect" though grpc.NewClient only creates a client; update the fmt.Fprintf call to reflect client-creation failure (e.g., "failed to create gRPC client: %v") where grpc.NewClient is invoked and err is checked, so the log accurately describes the failure before exiting (adjust the message involving conn and err accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/cgroups.bats`:
- Around line 221-224: The assertion for memory.min is too loose: change the
substring check on the variable output (from the crictl exec call that reads
/sys/fs/cgroup/memory.min) to an exact/anchored comparison so it only passes
when the file equals "0" (e.g., use an exact string equality or numeric compare
after trimming whitespace on output) instead of the current wildcard pattern;
keep the memory.high check as-is.
---
Nitpick comments:
In `@test/updateunified/updateunified.go`:
- Around line 37-44: The error message incorrectly says "failed to connect"
though grpc.NewClient only creates a client; update the fmt.Fprintf call to
reflect client-creation failure (e.g., "failed to create gRPC client: %v") where
grpc.NewClient is invoked and err is checked, so the log accurately describes
the failure before exiting (adjust the message involving conn and err
accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbb5a709-78a4-418b-962a-d28829dc2129
📒 Files selected for processing (4)
Makefiletest/cgroups.batstest/common.shtest/updateunified/updateunified.go
7ee33a4 to
9c5764a
Compare
|
/retest |
|
/retest |
sohankunkerkar
left a comment
There was a problem hiding this comment.
Overall, it looks good!
Could you add a unit test to check if toOCIResources persists the spec?
I have already added a test to see if the spec is persisted in server/container_update_resources_test.go. Could you please elaborate on where else it should be checked? |
3dc241a to
d41f8ec
Compare
Signed-off-by: Pannaga Rao Bhoja Ramamanohara
d41f8ec to
456e2e7
Compare
bitoku
left a comment
There was a problem hiding this comment.
lgtm overall, will wait for the tests to pass.
There was a problem hiding this comment.
I'd want to implement it in cri-tools and use it, but it'll take some time.
|
/retest |
1 similar comment
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: PannagaRao, sohankunkerkar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@bitoku I will leave the final lgtm to you. |
|
/lgtm |
|
/retest |
Added unified cgroupv2 settings to updateContainerResources
Signed-off-by: Pannaga Rao Bhoja Ramamanohara
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # #9816
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Summary by CodeRabbit
New Features
Tests
Chores