Add min_injected_gomaxprocs via runtime handler precreate hook#9860
Conversation
|
Skipping CI for Draft Pull Request. |
|
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 configurable GOMAXPROCS injection via a new runtime hook that runs at container PreCreate; wires CLI/config/template/docs/completions, introduces a SkipGoMaxProcs annotation and workload-partition detection, composes hooks when multiple apply, and adds unit + e2e tests exercising injection and skip paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CRI_O
participant HooksRetriever
participant CompositeHooks
participant GomaxprocsHooks
participant SpecGen
participant Sandbox
Client->>CRI_O: CreateContainer(request)
CRI_O->>HooksRetriever: Get(runtimeHandler)
HooksRetriever->>CRI_O: returns nil / single hook / CompositeHooks
alt Composite returned
CRI_O->>CompositeHooks: PreCreate(specgen, sandbox)
CompositeHooks->>GomaxprocsHooks: PreCreate(specgen, sandbox)
GomaxprocsHooks->>Sandbox: read annotations & cgroup_parent
GomaxprocsHooks->>SpecGen: read CPU quota/shares & Env
alt eligible (no quota, burstable/besteffort, not skipped)
GomaxprocsHooks->>SpecGen: AddProcessEnv("GOMAXPROCS", value)
end
GomaxprocsHooks-->>CompositeHooks: return nil/error
CompositeHooks-->>CRI_O: PreCreate completed
end
CRI_O->>Client: Create response / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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 |
43163b3 to
711c702
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9860 +/- ##
==========================================
+ Coverage 64.73% 67.77% +3.03%
==========================================
Files 212 214 +2
Lines 29366 29504 +138
==========================================
+ Hits 19011 19996 +985
+ Misses 8656 7813 -843
+ Partials 1699 1695 -4 🚀 New features to boost your workflow:
|
711c702 to
0c76ad4
Compare
0c76ad4 to
64a51f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtimehandlerhooks/gomaxprocs_hooks_linux_test.go`:
- Around line 76-99: The test currently only checks env slice length to detect
injection which misses in-place replacements; update the test around
injectGOMAXPROCS(&g, tc.defaultEnv, tc.maxProcs) to explicitly search
g.Config.Process.Env for any "GOMAXPROCS=" entries and assert their final value
equals tc.expectVal when tc.expectSet is true, and assert no "GOMAXPROCS=" entry
exists when tc.expectSet is false; keep the existing env length checks if
desired but add these explicit presence/value assertions to cover cases where
AddProcessEnv replaces existing keys in place.
In `@pkg/config/template.go`:
- Around line 999-1008: Update the documentation string in
templateStringCrioRuntimeInjectGOMAXPROCS to list the additional skip
conditions: explicitly state that injection is skipped for workload-partitioned
pods and for containers with a positive CPU quota (in addition to Guaranteed
pods and containers that already set GOMAXPROCS), and clarify that
inject_gomaxprocs (bound to .InjectGOMAXPROCS) only applies when none of these
skip conditions are met; modify the template text to add one or two concise
sentences after the existing explanation enumerating these skip paths so users
understand when injection will not occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2258797-ddf2-4ada-9450-1633461ac970
📒 Files selected for processing (14)
completions/bash/criocompletions/fish/crio.fishcompletions/zsh/_criodocs/crio.8.mddocs/crio.conf.5.mdinternal/criocli/criocli.gointernal/runtimehandlerhooks/composite_hooks.gointernal/runtimehandlerhooks/gomaxprocs_hooks_linux.gointernal/runtimehandlerhooks/gomaxprocs_hooks_linux_test.gointernal/runtimehandlerhooks/runtime_handler_hooks_linux.gopkg/annotations/v2/annotations.gopkg/config/config.gopkg/config/template.gopkg/config/workloads.go
64a51f7 to
9f0b629
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/runtimehandlerhooks/runtime_handler_hooks_linux.go (1)
39-89:⚠️ Potential issue | 🟡 MinorThe concrete return type is now configuration-dependent.
When
inject_gomaxprocsis enabled alongside another hook,Get()returns*CompositeHooksinstead of the concrete hook type.internal/runtimehandlerhooks/high_performance_hooks_test.go, Line 1654 still asserts*HighPerformanceHooks, so those cases need to be updated to assert through the interface or unwrap the composite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtimehandlerhooks/runtime_handler_hooks_linux.go` around lines 39 - 89, Get() can return a CompositeHooks when InjectGOMAXPROCS is enabled along with another hook, so tests that assert a concrete *HighPerformanceHooks (e.g. in internal/runtimehandlerhooks/high_performance_hooks_test.go) must be updated; modify the test to either assert via the RuntimeHandlerHooks interface or detect/unpack a CompositeHooks and locate the *HighPerformanceHooks instance inside its hooks slice (use a type switch or iterate CompositeHooks.hooks to find *HighPerformanceHooks) so the test passes regardless of whether Get() returns a single concrete hook or a CompositeHooks wrapper.
♻️ Duplicate comments (1)
pkg/config/template.go (1)
999-1008:⚠️ Potential issue | 🟡 MinorDocument the remaining skip paths.
This text still reads as if every burstable/best-effort container is eligible. The hook also skips workload-partitioned pods and containers with a positive CPU quota, so users will otherwise read
inject_gomaxprocsas broken when it intentionally no-ops.Suggested wording
const templateStringCrioRuntimeInjectGOMAXPROCS = `# Enables GOMAXPROCS injection for burstable and best-effort pod containers. # This value acts as a minimum floor. For burstable pods with a CPU request, # GOMAXPROCS is auto-calculated from the request; the calculated value is only # used if it exceeds this floor. For best-effort pods (no CPU request), this -# value is used directly. Guaranteed pods are skipped. The value is only +# value is used directly. Guaranteed and workload-partitioned pods are skipped. +# Containers with a positive CPU quota are also skipped so Go can honor the +# cgroup CPU limit directly. The value is only # injected if the container does not already have GOMAXPROCS set via the image # or pod spec. Set to 0 to disable (default).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/template.go` around lines 999 - 1008, Update the documentation string in templateStringCrioRuntimeInjectGOMAXPROCS to list all the additional skip conditions the injector respects: explicitly mention that workload-partitioned pods are skipped and that containers with a positive CPU quota are not modified, so users understand inject_gomaxprocs may intentionally no-op; locate and edit the constant templateStringCrioRuntimeInjectGOMAXPROCS to add those skip paths into the explanatory text near the top of the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/runtimehandlerhooks/runtime_handler_hooks_linux.go`:
- Around line 39-89: Get() can return a CompositeHooks when InjectGOMAXPROCS is
enabled along with another hook, so tests that assert a concrete
*HighPerformanceHooks (e.g. in
internal/runtimehandlerhooks/high_performance_hooks_test.go) must be updated;
modify the test to either assert via the RuntimeHandlerHooks interface or
detect/unpack a CompositeHooks and locate the *HighPerformanceHooks instance
inside its hooks slice (use a type switch or iterate CompositeHooks.hooks to
find *HighPerformanceHooks) so the test passes regardless of whether Get()
returns a single concrete hook or a CompositeHooks wrapper.
---
Duplicate comments:
In `@pkg/config/template.go`:
- Around line 999-1008: Update the documentation string in
templateStringCrioRuntimeInjectGOMAXPROCS to list all the additional skip
conditions the injector respects: explicitly mention that workload-partitioned
pods are skipped and that containers with a positive CPU quota are not modified,
so users understand inject_gomaxprocs may intentionally no-op; locate and edit
the constant templateStringCrioRuntimeInjectGOMAXPROCS to add those skip paths
into the explanatory text near the top of the template.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 820148ae-003d-4ea4-904a-6a5093453baf
📒 Files selected for processing (14)
completions/bash/criocompletions/fish/crio.fishcompletions/zsh/_criodocs/crio.8.mddocs/crio.conf.5.mdinternal/criocli/criocli.gointernal/runtimehandlerhooks/composite_hooks.gointernal/runtimehandlerhooks/gomaxprocs_hooks_linux.gointernal/runtimehandlerhooks/gomaxprocs_hooks_linux_test.gointernal/runtimehandlerhooks/runtime_handler_hooks_linux.gopkg/annotations/v2/annotations.gopkg/config/config.gopkg/config/template.gopkg/config/workloads.go
✅ Files skipped from review due to trivial changes (2)
- completions/bash/crio
- completions/fish/crio.fish
🚧 Files skipped from review as they are similar to previous changes (7)
- completions/zsh/_crio
- pkg/annotations/v2/annotations.go
- docs/crio.8.md
- docs/crio.conf.5.md
- internal/runtimehandlerhooks/composite_hooks.go
- internal/runtimehandlerhooks/gomaxprocs_hooks_linux_test.go
- internal/runtimehandlerhooks/gomaxprocs_hooks_linux.go
| PodLinuxResources, | ||
| SeccompNotifierAction, | ||
| SeccompProfile, | ||
| SkipGoMaxProcs, |
There was a problem hiding this comment.
this shouldn't be an allowed annotation, remove this line
There was a problem hiding this comment.
@haircommander If remove it then pods won't be able skip setting GOMAXPROCS by setting annotation? We want pods to be able to skip.
There was a problem hiding this comment.
that's not true, inclusion in this list means that the annotation will be scrubbed from the container if it's not in a runtime handler that has the allowed annotation. pods are allowed to have annotations that aren't in this list and cri-o can take any action on any annotation it wants. the reason to include here is if we feel there's a risk to the annotation and we want an admin to explicitly opt into its enablement
There was a problem hiding this comment.
Ok, thanks, looking into it.
There was a problem hiding this comment.
Just to be sure, I will test this again on AWS.
| // We reverse that with ceil(shares / 1024) to get the CPU count. | ||
| // The floor is used when the calculated value is lower. | ||
| func calculateGOMAXPROCS(shares, fallbackMaxProcs int64) int64 { | ||
| if shares > 2 { |
There was a problem hiding this comment.
why are we checking this? seems like we could just return
max( max((shares + 1023)/1024, 1), fallbackMaxProcs)
|
@haircommander: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, harche 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 |
|
/hold |
2403451 to
82fb64f
Compare
Implement GOMAXPROCS injection as a composable PreCreate runtime handler hook instead of inline logic in container_create.go. This follows the existing CRI-O hook architecture (like HighPerformanceHooks) and allows the GOMAXPROCS hook to compose with other hooks via CompositeHooks. Key changes: - GomaxprocsHooks: PreCreate hook that reads CPU shares from the OCI spec, calculates GOMAXPROCS with a configurable floor, and injects the env var for burstable/best-effort pods - CompositeHooks: chains multiple RuntimeHandlerHooks implementations, enabling GOMAXPROCS to work alongside high-performance hooks - HooksRetriever.Get(): builds a hook chain when inject_gomaxprocs > 0 - Config field, CLI flag, annotation, docs, completions, and tests The hook fires for all OCI runtimes and skips guaranteed pods, workload-partitioned pods, pods with CPU limits (Go 1.25+ auto-detects), and pods that already have GOMAXPROCS set. Signed-off-by: Peter Hunt <pehunt@redhat.com> Co-authored-by: Harshal Patil <12152047+harche@users.noreply.github.com>
82fb64f to
1b111f6
Compare
|
/lgtm |
|
/retest |
|
/test ci-cgroupv2-e2e |
|
/test e2e-aws-ovn |
|
/retest |
|
/override ci/prow/e2e-aws-ovn |
|
@haircommander: Overrode contexts on behalf of haircommander: ci/prow/ci-cgroupv2-e2e, ci/prow/e2e-aws-ovn DetailsIn response to this:
Instructions 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. |
|
@haircommander: new pull request created: #9876 DetailsIn response to this:
Instructions 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. |
Summary
Implements GOMAXPROCS injection as a composable
PreCreateruntime handler hook.GomaxprocsHooks.PreCreate()reads CPU shares from the OCI spec, calculates GOMAXPROCS with a configurable floor (min_injected_gomaxprocs), and injects the env var for burstable/best-effort podsCompositeHookschains multipleRuntimeHandlerHooks(e.g. GOMAXPROCS + high-performance hooks)skip-gomaxprocs.crio.iopod annotation to opt outThis is an alternative to #9844 based on reviewer feedback to use the precreate hook mechanism.