UPSTREAM: KEP-6063: Implement per-pod PID limit (Alpha)#2710
UPSTREAM: KEP-6063: Implement per-pod PID limit (Alpha)#2710BhargaviGudi wants to merge 1 commit into
Conversation
Adds support for per-pod PID limits via resources.limits.pid, gated behind the PerPodPIDLimit feature gate (Alpha, disabled by default). The kubelet enforces the effective limit as min(node podPidsLimit, pod pid limit) on cgroupsv2 nodes, falling back to the node-level limit on cgroupsv1 with a warning. - Add PerPodPIDLimit feature gate (Alpha, v1.37) - Register pid as a standard, integer, non-overcommittable resource - Validate pid values in the range 1024-16384 - Strip pid fields when the gate is disabled, preserve on existing pods - Add Prometheus metric kubelet_pod_pid_limit_applied_total - Add unit tests for validation, helpers, field stripping, kubelet logic - Add e2e node tests for per-pod PID limit enforcement Signed-off-by: Bhargavi Gudi <bgudi@redhat.com>
|
@BhargaviGudi: No Jira issue with key KEP-6063 exists in the tracker at https://redhat.atlassian.net. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
backports/unvalidated-commits
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@BhargaviGudi: the contents of this pull request could not be automatically validated. The following commits could not be validated and must be approved by a top-level approver:
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThis PR adds a new alpha ChangesPer-Pod PID Limit Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Kubelet as Kubelet PodContainerManager
participant PodSpec as Pod Spec
participant Metrics as Metrics Registry
participant Recorder as Event Recorder
participant Cgroup as Pod Cgroup
Kubelet->>PodSpec: getPodPIDLimit(pod)
PodSpec-->>Kubelet: requested pid limit (or 0)
Kubelet->>Kubelet: compute effectivePidLimit from node default + request
alt PerPodPIDLimit gate enabled and request set
Kubelet->>Kubelet: validate cgroups v2 support
alt request exceeds node limit
Kubelet->>Recorder: emit PIDLimitCapped warning event
Kubelet->>Metrics: increment PodPIDLimitApplied
end
end
Kubelet->>Cgroup: set ResourceParameters.PidsLimit = effectivePidLimit
Estimated code review effort: 4 (Complex) | ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)pkg/apis/core/validation/validation_test.goThanks 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 |
BhargaviGudi
changed the title
|
@BhargaviGudi: No Jira issue with key KEP-6063 exists in the tracker at https://redhat.atlassian.net. 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/apis/core/validation/validation.go (1)
7457-7475: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve existing
pidvalues whenPerPodPIDLimitis disabled.
ValidatePodUpdatere-runs pod-level resource validation on the full new spec, andvalidatePodResourceNamehas no old-object context. A pod created withlimits.pidwill fail any later update once the gate is turned off, even thoughdropDisabledPodPIDLimitFieldsexplicitly preserves that field on update. Thread the old pod through this validator or add a ratchet forpidhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apis/core/validation/validation.go` around lines 7457 - 7475, validatePodResourceName currently rejects core.ResourcePID whenever PerPodPIDLimit is disabled, which breaks updates for pods that already had limits.pid set. Update the validation path used by ValidatePodUpdate so it can see the old pod state, and either thread the old object into validatePodResourceName or add a ratchet there to allow existing pid values to pass when they were already present before the feature gate was turned off. Keep the fix anchored around validatePodResourceName and the pod update validation flow.
🧹 Nitpick comments (2)
pkg/kubelet/cm/pod_container_manager_linux_test.go (1)
442-509: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests reimplement production logic instead of exercising it — no regression protection for the actual code paths.
TestGetPodPIDLimitCgroupsV1ErrorMessagebuilds its ownfmt.Errorfwith the same literal string as the source instead of calling into the code that produces it, andTestPIDLimitCappedEvent/TestNoPIDLimitCappedEventWhenNotCappedreimplement thepodPid > effectivePidLimitcondition inline rather than calling intoEnsureExists's actual capping logic. As written, a regression in the real cgroupsv1-handling or capping logic inpod_container_manager_linux.gowould not be caught by these tests. Consider extracting the decision logic (as suggested on the source file) into a small function these tests can call directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kubelet/cm/pod_container_manager_linux_test.go` around lines 442 - 509, These tests are duplicating production behavior instead of verifying it, so they won’t catch regressions in the real PID-limit and capping paths. Move the cgroupsv1 error decision and PID-limit capping decision into a small helper used by EnsureExists in pod_container_manager_linux.go, then update TestGetPodPIDLimitCgroupsV1ErrorMessage and the PIDLimitCapped tests to call that helper (or the real path) rather than constructing the same fmt.Errorf or inline if-check themselves.pkg/kubelet/cm/pod_container_manager_linux.go (1)
98-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTestability concern: PID-limit computation is embedded in
EnsureExists, making it hard to unit test directly.The effective-PID-limit computation, capping, warning-event emission, and cgroupsv1 handling are all inlined in
EnsureExists, which also depends onlibcontainercgroups.IsCgroup2UnifiedMode()(a real system check) and cgroup creation. This makes it impossible to unit test the actual capping/error logic without relying on the host's cgroup mode — as evidenced by the new tests inpod_container_manager_linux_test.go, which reimplement the logic inline rather than calling into this code (see comment there). Extracting this into a small, injectable helper (e.g.computeEffectivePIDLimit(pod, nodePidsLimit int64, isCgroup2 func() bool, recorder, logger) (int64, error)) would let tests exercise the real decision logic directly.Also applies to: 388-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/kubelet/cm/pod_container_manager_linux.go` around lines 98 - 119, The PID-limit decision logic inside EnsureExists is too tightly coupled to cgroup checks and side effects to unit test cleanly. Extract the effective PID limit computation, cgroupsv1/v2 validation, capping, metrics, and warning-event emission into a small helper (for example around getPodPIDLimit and the podPidsLimit handling) that takes an injectable cgroups-mode check and recorder/logger dependencies. Keep EnsureExists focused on wiring the helper’s result into containerConfig.ResourceParameters.PidsLimit so tests can call the real logic directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/kubelet/cm/pod_container_manager_linux.go`:
- Around line 98-119: The cgroupsv1 branch in the pod PID limit handling is
hard-failing in the pod container manager instead of falling back as intended.
Update the logic in pod_container_manager_linux.go around the per-pod PID limit
check so that, when PerPodPIDLimit is enabled and getPodPIDLimit(pod) is set on
cgroupsv1, it logs a warning and continues using m.podPidsLimit rather than
returning an error from EnsureExists. Keep the existing cgroupsv2 min(node
limit, pod limit) behavior in the same Pod PID limit path and preserve the
metrics/event reporting symbols (getPodPIDLimit, m.podPidsLimit, EnsureExists,
containerConfig.ResourceParameters.PidsLimit).
In `@test/e2e_node/eviction_test.go`:
- Line 743: The PID-pressure eviction test is expecting the wrong starved
resource because SignalPIDAvailable maps to the internal pids resource, not
none. Update the expected value in verifyEvictionEvents for this PID eviction
case so it uses the PID starved resource constant instead of noStarvedResource,
and keep the change localized around the expectedStarvedResource assignment in
eviction_test.go.
In `@test/e2e_node/pids_test.go`:
- Around line 423-425: The PID verifier path in makePodToVerifyPids is using a
HostPath mount that will be rejected in PSA Baseline/Restricted namespaces, so
don’t create or wait for that verifier pod in those namespaces. Update the e2e
flow around the verifyPod creation and WaitForPodSuccessInNamespace calls to
either run the verifier in a privileged namespace or only execute it when the
test is validating admission success, and apply the same fix in the other
matching verify block.
- Around line 178-180: The per-pod PID enforcement checks in the pids E2E flow
are assuming cgroups v2 behavior, but cgroups v1 should fall back to the node
PodPidsLimit instead. Update the assertions in pidsTest (including the
makePodToVerifyPids/Create checks and the fork-failure verification paths) to
either skip on cgroups v1 or choose expected PID limits based on cgroup mode, so
the 2048/1024 expectations only run when cgroups v2 is enabled.
- Around line 467-470: The SecurityContext initializers use invalid new(value)
calls, which do not compile. Update the pids test fixture to use valid pointer
creation for the RunAsNonRoot, RunAsUser, and AllowPrivilegeEscalation fields in
the SecurityContext literal, keeping the changes localized around the existing
SecurityContext setup.
---
Outside diff comments:
In `@pkg/apis/core/validation/validation.go`:
- Around line 7457-7475: validatePodResourceName currently rejects
core.ResourcePID whenever PerPodPIDLimit is disabled, which breaks updates for
pods that already had limits.pid set. Update the validation path used by
ValidatePodUpdate so it can see the old pod state, and either thread the old
object into validatePodResourceName or add a ratchet there to allow existing pid
values to pass when they were already present before the feature gate was turned
off. Keep the fix anchored around validatePodResourceName and the pod update
validation flow.
---
Nitpick comments:
In `@pkg/kubelet/cm/pod_container_manager_linux_test.go`:
- Around line 442-509: These tests are duplicating production behavior instead
of verifying it, so they won’t catch regressions in the real PID-limit and
capping paths. Move the cgroupsv1 error decision and PID-limit capping decision
into a small helper used by EnsureExists in pod_container_manager_linux.go, then
update TestGetPodPIDLimitCgroupsV1ErrorMessage and the PIDLimitCapped tests to
call that helper (or the real path) rather than constructing the same fmt.Errorf
or inline if-check themselves.
In `@pkg/kubelet/cm/pod_container_manager_linux.go`:
- Around line 98-119: The PID-limit decision logic inside EnsureExists is too
tightly coupled to cgroup checks and side effects to unit test cleanly. Extract
the effective PID limit computation, cgroupsv1/v2 validation, capping, metrics,
and warning-event emission into a small helper (for example around
getPodPIDLimit and the podPidsLimit handling) that takes an injectable
cgroups-mode check and recorder/logger dependencies. Keep EnsureExists focused
on wiring the helper’s result into containerConfig.ResourceParameters.PidsLimit
so tests can call the real logic directly.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1d6772d8-82b9-44cd-8a17-a19a43ba32a8
📒 Files selected for processing (21)
pkg/api/pod/util.gopkg/api/pod/util_test.gopkg/apis/core/helper/helpers.gopkg/apis/core/helper/helpers_test.gopkg/apis/core/types.gopkg/apis/core/validation/validation.gopkg/apis/core/validation/validation_test.gopkg/features/kube_features.gopkg/kubelet/cm/container_manager_linux.gopkg/kubelet/cm/pod_container_manager_linux.gopkg/kubelet/cm/pod_container_manager_linux_test.gopkg/kubelet/eviction/eviction_manager_test.gopkg/kubelet/metrics/metrics.gostaging/src/k8s.io/api/core/v1/types.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/perpodpidlimit/feature.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/perpodpidlimit/feature_test.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/registry.gotest/compatibility_lifecycle/reference/versioned_feature_list.yamltest/e2e_node/eviction_test.gotest/e2e_node/pids_test.gotest/instrumentation/documentation/documentation-list.yaml
| effectivePidLimit := m.podPidsLimit | ||
| if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PerPodPIDLimit) { | ||
| if podPid := getPodPIDLimit(pod); podPid > 0 { | ||
| if !libcontainercgroups.IsCgroup2UnifiedMode() { | ||
| return fmt.Errorf("per-pod PID limit requires cgroupsv2, but this node is running cgroupsv1; pod %s specifies spec.resources.limits.pid", pod.Name) | ||
| } else { | ||
| if podPid < effectivePidLimit || effectivePidLimit <= 0 { | ||
| effectivePidLimit = podPid | ||
| } else if podPid > effectivePidLimit { | ||
| logger.Info("Pod PID limit capped by node podPidsLimit", "pod", klog.KObj(pod), "requested", podPid, "effective", effectivePidLimit) | ||
| if m.recorder != nil { | ||
| m.recorder.Eventf(pod, v1.EventTypeWarning, "PIDLimitCapped", | ||
| "Requested PID limit %d exceeds node podPidsLimit %d; effective limit capped to %d", | ||
| podPid, effectivePidLimit, effectivePidLimit) | ||
| } | ||
| } | ||
| metrics.PodPIDLimitApplied.Inc() | ||
| } | ||
| } | ||
| } | ||
| if effectivePidLimit > 0 { | ||
| containerConfig.ResourceParameters.PidsLimit = &effectivePidLimit |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cgroupsv1 path hard-errors instead of falling back, contradicting the PR's stated design.
Per the PR objectives: "on cgroupsv2 nodes, kubelet enforces the effective PID limit as min(node podPidsLimit, pod pid limit); on cgroupsv1, it falls back to the node-level limit and emits a warning." The implementation instead returns an error, which propagates out of EnsureExists and fails pod cgroup/sandbox creation entirely — the pod will never start on a cgroupsv1 node once PerPodPIDLimit is enabled and a pod specifies spec.resources.limits.pid, rather than gracefully degrading to the node-level limit as documented.
🛡️ Proposed fix: fall back to node-level limit with a warning instead of erroring
effectivePidLimit := m.podPidsLimit
if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PerPodPIDLimit) {
if podPid := getPodPIDLimit(pod); podPid > 0 {
if !libcontainercgroups.IsCgroup2UnifiedMode() {
- return fmt.Errorf("per-pod PID limit requires cgroupsv2, but this node is running cgroupsv1; pod %s specifies spec.resources.limits.pid", pod.Name)
+ logger.Info("Per-pod PID limit requires cgroupsv2; falling back to node-level podPidsLimit", "pod", klog.KObj(pod))
+ if m.recorder != nil {
+ m.recorder.Eventf(pod, v1.EventTypeWarning, "PIDLimitUnsupported",
+ "Per-pod PID limit requires cgroupsv2, but this node is running cgroupsv1; falling back to node podPidsLimit %d", effectivePidLimit)
+ }
} else {
if podPid < effectivePidLimit || effectivePidLimit <= 0 {
effectivePidLimit = podPidPlease confirm against the KEP-6063 design whether hard-failing or falling back is intended.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| effectivePidLimit := m.podPidsLimit | |
| if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PerPodPIDLimit) { | |
| if podPid := getPodPIDLimit(pod); podPid > 0 { | |
| if !libcontainercgroups.IsCgroup2UnifiedMode() { | |
| return fmt.Errorf("per-pod PID limit requires cgroupsv2, but this node is running cgroupsv1; pod %s specifies spec.resources.limits.pid", pod.Name) | |
| } else { | |
| if podPid < effectivePidLimit || effectivePidLimit <= 0 { | |
| effectivePidLimit = podPid | |
| } else if podPid > effectivePidLimit { | |
| logger.Info("Pod PID limit capped by node podPidsLimit", "pod", klog.KObj(pod), "requested", podPid, "effective", effectivePidLimit) | |
| if m.recorder != nil { | |
| m.recorder.Eventf(pod, v1.EventTypeWarning, "PIDLimitCapped", | |
| "Requested PID limit %d exceeds node podPidsLimit %d; effective limit capped to %d", | |
| podPid, effectivePidLimit, effectivePidLimit) | |
| } | |
| } | |
| metrics.PodPIDLimitApplied.Inc() | |
| } | |
| } | |
| } | |
| if effectivePidLimit > 0 { | |
| containerConfig.ResourceParameters.PidsLimit = &effectivePidLimit | |
| effectivePidLimit := m.podPidsLimit | |
| if utilfeature.DefaultFeatureGate.Enabled(kubefeatures.PerPodPIDLimit) { | |
| if podPid := getPodPIDLimit(pod); podPid > 0 { | |
| if !libcontainercgroups.IsCgroup2UnifiedMode() { | |
| logger.Info("Per-pod PID limit requires cgroupsv2; falling back to node-level podPidsLimit", "pod", klog.KObj(pod)) | |
| if m.recorder != nil { | |
| m.recorder.Eventf(pod, v1.EventTypeWarning, "PIDLimitUnsupported", | |
| "Per-pod PID limit requires cgroupsv2, but this node is running cgroupsv1; falling back to node podPidsLimit %d", effectivePidLimit) | |
| } | |
| } else { | |
| if podPid < effectivePidLimit || effectivePidLimit <= 0 { | |
| effectivePidLimit = podPid | |
| } else if podPid > effectivePidLimit { | |
| logger.Info("Pod PID limit capped by node podPidsLimit", "pod", klog.KObj(pod), "requested", podPid, "effective", effectivePidLimit) | |
| if m.recorder != nil { | |
| m.recorder.Eventf(pod, v1.EventTypeWarning, "PIDLimitCapped", | |
| "Requested PID limit %d exceeds node podPidsLimit %d; effective limit capped to %d", | |
| podPid, effectivePidLimit, effectivePidLimit) | |
| } | |
| } | |
| metrics.PodPIDLimitApplied.Inc() | |
| } | |
| } | |
| } | |
| if effectivePidLimit > 0 { | |
| containerConfig.ResourceParameters.PidsLimit = &effectivePidLimit |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/kubelet/cm/pod_container_manager_linux.go` around lines 98 - 119, The
cgroupsv1 branch in the pod PID limit handling is hard-failing in the pod
container manager instead of falling back as intended. Update the logic in
pod_container_manager_linux.go around the per-pod PID limit check so that, when
PerPodPIDLimit is enabled and getPodPIDLimit(pod) is set on cgroupsv1, it logs a
warning and continues using m.podPidsLimit rather than returning an error from
EnsureExists. Keep the existing cgroupsv2 min(node limit, pod limit) behavior in
the same Pod PID limit path and preserve the metrics/event reporting symbols
(getPodPIDLimit, m.podPidsLimit, EnsureExists,
containerConfig.ResourceParameters.PidsLimit).
| f.NamespacePodSecurityLevel = admissionapi.LevelPrivileged | ||
| const pressureTimeout = 10 * time.Minute | ||
| const expectedNodeCondition = v1.NodePIDPressure | ||
| const expectedStarvedResource = noStarvedResource |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expect the PID starved resource in eviction events.
SignalPIDAvailable maps to the internal "pids" resource, so passing none here makes verifyEvictionEvents expect no starved resource for a PID-pressure eviction.
Proposed fix
- const expectedStarvedResource = noStarvedResource
+ const expectedStarvedResource = v1.ResourceName("pids")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const expectedStarvedResource = noStarvedResource | |
| const expectedStarvedResource = v1.ResourceName("pids") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e_node/eviction_test.go` at line 743, The PID-pressure eviction test
is expecting the wrong starved resource because SignalPIDAvailable maps to the
internal pids resource, not none. Update the expected value in
verifyEvictionEvents for this PID eviction case so it uses the PID starved
resource constant instead of noStarvedResource, and keep the change localized
around the expectedStarvedResource assignment in eviction_test.go.
| ginkgo.By("checking if the per-pod PID limit was applied (min of node 4096, pod 2048 = 2048)") | ||
| verifyPod := makePodToVerifyPids("pod"+podUID, resource.MustParse("2048")) | ||
| e2epod.NewPodClient(f).Create(ctx, verifyPod) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate per-pod enforcement assertions to cgroups v2.
These checks expect 2048/1024 and fork failure unconditionally, but the PR contract says cgroups v1 falls back to the node PodPidsLimit (4096). Skip these enforcement-only checks on cgroups v1 or branch expected values by cgroup mode.
Example adjustment
ginkgo.It("should apply per-pod PID limit lower than node default", func(ctx context.Context) {
+ if !IsCgroup2UnifiedMode() {
+ ginkgo.Skip("per-pod PID limits are enforced only on cgroup v2")
+ }
ginkgo.By("creating a pod with pid limit below the node default")Also applies to: 258-260, 324-327, 354-359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e_node/pids_test.go` around lines 178 - 180, The per-pod PID
enforcement checks in the pids E2E flow are assuming cgroups v2 behavior, but
cgroups v1 should fall back to the node PodPidsLimit instead. Update the
assertions in pidsTest (including the makePodToVerifyPids/Create checks and the
fork-failure verification paths) to either skip on cgroups v1 or choose expected
PID limits based on cgroup mode, so the 2048/1024 expectations only run when
cgroups v2 is enabled.
| verifyPod := makePodToVerifyPids("pod"+podUID, resource.MustParse("2048")) | ||
| e2epod.NewPodClient(f).Create(ctx, verifyPod) | ||
| err := e2epod.WaitForPodSuccessInNamespace(ctx, f.ClientSet, verifyPod.Name, f.Namespace.Name) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t run the HostPath verifier inside PSA namespaces.
makePodToVerifyPids mounts /sys/fs/cgroup via HostPath, so the verifier pod will be rejected under Baseline/Restricted PSA even if the PID-limited workload pod is admitted. Move verification to a privileged namespace or limit these cases to admission success.
Also applies to: 484-486
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e_node/pids_test.go` around lines 423 - 425, The PID verifier path in
makePodToVerifyPids is using a HostPath mount that will be rejected in PSA
Baseline/Restricted namespaces, so don’t create or wait for that verifier pod in
those namespaces. Update the e2e flow around the verifyPod creation and
WaitForPodSuccessInNamespace calls to either run the verifier in a privileged
namespace or only execute it when the test is validating admission success, and
apply the same fix in the other matching verify block.
| SecurityContext: &v1.SecurityContext{ | ||
| RunAsNonRoot: new(true), | ||
| RunAsUser: new(int64(1000)), | ||
| AllowPrivilegeEscalation: new(false), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no invalid new(value) bool/int64 pointer initializers remain.
rg -nP 'new\((true|false|int64\()' --type=go .Repository: openshift/kubernetes
Length of output: 400
Replace invalid new(value) initializers. new takes a type, so new(true), new(int64(1000)), and new(false) won’t compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e_node/pids_test.go` around lines 467 - 470, The SecurityContext
initializers use invalid new(value) calls, which do not compile. Update the pids
test fixture to use valid pointer creation for the RunAsNonRoot, RunAsUser, and
AllowPrivilegeEscalation fields in the SecurityContext literal, keeping the
changes localized around the existing SecurityContext setup.
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Ports the KEP-6063 per-pod PID limit implementation to openshift/kubernetes as an UPSTREAM carry.
This adds support for per-pod PID limits via
spec.resources.limits.pid, gated behind thePerPodPIDLimitfeature gate (Alpha, disabled by default). The kubelet enforces the effective limit asmin(node podPidsLimit, pod pid limit)on cgroupsv2 nodes, falling back to the node-level limit on cgroupsv1 with a warning.Changes:
PerPodPIDLimitfeature gate (Alpha, v1.37) withPodLevelResourcesdependencypidas a standard, integer, non-overcommittable resourcerequests.pid(onlylimits.pidis valid)kubelet_pod_pid_limit_applied_totalperpodpidlimitnode declared feature for schedulingWhich issue(s) this PR is related to:
Upstream PR: kubernetes#139277
KEP: kubernetes/enhancements#6063
Special notes for your reviewer:
This is an UPSTREAM carry since the upstream PR (kubernetes#139277) has not merged yet. The
perpodpidlimitnode declared feature was adapted to use OCP's existingnodedeclaredfeatures.Featureinterface (upstream refactored this into a separatetypespackage which OCP hasn't picked up yet). Once the upstream PR merges, this carry should be replaced with the merged commit.Does this PR introduce a user-facing change?