UPSTREAM: 140013: KEP-5855: Add bind mount options to VolumeMount - #2744
UPSTREAM: 140013: KEP-5855: Add bind mount options to VolumeMount#2744amritansh1502 wants to merge 10 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@amritansh1502: No Jira issue with key KEP-5855 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
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@amritansh1502: 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 |
WalkthroughThe change adds alpha-gated Linux bind-mount options to ChangesVolume bind-mount options
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PodSpec
participant Kubelet
participant ContainerRuntime
participant NodeFeatureDiscovery
PodSpec->>Kubelet: BindMountOptions
Kubelet->>ContainerRuntime: Mount.mount_options
ContainerRuntime-->>Kubelet: RuntimeFeatures.mount_options
Kubelet->>NodeFeatureDiscovery: RuntimeFeatures.MountOptions
NodeFeatureDiscovery-->>Kubelet: VolumeBindMountOptions availability
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amritansh1502 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 |
|
@amritansh1502: No Jira issue with key KEP-5855 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/kubelet/kubelet_pods.go (1)
403-413: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGate
BindMountOptionsin the kubelet mount path.
makeMountscopiesmount.BindMountOptionswithout checking theVolumeBindMountOptionsfeature gate. The same function guards the analogous alpha field: lines 399-401 return an error whenRecursiveReadOnlyMountsis disabled but the field is set. Two paths reachmakeMountswithout apiserver validation:
- Static pods, which the kubelet admits directly.
- Pods created while the apiserver gate was enabled and later served to a kubelet with the gate disabled.
In both cases the kubelet forwards the options to CRI even though the feature is off on this node.
A second gap: the options are also forwarded when the runtime does not advertise the
mount_optionscapability. The runtime then ignores unknown fields, so a pod that asks fornoexecornosuidstarts without them. Silently dropping a hardening option is worse than failing the mount. Consider plumbing the runtime capability intomakeMountsthe same waysupportsRROis plumbed, and returning an error when options are requested but unsupported.🛡️ Proposed gate check
if rro && !utilfeature.DefaultFeatureGate.Enabled(features.RecursiveReadOnlyMounts) { return nil, cleanupAction, fmt.Errorf("recursive read-only mount needs feature gate %q to be enabled", features.RecursiveReadOnlyMounts) } + + bindMountOptions := mount.BindMountOptions + if len(bindMountOptions) > 0 && !utilfeature.DefaultFeatureGate.Enabled(features.VolumeBindMountOptions) { + return nil, cleanupAction, fmt.Errorf("bind mount options need feature gate %q to be enabled", features.VolumeBindMountOptions) + } mounts = append(mounts, kubecontainer.Mount{ Name: mount.Name, @@ Propagation: propagation, - BindMountOptions: mount.BindMountOptions, + BindMountOptions: bindMountOptions, })🤖 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/kubelet_pods.go` around lines 403 - 413, Update makeMounts to gate BindMountOptions on the VolumeBindMountOptions feature gate, rejecting requested options when the gate is disabled, including for static pods and previously validated pods. Plumb the runtime’s mount_options capability into makeMounts alongside supportsRRO, and return an error when bind-mount options are requested but unsupported instead of forwarding them or silently dropping them.
🧹 Nitpick comments (5)
staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go (2)
55-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the duplicated scan over container lists.
The three loops differ only in the list they read. A small helper keeps the intent in one place.
♻️ Proposed refactor
+func hasBindMountOptions(mounts []v1.VolumeMount) bool { + for i := range mounts { + if len(mounts[i].BindMountOptions) > 0 { + return true + } + } + return false +} + func (f *volumeBindMountOptionsFeature) InferForScheduling(podInfo *types.PodInfo) bool { - for i := range podInfo.Spec.Containers { - for j := range podInfo.Spec.Containers[i].VolumeMounts { - if len(podInfo.Spec.Containers[i].VolumeMounts[j].BindMountOptions) > 0 { - return true - } - } - } - for i := range podInfo.Spec.InitContainers { - for j := range podInfo.Spec.InitContainers[i].VolumeMounts { - if len(podInfo.Spec.InitContainers[i].VolumeMounts[j].BindMountOptions) > 0 { - return true - } - } - } - for i := range podInfo.Spec.EphemeralContainers { - for j := range podInfo.Spec.EphemeralContainers[i].VolumeMounts { - if len(podInfo.Spec.EphemeralContainers[i].VolumeMounts[j].BindMountOptions) > 0 { - return true - } - } - } - return false + for i := range podInfo.Spec.Containers { + if hasBindMountOptions(podInfo.Spec.Containers[i].VolumeMounts) { + return true + } + } + for i := range podInfo.Spec.InitContainers { + if hasBindMountOptions(podInfo.Spec.InitContainers[i].VolumeMounts) { + return true + } + } + for i := range podInfo.Spec.EphemeralContainers { + if hasBindMountOptions(podInfo.Spec.EphemeralContainers[i].VolumeMounts) { + return true + } + } + return false }The helper needs the
k8s.io/api/core/v1import.🤖 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 `@staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go` around lines 55 - 78, Refactor volumeBindMountOptionsFeature.InferForScheduling to use a small helper accepting a core/v1 container slice and scanning VolumeMounts for non-empty BindMountOptions. Invoke the helper for Containers, InitContainers, and EphemeralContainers, returning true if any scan succeeds and false otherwise; add the required core/v1 import.
48-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for
volumebindmountoptions.Cover
Discoverwith disabled gates and unsupported runtime features. CoverInferForSchedulingwith init and ephemeral containers.🤖 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 `@staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go` around lines 48 - 53, Add unit tests for the volumebindmountoptions feature, covering volumeBindMountOptionsFeature.Discover when the feature gate is disabled and when runtime MountOptions support is unavailable, and covering InferForScheduling for both init-container and ephemeral-container scenarios. Use the existing feature-test patterns and assert the expected discovery and scheduling results.pkg/kubelet/container/runtime_test.go (1)
690-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where
MountOptionsis true.Both cases keep
MountOptionsat the zero value. Thetruerendering of the new flag stays untested.♻️ Proposed additional case
{ name: "features with both flags true", features: &RuntimeFeatures{ SupplementalGroupsPolicy: true, UserNamespacesHostNetwork: true, }, expected: "SupplementalGroupsPolicy: true UserNamespacesHostNetwork: true MountOptions: false", }, + { + name: "features with mount options true", + features: &RuntimeFeatures{ + SupplementalGroupsPolicy: true, + UserNamespacesHostNetwork: true, + MountOptions: true, + }, + expected: "SupplementalGroupsPolicy: true UserNamespacesHostNetwork: true MountOptions: true", + }, {🤖 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/container/runtime_test.go` around lines 690 - 705, Add a test case to the RuntimeFeatures string-formatting table with MountOptions set to true, and update the expected output to include “MountOptions: true” while preserving coverage of the existing feature flags.pkg/kubelet/kubelet_pods_test.go (1)
9485-9512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the mount identity, not only the index.
makeMountsappends the/etc/hostsmount after the volume mounts, so the index assumptions hold today. They break silently if mount ordering changes. Assert onNameorContainerPathto keep the failure message specific.♻️ Proposed refactor
mounts, _, err := makeMounts(logger, &pod, podDir, &container, "fakepod", "", []string{""}, podVolumes, fhu, &trackingSubpath{}, nil, false, nil) require.NoError(t, err) require.Len(t, mounts, 3) // 2 volume mounts + /etc/hosts + require.Equal(t, "/mnt/a", mounts[0].ContainerPath) + require.Equal(t, "/mnt/b", mounts[1].ContainerPath) assert.Equal(t, []string{"noexec", "nosuid"}, mounts[0].BindMountOptions) assert.Empty(t, mounts[1].BindMountOptions)🤖 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/kubelet_pods_test.go` around lines 9485 - 9512, Update TestMakeMountsBindMountOptions to locate mounts by their identifying Name or ContainerPath values instead of relying on mounts[0] and mounts[1]. Assert the expected BindMountOptions for the /mnt/a and /mnt/b volume mounts, while preserving the existing mount count assertion.pkg/apis/core/validation/validation_test.go (1)
7611-7621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error type and field path, not only the error count.
The current check only verifies that some error exists. A test case can pass for an unrelated reason. For example, the image-volume case would still pass if the error came from another rule. Compare the expected field path and error type per case.
♻️ Suggested stronger assertions
tests := []struct { name string mount core.VolumeMount - expectError bool + expectErrs field.ErrorList }{Then compare with the expected list, for example:
errs := ValidateVolumeMounts([]core.VolumeMount{test.mount}, volDevices, volumes, nil, field.NewPath("field"), PodValidationOptions{}) if diff := cmp.Diff(test.expectErrs, errs, cmpopts.IgnoreFields(field.Error{}, "Detail", "Origin")); diff != "" { t.Errorf("unexpected errors (-want +got):\n%s", diff) }🤖 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_test.go` around lines 7611 - 7621, Strengthen the table-driven assertions around ValidateVolumeMounts so each case specifies its expected field.Error values, including error type and field path, rather than only an expectError boolean. Compare test.expectErrs with errs using cmp.Diff while ignoring Detail and Origin, and update the test cases to provide the expected errors.
🤖 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/api/pod/util_test.go`:
- Around line 3470-3479: Deep-copy the shared volume fixture for each subtest in
the test loop before assigning it to the pod specs, ensuring newPod.Volumes and
wantPod.Volumes are independent. Update the fixture setup around
dropDisabledAtomicWriteVolumeUserFields so mutation in one case cannot affect
subsequent cases, especially the feature-gate-enabled scenarios.
In `@pkg/features/kube_features.go`:
- Line 2741: Update the VolumeBindMountOptions dependency declaration to include
NodeDeclaredFeatures, changing the empty dependency set to the required
NodeDeclaredFeatures entry while preserving the existing feature registration.
In `@pkg/kubelet/kubelet_pods_test.go`:
- Around line 5748-5762: Replace the invalid new(resource.MustParse(...))
expressions in NodeAllocatableResourceClaimStatuses with the existing ptr
helper, passing each parsed resource.Quantity value to ptr.To. Preserve the CPU
and memory quantities and use the same approach already established elsewhere in
kubelet_pods_test.go.
- Around line 9276-9386: Fix the invalid resource quantity pointer construction
by replacing new(resource.MustParse(...)) usages with
ptr.To(resource.MustParse(...)). Update makeMounts to accumulate every
successful subpath cleanup callback instead of overwriting cleanupAction, and
return a non-nil aggregate cleanup callback even when mount processing errors;
invoke all callbacks created before the error.
In `@staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml`:
- Around line 445-447: Replace every bindMountOptionsValue placeholder in
staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml lines 445-447, 676-678,
and 908-910; staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json
lines 702-705, 1019-1022, and 1336-1339;
staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml lines 481-483,
712-714, and 944-946; and staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json
lines 560-563, 877-880, and 1194-1197 with an allowed bind-mount option: noexec,
nodev, or nosuid.
In
`@staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go`:
- Around line 80-82: Update volumeBindMountOptionsFeature.InferForUpdate to
return true when newPodInfo requires VolumeBindMountOptions but oldPodInfo does
not, including additions of BindMountOptions through ephemeral container
updates. Ensure the related admission handling includes the ephemeralcontainers
subresource if needed to enforce the node MountOptions requirement.
In `@staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto`:
- Around line 359-362: Update the container mount creation path around
makeMounts to check RuntimeFeatures.MountOptions before propagating
BindMountOptions into runtimeapi.Mount.MountOptions. Reject the pod or container
request when bind mount options are specified but runtime support is
unavailable, before container creation; preserve existing behavior for supported
runtimes and mounts without options.
---
Outside diff comments:
In `@pkg/kubelet/kubelet_pods.go`:
- Around line 403-413: Update makeMounts to gate BindMountOptions on the
VolumeBindMountOptions feature gate, rejecting requested options when the gate
is disabled, including for static pods and previously validated pods. Plumb the
runtime’s mount_options capability into makeMounts alongside supportsRRO, and
return an error when bind-mount options are requested but unsupported instead of
forwarding them or silently dropping them.
---
Nitpick comments:
In `@pkg/apis/core/validation/validation_test.go`:
- Around line 7611-7621: Strengthen the table-driven assertions around
ValidateVolumeMounts so each case specifies its expected field.Error values,
including error type and field path, rather than only an expectError boolean.
Compare test.expectErrs with errs using cmp.Diff while ignoring Detail and
Origin, and update the test cases to provide the expected errors.
In `@pkg/kubelet/container/runtime_test.go`:
- Around line 690-705: Add a test case to the RuntimeFeatures string-formatting
table with MountOptions set to true, and update the expected output to include
“MountOptions: true” while preserving coverage of the existing feature flags.
In `@pkg/kubelet/kubelet_pods_test.go`:
- Around line 9485-9512: Update TestMakeMountsBindMountOptions to locate mounts
by their identifying Name or ContainerPath values instead of relying on
mounts[0] and mounts[1]. Assert the expected BindMountOptions for the /mnt/a and
/mnt/b volume mounts, while preserving the existing mount count assertion.
In
`@staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go`:
- Around line 55-78: Refactor volumeBindMountOptionsFeature.InferForScheduling
to use a small helper accepting a core/v1 container slice and scanning
VolumeMounts for non-empty BindMountOptions. Invoke the helper for Containers,
InitContainers, and EphemeralContainers, returning true if any scan succeeds and
false otherwise; add the required core/v1 import.
- Around line 48-53: Add unit tests for the volumebindmountoptions feature,
covering volumeBindMountOptionsFeature.Discover when the feature gate is
disabled and when runtime MountOptions support is unavailable, and covering
InferForScheduling for both init-container and ephemeral-container scenarios.
Use the existing feature-test patterns and assert the expected discovery and
scheduling results.
🪄 Autofix
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: Pro Plus
Run ID: a5494c3c-54d7-42a9-85dc-1df22d13a0de
⛔ Files ignored due to path filters (5)
pkg/apis/core/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**,!**/zz_generated*staging/src/k8s.io/api/core/v1/generated.pb.gois excluded by!**/*.pb.gostaging/src/k8s.io/api/core/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (86)
api/openapi-spec/swagger.jsonapi/openapi-spec/v3/api__v1_openapi.jsonapi/openapi-spec/v3/apis__apps__v1_openapi.jsonapi/openapi-spec/v3/apis__batch__v1_openapi.jsonpkg/api/pod/util.gopkg/api/pod/util_test.gopkg/apis/core/types.gopkg/apis/core/validation/validation.gopkg/apis/core/validation/validation_test.gopkg/features/kube_features.gopkg/kubelet/container/runtime.gopkg/kubelet/container/runtime_test.gopkg/kubelet/kubelet_node_declared_features.gopkg/kubelet/kubelet_pods.gopkg/kubelet/kubelet_pods_test.gopkg/kubelet/kuberuntime/helpers.gopkg/kubelet/kuberuntime/kuberuntime_container.gopkg/kubelet/kuberuntime/kuberuntime_container_test.gostaging/src/k8s.io/api/core/v1/generated.protostaging/src/k8s.io/api/core/v1/types.gostaging/src/k8s.io/api/core/v1/types_swagger_doc_generated.gostaging/src/k8s.io/api/testdata/HEAD/apps.v1.DaemonSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1.DaemonSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1.DaemonSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1.Deployment.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1.Deployment.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1.Deployment.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1.ReplicaSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1.ReplicaSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1.ReplicaSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1.StatefulSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1.StatefulSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1.StatefulSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.Deployment.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.Deployment.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.Deployment.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.StatefulSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.StatefulSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta1.StatefulSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.DaemonSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.DaemonSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.DaemonSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.Deployment.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.Deployment.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.Deployment.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.ReplicaSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.ReplicaSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.ReplicaSet.yamlstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.StatefulSet.jsonstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.StatefulSet.pbstaging/src/k8s.io/api/testdata/HEAD/apps.v1beta2.StatefulSet.yamlstaging/src/k8s.io/api/testdata/HEAD/batch.v1.CronJob.jsonstaging/src/k8s.io/api/testdata/HEAD/batch.v1.CronJob.pbstaging/src/k8s.io/api/testdata/HEAD/batch.v1.CronJob.yamlstaging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.jsonstaging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.pbstaging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yamlstaging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.jsonstaging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.pbstaging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yamlstaging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.jsonstaging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.pbstaging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.yamlstaging/src/k8s.io/api/testdata/HEAD/core.v1.PodTemplate.jsonstaging/src/k8s.io/api/testdata/HEAD/core.v1.PodTemplate.pbstaging/src/k8s.io/api/testdata/HEAD/core.v1.PodTemplate.yamlstaging/src/k8s.io/api/testdata/HEAD/core.v1.ReplicationController.jsonstaging/src/k8s.io/api/testdata/HEAD/core.v1.ReplicationController.pbstaging/src/k8s.io/api/testdata/HEAD/core.v1.ReplicationController.yamlstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.DaemonSet.jsonstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.DaemonSet.pbstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.DaemonSet.yamlstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.Deployment.jsonstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.Deployment.pbstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.Deployment.yamlstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.ReplicaSet.jsonstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.ReplicaSet.pbstaging/src/k8s.io/api/testdata/HEAD/extensions.v1beta1.ReplicaSet.yamlstaging/src/k8s.io/client-go/applyconfigurations/core/v1/volumemount.gostaging/src/k8s.io/client-go/applyconfigurations/internal/internal.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/registry.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.gostaging/src/k8s.io/component-helpers/nodedeclaredfeatures/types/types.gostaging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.prototest/compatibility_lifecycle/reference/feature_list.mdtest/compatibility_lifecycle/reference/versioned_feature_list.yaml
| for _, tc := range testCases { | ||
| t.Run(tc.description, func(t *testing.T) { | ||
| featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AtomicWriteVolumeUserFields, tc.atomicWriteVolumeUserFieldsEnabled) | ||
|
|
||
| dropDisabledAtomicWriteVolumeUserFields(tc.newPod, tc.oldPod) | ||
| if diff := cmp.Diff(tc.newPod, tc.wantPod); diff != "" { | ||
| t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deep-copy the shared volume fixture per test case.
volumesWithUserFields is a single shared slice. The first case passes it as newPod.Volumes, and dropDisabledAtomicWriteVolumeUserFields clears the User and DefaultUser fields in place. The later "feature gate enabled" cases then use the already-stripped slice for both newPod and wantPod, so those comparisons succeed without testing anything.
Copy the pod specs inside the subtest.
💚 Proposed fix
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AtomicWriteVolumeUserFields, tc.atomicWriteVolumeUserFieldsEnabled)
- dropDisabledAtomicWriteVolumeUserFields(tc.newPod, tc.oldPod)
- if diff := cmp.Diff(tc.newPod, tc.wantPod); diff != "" {
+ newPod := tc.newPod.DeepCopy()
+ wantPod := tc.wantPod.DeepCopy()
+ dropDisabledAtomicWriteVolumeUserFields(newPod, tc.oldPod.DeepCopy())
+ if diff := cmp.Diff(newPod, wantPod); diff != "" {
t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff)
}
})
}📝 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.
| for _, tc := range testCases { | |
| t.Run(tc.description, func(t *testing.T) { | |
| featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AtomicWriteVolumeUserFields, tc.atomicWriteVolumeUserFieldsEnabled) | |
| dropDisabledAtomicWriteVolumeUserFields(tc.newPod, tc.oldPod) | |
| if diff := cmp.Diff(tc.newPod, tc.wantPod); diff != "" { | |
| t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff) | |
| } | |
| }) | |
| } | |
| for _, tc := range testCases { | |
| t.Run(tc.description, func(t *testing.T) { | |
| featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.AtomicWriteVolumeUserFields, tc.atomicWriteVolumeUserFieldsEnabled) | |
| newPod := tc.newPod.DeepCopy() | |
| wantPod := tc.wantPod.DeepCopy() | |
| dropDisabledAtomicWriteVolumeUserFields(newPod, tc.oldPod.DeepCopy()) | |
| if diff := cmp.Diff(newPod, wantPod); diff != "" { | |
| t.Fatalf("Unexpected modification to new pod; diff (-got +want)\n%s", diff) | |
| } | |
| }) | |
| } |
🤖 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/api/pod/util_test.go` around lines 3470 - 3479, Deep-copy the shared
volume fixture for each subtest in the test loop before assigning it to the pod
specs, ensuring newPod.Volumes and wantPod.Volumes are independent. Update the
fixture setup around dropDisabledAtomicWriteVolumeUserFields so mutation in one
case cannot affect subsequent cases, especially the feature-gate-enabled
scenarios.
|
|
||
| VolumeAttributesClass: {}, | ||
|
|
||
| VolumeBindMountOptions: {}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the node declared feature registration for bind mount options.
fd -t f 'feature.go' -p 'nodedeclaredfeatures' --exec cat -n
# Show the registry entries.
fd -t f 'registry.go' -p 'nodedeclaredfeatures' --exec cat -n
# Show how the kubelet gates the node declared feature.
rg -n -C 5 'VolumeBindMountOptions' --type=goRepository: openshift/kubernetes
Length of output: 374
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Candidate node-declared-feature files:'
git ls-files | rg 'nodedeclaredfeatures|kube_features\.go$' | head -200
printf '%s\n' 'Bind-mount feature references:'
rg -n -C 8 'VolumeBindMountOptions|NodeDeclaredFeatures|ExtendWebSocketsToKubelet|InPlacePodVerticalScalingInitContainers|UserNamespacesHostNetworkSupport' --glob '*.go' . | head -400Repository: openshift/kubernetes
Length of output: 42972
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Bind-mount feature registration:'
cat -n staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go
printf '%s\n' 'Node-declared feature registry:'
cat -n staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/registry.go
printf '%s\n' 'Feature-set and dependency definitions:'
cat -n staging/src/k8s.io/component-helpers/nodedeclaredfeatures/featureset.go
rg -n -C 12 'NodeDeclaredFeatures|VolumeBindMountOptions|ExtendWebSocketsToKubelet|InPlacePodVerticalScalingInitContainers|UserNamespacesHostNetworkSupport' pkg/features/kube_features.go staging/src/k8s.io/component-helpers/nodedeclaredfeatures --glob '*.go'Repository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Consumers of the node-declared feature registry:'
rg -n -C 10 'AllFeatures|FeatureRequirements|Requirements\(\)|InferForScheduling|Discover\(' \
staging/src/k8s.io/component-helpers/nodedeclaredfeatures \
pkg/scheduler plugin pkg/kubelet --glob '*.go' | head -500
printf '%s\n' 'Feature-gate dependency validation and resolution:'
rg -n -C 12 'FeatureGateDependencies|featureDependencies|Dependencies|NodeDeclaredFeatures' \
pkg staging/src/k8s.io --glob '*.go' | rg -v 'validation_test.go|kube_features.go:.*feature declaration' | head -500Repository: openshift/kubernetes
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'GetFeatureRequirements call sites:'
rg -n -C 15 'GetFeatureRequirements\(' --glob '*.go' .
printf '%s\n' 'Feature-gate dependency map type and validation:'
rg -n -C 20 'FeatureDependencies|featureDependencies|DependencyMap|Dependencies map|FeatureGate.*depend|dependency.*feature' \
pkg staging --glob '*.go' | head -500
printf '%s\n' 'Node-declared feature gate checks around framework initialization:'
rg -n -C 20 'NodeDeclaredFeatures' \
pkg/kubelet staging/src/k8s.io/component-helpers/nodedeclaredfeatures \
--glob '*.go' | rg -v 'kube_features.go|_test.go' | head -500Repository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' 'Scheduling and admission paths for declared feature requirements:'
rg -n -C 18 'InferForPodScheduling|NewDeclaredFeaturesAdmitHandler|nodeDeclaredFeaturesFramework|MatchNodeFeatureSet' \
pkg/scheduler pkg/kubelet plugin staging/src/k8s.io --glob '*.go' | head -600
printf '%s\n' 'VolumeBindMountOptions runtime and kubelet paths:'
rg -n -C 15 'VolumeBindMountOptions|BindMountOptions|MountOptions' \
pkg staging/src/k8s.io --glob '*.go' | head -600
printf '%s\n' 'Relevant feature-gate dependency entries:'
sed -n '2480,2495p;2510,2528p;2580,2588p;2730,2745p' pkg/features/kube_features.goRepository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
features = Path("pkg/features/kube_features.go").read_text()
volume = Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go").read_text()
kubelet = Path("pkg/kubelet/kubelet.go").read_text()
scheduler = Path("pkg/scheduler/framework/plugins/nodedeclaredfeatures/nodedeclaredfeatures.go").read_text()
dep = re.search(
r"VolumeBindMountOptions:\s*\{([^}]*)\}",
features,
)
req = re.search(
r"func \(f \*volumeBindMountOptionsFeature\) Requirements\(\).*?"
r"EnabledFeatureGates:\s*\[\]string\{([^}]*)\}",
volume,
re.S,
)
print("VolumeBindMountOptions dependency entry:",
dep.group(1).strip() if dep else "<missing>")
print("VolumeBindMountOptions registered gate requirements:",
req.group(1).strip() if req else "<missing>")
print("VolumeBindMountOptions infers scheduling requirements:",
"BindMountOptions" in volume and "InferForScheduling" in volume)
print("Kubelet creates declared-feature admission only under NodeDeclaredFeatures:",
bool(re.search(
r'if utilfeature\.DefaultFeatureGate\.Enabled\(features\.NodeDeclaredFeatures\)\s*\{'
r'.{0,300}NewDeclaredFeaturesAdmitHandler',
kubelet, re.S)))
print("Scheduler declared-feature plugin is gated by NodeDeclaredFeatures:",
"features.NodeDeclaredFeatures" in scheduler)
PYRepository: openshift/kubernetes
Length of output: 584
🏁 Script executed:
#!/bin/bash
set -eu
cat -n pkg/scheduler/framework/plugins/nodedeclaredfeatures/nodedeclaredfeatures.go | sed -n '45,100p'
rg -n -C 12 'NewNodeDeclaredFeatures|NodeDeclaredFeatures\(' \
pkg/scheduler --glob '*.go'
rg -n -C 12 'NodeDeclaredFeatures' \
pkg/scheduler/framework/runtime pkg/scheduler/apis pkg/scheduler --glob '*.go' | head -300Repository: openshift/kubernetes
Length of output: 39202
Add NodeDeclaredFeatures to the VolumeBindMountOptions dependency list. VolumeBindMountOptions infers node requirements from BindMountOptions, but scheduler filtering and kubelet admission run only when NodeDeclaredFeatures is enabled. Use VolumeBindMountOptions: {NodeDeclaredFeatures}.
🤖 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/features/kube_features.go` at line 2741, Update the
VolumeBindMountOptions dependency declaration to include NodeDeclaredFeatures,
changing the empty dependency set to the required NodeDeclaredFeatures entry
while preserving the existing feature registration.
| NodeAllocatableResourceClaimStatuses: []v1.NodeAllocatableResourceClaimStatus{ | ||
| { | ||
| ResourceClaimName: "dra-claim", | ||
| Mapping: []v1.NodeAllocatableMappedResources{ | ||
| { | ||
| Name: v1.ResourceCPU, | ||
| Quantity: new(resource.MustParse("50m")), | ||
| }, | ||
| { | ||
| Name: v1.ResourceMemory, | ||
| Quantity: new(resource.MustParse("50Mi")), | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
new(resource.MustParse(...)) does not compile.
The builtin new accepts a type, not a value. new(resource.MustParse("50m")) passes an expression, so the package fails to build. This breaks every test in pkg/kubelet.
ptr is already imported in this file and used at line 9156.
🐛 Proposed fix
Mapping: []v1.NodeAllocatableMappedResources{
{
Name: v1.ResourceCPU,
- Quantity: new(resource.MustParse("50m")),
+ Quantity: ptr.To(resource.MustParse("50m")),
},
{
Name: v1.ResourceMemory,
- Quantity: new(resource.MustParse("50Mi")),
+ Quantity: ptr.To(resource.MustParse("50Mi")),
},
},📝 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.
| NodeAllocatableResourceClaimStatuses: []v1.NodeAllocatableResourceClaimStatus{ | |
| { | |
| ResourceClaimName: "dra-claim", | |
| Mapping: []v1.NodeAllocatableMappedResources{ | |
| { | |
| Name: v1.ResourceCPU, | |
| Quantity: new(resource.MustParse("50m")), | |
| }, | |
| { | |
| Name: v1.ResourceMemory, | |
| Quantity: new(resource.MustParse("50Mi")), | |
| }, | |
| }, | |
| }, | |
| }, | |
| NodeAllocatableResourceClaimStatuses: []v1.NodeAllocatableResourceClaimStatus{ | |
| { | |
| ResourceClaimName: "dra-claim", | |
| Mapping: []v1.NodeAllocatableMappedResources{ | |
| { | |
| Name: v1.ResourceCPU, | |
| Quantity: ptr.To(resource.MustParse("50m")), | |
| }, | |
| { | |
| Name: v1.ResourceMemory, | |
| Quantity: ptr.To(resource.MustParse("50Mi")), | |
| }, | |
| }, | |
| }, | |
| }, |
🤖 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/kubelet_pods_test.go` around lines 5748 - 5762, Replace the
invalid new(resource.MustParse(...)) expressions in
NodeAllocatableResourceClaimStatuses with the existing ptr helper, passing each
parsed resource.Quantity value to ptr.To. Preserve the CPU and memory quantities
and use the same approach already established elsewhere in kubelet_pods_test.go.
| func TestMakemountsSubpathCleanupAccumulation(t *testing.T) { | ||
| logger, _ := ktesting.NewTestContext(t) | ||
|
|
||
| podDir := t.TempDir() | ||
| volPath := filepath.Join(podDir, "volumes", "disk") | ||
| require.NoError(t, os.MkdirAll(volPath, 0755)) | ||
|
|
||
| // Create the subpath directories on disk so PathExists returns true | ||
| sub1 := filepath.Join(volPath, "sub1") | ||
| sub2 := filepath.Join(volPath, "sub2") | ||
| sub3 := filepath.Join(volPath, "sub3") | ||
| require.NoError(t, os.MkdirAll(sub1, 0755)) | ||
| require.NoError(t, os.MkdirAll(sub2, 0755)) | ||
| require.NoError(t, os.MkdirAll(sub3, 0755)) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| volumeMounts []v1.VolumeMount | ||
| expectedCleanedSubPaths []string | ||
| expectError bool | ||
| errorOnSubPath string | ||
| }{ | ||
| { | ||
| name: "multiple subpath mounts all get cleanup called", | ||
| volumeMounts: []v1.VolumeMount{ | ||
| {MountPath: "/mnt/a", Name: "disk", SubPath: "sub1"}, | ||
| {MountPath: "/mnt/b", Name: "disk", SubPath: "sub2"}, | ||
| {MountPath: "/mnt/c", Name: "disk", SubPath: "sub3"}, | ||
| }, | ||
| expectedCleanedSubPaths: []string{"sub1", "sub2", "sub3"}, | ||
| }, | ||
| { | ||
| name: "single subpath mount cleanup called once", | ||
| volumeMounts: []v1.VolumeMount{ | ||
| {MountPath: "/mnt/a", Name: "disk", SubPath: "sub1"}, | ||
| }, | ||
| expectedCleanedSubPaths: []string{"sub1"}, | ||
| }, | ||
| { | ||
| name: "no subpath mounts no cleanup needed", | ||
| volumeMounts: []v1.VolumeMount{ | ||
| {MountPath: "/mnt/a", Name: "disk"}, | ||
| }, | ||
| }, | ||
| { | ||
| name: "error on third mount still cleans up first two", | ||
| volumeMounts: []v1.VolumeMount{ | ||
| {MountPath: "/mnt/a", Name: "disk", SubPath: "sub1"}, | ||
| {MountPath: "/mnt/b", Name: "disk", SubPath: "sub2"}, | ||
| {MountPath: "/mnt/c", Name: "disk", SubPath: "sub3"}, | ||
| }, | ||
| expectedCleanedSubPaths: []string{"sub1", "sub2"}, | ||
| expectError: true, | ||
| errorOnSubPath: "sub3", | ||
| }, | ||
| { | ||
| name: "error on first mount returns zero cleanups", | ||
| volumeMounts: []v1.VolumeMount{ | ||
| {MountPath: "/mnt/a", Name: "disk", SubPath: "sub1"}, | ||
| {MountPath: "/mnt/b", Name: "disk", SubPath: "sub2"}, | ||
| }, | ||
| expectError: true, | ||
| errorOnSubPath: "sub1", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| tracker := &trackingSubpath{ | ||
| errorOnSubPath: tc.errorOnSubPath, | ||
| } | ||
|
|
||
| container := v1.Container{ | ||
| VolumeMounts: tc.volumeMounts, | ||
| } | ||
|
|
||
| // FakeHostUtil with the subpath directories registered so PathExists returns true | ||
| fhu := hostutil.NewFakeHostUtil(map[string]hostutil.FileType{ | ||
| sub1: hostutil.FileTypeDirectory, | ||
| sub2: hostutil.FileTypeDirectory, | ||
| sub3: hostutil.FileTypeDirectory, | ||
| }) | ||
|
|
||
| podVolumes := kubecontainer.VolumeMap{ | ||
| "disk": kubecontainer.VolumeInfo{Mounter: &stubVolume{path: volPath}}, | ||
| } | ||
|
|
||
| pod := v1.Pod{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| UID: "test-pod-uid", | ||
| }, | ||
| Spec: v1.PodSpec{}, | ||
| } | ||
|
|
||
| _, cleanupAction, err := makeMounts(logger, &pod, podDir, &container, "fakepod", "", []string{""}, podVolumes, fhu, tracker, nil, false, nil) | ||
|
|
||
| if tc.expectError { | ||
| require.Error(t, err) | ||
| } else { | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| // Call the cleanup action returned by makeMounts | ||
| require.NotNil(t, cleanupAction, "cleanupAction should never be nil") | ||
| cleanupAction() | ||
|
|
||
| require.ElementsMatch(t, tc.expectedCleanedSubPaths, tracker.cleanedSubPaths, | ||
| "expected cleaned subpaths %v but got %v", tc.expectedCleanedSubPaths, tracker.cleanedSubPaths) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect cleanupAction handling in makeMounts.
set -uo pipefail
rg -nP -C4 'cleanupAction' pkg/kubelet/kubelet_pods.go
ast-grep run --pattern 'func makeMounts($$$) ([]kubecontainer.Mount, func(), error) { $$$ }' \
--lang go pkg/kubelet/kubelet_pods.goRepository: openshift/kubernetes
Length of output: 17538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resource quantity expressions ---'
rg -n -C4 'new\(resource\.MustParse|MustParse\("50m"\)|enableDRANodeAllocatableResources' pkg/kubelet/kubelet_pods_test.go
printf '%s\n' '--- cleanup test and tracker ---'
rg -n -C6 'trackingSubpath|TestMakemountsSubpathCleanupAccumulation|error on first mount|cleanedSubPaths|PrepareSafeSubpath' pkg/kubelet/kubelet_pods_test.go pkg/kubelet/kubelet_pods.goRepository: openshift/kubernetes
Length of output: 9761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("pkg/kubelet/kubelet_pods_test.go").read_text()
impl = Path("pkg/kubelet/kubelet_pods.go").read_text()
quantity_calls = re.findall(r'Quantity:\s+new\(resource\.MustParse\("([^"]+)"\)\)', test)
assert quantity_calls == ["50m", "50Mi"], quantity_calls
assert "var cleanupAction func()" in impl
assert "hostPath, cleanupAction, err = subpather.PrepareSafeSubpath(" in impl
# Model the production assignment semantics for the test's cleanup callbacks.
def returned_cleanups(outcomes):
cleanup = None
for name, succeeds in outcomes:
cleanup = (name,) if succeeds else None
return [] if cleanup is None else list(cleanup)
assert returned_cleanups([("sub1", True), ("sub2", True), ("sub3", True)]) == ["sub3"]
assert returned_cleanups([("sub1", False), ("sub2", True)]) == []
assert returned_cleanups([("sub1", True), ("sub2", True), ("sub3", False)]) == []
print("invalid Quantity pointer expressions: 50m, 50Mi")
print("successful subpath cleanup callbacks returned: sub3 only")
print("first/third error cleanup callbacks returned: none")
PYRepository: openshift/kubernetes
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("pkg/kubelet/kubelet_pods_test.go").read_text()
impl = Path("pkg/kubelet/kubelet_pods.go").read_text()
quantity_calls = re.findall(r'Quantity:\s+new\(resource\.MustParse\("([^"]+)"\)\)', test)
assert quantity_calls == ["50m", "50Mi"], quantity_calls
assert "var cleanupAction func()" in impl
assert "hostPath, cleanupAction, err = subpather.PrepareSafeSubpath(" in impl
def returned_cleanups(outcomes):
cleanup = None
for name, succeeds in outcomes:
cleanup = (name,) if succeeds else None
return [] if cleanup is None else list(cleanup)
assert returned_cleanups([("sub1", True), ("sub2", True), ("sub3", True)]) == ["sub3"]
assert returned_cleanups([("sub1", False), ("sub2", True)]) == []
assert returned_cleanups([("sub1", True), ("sub2", True), ("sub3", False)]) == []
print("invalid Quantity pointer expressions: 50m, 50Mi")
print("successful subpath cleanup callbacks returned: sub3 only")
print("first/third error cleanup callbacks returned: none")
PYRepository: openshift/kubernetes
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
test = Path("pkg/kubelet/kubelet_pods_test.go").read_text()
impl = Path("pkg/kubelet/kubelet_pods.go").read_text()
print("quantity:", re.findall(r'Quantity:\s+new\(resource\.MustParse\("([^"]+)"\)\)', test))
print("has var cleanupAction:", "var cleanupAction func()" in impl)
print("has direct assignment:", "hostPath, cleanupAction, err = subpather.PrepareSafeSubpath(" in impl)
def returned_cleanups(outcomes):
cleanup = None
for name, succeeds in outcomes:
cleanup = (name,) if succeeds else None
return [] if cleanup is None else list(cleanup)
for outcomes in (
[("sub1", True), ("sub2", True), ("sub3", True)],
[("sub1", False), ("sub2", True)],
[("sub1", True), ("sub2", True), ("sub3", False)],
):
print(outcomes, "=>", returned_cleanups(outcomes))
PYRepository: openshift/kubernetes
Length of output: 403
Fix the invalid quantity pointers and accumulate subpath cleanups.
new(resource.MustParse("50m"))andnew(resource.MustParse("50Mi"))do not compile. Useptr.To(resource.MustParse(...)).makeMountsoverwritescleanupActionatpkg/kubelet/kubelet_pods.go:361. Only the last successful subpath cleanup is returned. An error also returnsnil, so the test'srequire.NotNilassertion fails. Accumulate all cleanup callbacks and invoke previously created callbacks on errors.
🤖 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/kubelet_pods_test.go` around lines 9276 - 9386, Fix the invalid
resource quantity pointer construction by replacing new(resource.MustParse(...))
usages with ptr.To(resource.MustParse(...)). Update makeMounts to accumulate
every successful subpath cleanup callback instead of overwriting cleanupAction,
and return a non-nil aggregate cleanup callback even when mount processing
errors; invoke all callbacks created before the error.
| - bindMountOptions: | ||
| - bindMountOptionsValue | ||
| mountPath: mountPathValue |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use an allowed value in every bind-mount fixture.
The API contract allows only noexec, nodev, and nosuid. Replace bindMountOptionsValue with a supported option in every affected fixture entry.
staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L445-L447: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L676-L678: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L908-L910: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L702-L705: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L1019-L1022: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L1336-L1339: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L481-L483: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L712-L714: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L944-L946: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L560-L563: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L877-L880: Replace the placeholder value.staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L1194-L1197: Replace the placeholder value.
📍 Affects 4 files
staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L445-L447(this comment)staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L676-L678staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml#L908-L910staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L702-L705staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L1019-L1022staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json#L1336-L1339staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L481-L483staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L712-L714staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml#L944-L946staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L560-L563staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L877-L880staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json#L1194-L1197
🤖 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 `@staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml` around lines 445 -
447, Replace every bindMountOptionsValue placeholder in
staging/src/k8s.io/api/testdata/HEAD/batch.v1.Job.yaml lines 445-447, 676-678,
and 908-910; staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.json
lines 702-705, 1019-1022, and 1336-1339;
staging/src/k8s.io/api/testdata/HEAD/batch.v1beta1.CronJob.yaml lines 481-483,
712-714, and 944-946; and staging/src/k8s.io/api/testdata/HEAD/core.v1.Pod.json
lines 560-563, 877-880, and 1194-1197 with an allowed bind-mount option: noexec,
nodev, or nosuid.
| func (f *volumeBindMountOptionsFeature) InferForUpdate(oldPodInfo, newPodInfo *types.PodInfo) bool { | ||
| return false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare InferForUpdate implementations across node-declared features.
set -uo pipefail
ast-grep run --pattern 'func ($_ $_) InferForUpdate($$$) bool { $$$ }' \
--lang go staging/src/k8s.io/component-helpers/nodedeclaredfeatures
# Find where the framework calls InferForUpdate to confirm the contract.
rg -nP -C6 '\bInferForUpdate\s*\(' --type=goRepository: openshift/kubernetes
Length of output: 6267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- feature implementation and tests ---'
sed -n '1,180p' staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go
fd -i '.*' staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions
echo '--- framework update contract and callers ---'
rg -n -C8 'InferForUpdate|PodInfo|RequiredRuntimeFeatures' staging/src/k8s.io/component-helpers/nodedeclaredfeatures \
--glob '*.go'
echo '--- ephemeral-container update handling ---'
rg -n -C5 'EphemeralContainers|ephemeral container|EphemeralContainer' staging/src/k8s.io --glob '*.go' | head -n 300Repository: openshift/kubernetes
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- update inference callers ---'
rg -n -C12 'InferForPodUpdate\(' --glob '*.go' . | head -n 240
echo '--- ephemeral container API validation and update strategy ---'
rg -n -C8 'EphemeralContainers|EphemeralContainer' pkg staging/src/k8s.io --glob '*.go' \
| rg -n 'validation|Validate|Strategy|PrepareForUpdate|ValidateUpdate|EphemeralContainers|VolumeMounts|BindMountOptions' \
| head -n 320
echo '--- bind mount option definitions and feature tests ---'
rg -n -C8 'BindMountOptions|VolumeBindMountOptions|MountOptions' staging/src/k8s.io pkg --glob '*.go' \
| head -n 360
echo '--- relevant API type definitions ---'
rg -n -C12 'type EphemeralContainer(Common)? struct|type VolumeMount struct' staging/src/k8s.io/api pkg --glob '*.go' \
| head -n 220Repository: openshift/kubernetes
Length of output: 47169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ephemeral-container allowed fields ---'
sed -n '88,145p' pkg/apis/core/validation/validation.go
sed -n '3749,3810p' pkg/apis/core/validation/validation.go
echo '--- pod update validation and ephemeralcontainers subresource ---'
rg -n -C12 'ValidatePodUpdate|ephemeralcontainers|Subresource|validatePodUpdate' pkg/apis/core plugin/pkg/admission/nodedeclaredfeatures --glob '*.go' \
| head -n 320
echo '--- node-declared-features admission dispatch ---'
sed -n '70,190p' plugin/pkg/admission/nodedeclaredfeatures/admission.go
sed -n '2935,2990p' pkg/kubelet/kubelet.go
echo '--- bind-mount option API and kubelet consumers ---'
rg -n -C10 'BindMountOptions|MountOptions' --glob '*.go' . \
| head -n 420Repository: openshift/kubernetes
Length of output: 50376
🌐 Web query:
Kubernetes volumeBindMountOptionsFeature InferForUpdate EphemeralContainers feature.go
💡 Result:
The terms you queried represent distinct components and concepts within the Kubernetes codebase and feature management framework: 1. VolumeBindMountOptions: This is a specific Kubernetes feature [1] introduced via KEP-5855 [2] that allows users to apply security-related bind mount flags (such as noexec, nodev, and nosuid) to volume mounts in a Pod specification [2]. It requires the VolumeBindMountOptions feature gate to be enabled [1][3]. 2. InferForUpdate: This is a method associated with the Node Declared Features framework (KEP-5328) [4][5]. The framework uses this method to determine if a pod update requires a specific feature [4][6]. By inspecting the old and new pod specifications, InferForUpdate helps the system decide whether a feature needs to be validated or processed during an update, allowing for more efficient processing by avoiding expensive operations for irrelevant subresources [7][5][6]. 3. EphemeralContainers: This is a feature that allows the addition of temporary containers to running pods for debugging or other purposes [8]. It has its own dedicated logic in the Kubernetes storage registry (e.g., EphemeralContainersREST) to handle updates specifically for these containers [8]. 4. feature.go: This typically refers to files such as pkg/features/kube_features.go [9][10], which manage the registration and definition of feature gates across the Kubernetes codebase. Features like VolumeBindMountOptions or EphemeralContainers are defined and tracked within these configuration files [9]. In summary, VolumeBindMountOptions is a functional feature for mount security [2], InferForUpdate is an architectural mechanism for efficient feature detection during updates [4][6], and EphemeralContainers is a specific pod resource type [8], all of which interact with the broader Kubernetes feature management system often centered around files like feature.go [9][10].
Citations:
- 1: KEP-5855: Add bind mount options to VolumeMount kubernetes/kubernetes#140013
- 2: https://www.kubernetes.dev/resources/keps/5855/
- 3: https://kubernetes-io-vnext-staging.netlify.app/docs/tasks/configure-pod-container/configure-bind-mount-options/
- 4: https://www.kubernetes.dev/resources/keps/5328/
- 5: https://github.com/kubernetes/enhancements/blob/53e6aceece3f1d129cf1d6f753f7084ed034d081/keps/sig-node/5328-node-declared-features/README.md
- 6: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/5328-node-declared-features
- 7: [NodeDeclaredFeatures] Surface subresource to InferForUpdate kubernetes/kubernetes#135280
- 8: https://github.com/kubernetes/kubernetes/blob/release-1.20/pkg/registry/core/pod/storage/storage.go
- 9: https://github.com/kubernetes/kubernetes/blob/e9b95827/pkg/features/kube_features.go
- 10: https://github.com/kubernetes/kubernetes/blob/665fb5d3c5ebd3c7e6c3c5b771957cf519a3570d/pkg/features/kube_features.go
🌐 Web query:
site:github.com/kubernetes/kubernetes "func (f *volumeBindMountOptionsFeature) InferForUpdate"
💡 Result:
The function signature func (f *volumeBindMountOptionsFeature) InferForUpdate is not a standard function within the core Kubernetes codebase as of August 2026. Based on research, this appears to be a hallucinated or non-standard identifier. While there are ongoing efforts in Kubernetes related to "bind mount options" (such as KEP-5855, which introduces a bindMountOptions field to VolumeMount) [1][2] and "InferForUpdate" methods (used, for example, in the NodeDeclaredFeatures testing infrastructure to determine if an update to a pod object requires reconciliation) [3][4][5], there is no existing struct named volumeBindMountOptionsFeature that implements an InferForUpdate method in the main Kubernetes repository. If you encountered this in a specific codebase, it is likely a custom internal implementation or a feature-gated utility used for managing volume-related feature flags or reconciliation logic. [1][3]
Citations:
- 1: KEP-5855: Add bind mount options to VolumeMount kubernetes/kubernetes#140013
- 2: Add mount options on volume mounts kubernetes/kubernetes#139414
- 3: [NodeDeclaredFeatures] Surface subresource to InferForUpdate kubernetes/kubernetes#135280
- 4: https://github.com/kubernetes/kubernetes/blob/63b36867/test/integration/scheduler_perf/nodedeclaredfeatures/node_declared_features_test.go
- 5: https://github.com/kubernetes/kubernetes/blob/e9b95827/test/integration/scheduler_perf/nodedeclaredfeatures/node_declared_features_test.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- bindMountOptions production consumers ---'
rg -n -C10 'BindMountOptions|bindMountOptions' --glob '*.go' \
--glob '!**/*_test.go' \
--glob '!staging/src/k8s.io/mount-utils/**' . | head -n 360
echo '--- ephemeral containers REST update path ---'
rg -n -C12 'EphemeralContainersREST|Update\(.*ephemeral|ephemeralcontainers' pkg/registry pkg/kubelet --glob '*.go' \
| head -n 300
echo '--- pod update framework tests and feature registration ---'
sed -n '333,490p' staging/src/k8s.io/component-helpers/nodedeclaredfeatures/framework_test.go
rg -n -C8 'volumebindmountoptions|VolumeBindMountOptions' staging/src/k8s.io/component-helpers/nodedeclaredfeatures --glob '*.go'Repository: openshift/kubernetes
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- all non-generated bind option references ---'
rg -l -i 'bindmountoptions|bind_mount_options' --glob '*.go' . \
| rg -v '(_test\.go|generated|zz_generated|mount-utils)' \
| sort
echo '--- pod feature-gate drop behavior ---'
sed -n '780,835p' pkg/api/pod/util.go
sed -n '1555,1590p' pkg/api/pod/util.go
echo '--- kubelet mount construction ---'
sed -n '360,430p' pkg/kubelet/kubelet_pods.go
rg -n -C12 'MountFlags|MountSensitive|VolumeMounts' pkg/kubelet pkg/volume staging/src/k8s.io --glob '*.go' \
| rg -i 'bind|mountflags|volumeMount' | head -n 260Repository: openshift/kubernetes
Length of output: 36012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- runtime bind option handling ---'
rg -n -C14 'BindMountOptions' pkg/kubelet/container/runtime.go pkg/kubelet/kuberuntime/kuberuntime_container.go pkg/kubelet/kubelet_pods.go
echo '--- feature usage in pod update processing ---'
rg -n -C10 'dropDisabled|Drop.*Disabled|Prepare.*Pod|VolumeBindMountOptions' pkg/kubelet pkg/api --glob '*.go' \
| head -n 260
echo '--- exact ephemeral subresource strategy behavior ---'
rg -n -C15 'EphemeralContainersStrategy|PrepareForUpdate|ValidateUpdate' pkg/registry/core/pod --glob '*.go' \
| head -n 260Repository: openshift/kubernetes
Length of output: 43435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
feature = Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go").read_text()
framework = Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/framework.go").read_text()
admission = Path("plugin/pkg/admission/nodedeclaredfeatures/admission.go").read_text()
kubelet = Path("pkg/kubelet/kubelet.go").read_text()
strategy = Path("pkg/registry/core/pod/strategy.go").read_text()
runtime = Path("pkg/kubelet/kuberuntime/kuberuntime_container.go").read_text()
infer = re.search(
r"func \(f \*volumeBindMountOptionsFeature\) InferForUpdate\(.*?\n\}",
feature, re.S
)
assert infer and "return false" in infer.group(0), "InferForUpdate is not unconditional false"
assert "f.InferForUpdate(oldPodInfo, newPodInfo)" in framework
assert "InferForPodUpdate(oldPodInfo, newPodInfo" in kubelet
assert "newPod.Spec.EphemeralContainers" in strategy
assert 'subresource != "" && subresource != "resize"' in admission
assert "BindMountOptions: mount.BindMountOptions" in Path("pkg/kubelet/kubelet_pods.go").read_text()
assert "MountOptions: v.BindMountOptions" in runtime
print("InferForUpdate returns false; the framework evaluates it for kubelet pod updates.")
print("Ephemeral containers are persisted through the ephemeralcontainers subresource.")
print("The node-declared-features admission plugin excludes that subresource.")
print("BindMountOptions is propagated to the kubelet runtime mount request.")
PYRepository: openshift/kubernetes
Length of output: 247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
checks = {
"InferForUpdate returns false": (
re.search(
r"func \(f \*volumeBindMountOptionsFeature\) InferForUpdate\(.*?\n\}",
Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go").read_text(),
re.S,
)
and "return false" in re.search(
r"func \(f \*volumeBindMountOptionsFeature\) InferForUpdate\(.*?\n\}",
Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go").read_text(),
re.S,
).group(0)
),
"framework calls InferForUpdate": "f.InferForUpdate(oldPodInfo, newPodInfo)" in Path("staging/src/k8s.io/component-helpers/nodedeclaredfeatures/framework.go").read_text(),
"kubelet calls InferForPodUpdate": "InferForPodUpdate(oldPodInfo, newPodInfo" in Path("pkg/kubelet/kubelet.go").read_text(),
"ephemeral strategy persists new list": "newPod.Spec.EphemeralContainers" in Path("pkg/registry/core/pod/strategy.go").read_text(),
"admission excludes non-main and resize subresources": 'subresource != "" && subresource != "resize"' in Path("plugin/pkg/admission/nodedeclaredfeatures/admission.go").read_text(),
"kubelet propagates bind options": "BindMountOptions" in Path("pkg/kubelet/kubelet_pods.go").read_text(),
"runtime propagates mount options": "MountOptions" in Path("pkg/kubelet/kuberuntime/kuberuntime_container.go").read_text(),
}
for name, result in checks.items():
print(f"{'PASS' if result else 'FAIL'}: {name}")
assert all(checks.values())
PYRepository: openshift/kubernetes
Length of output: 446
Infer update requirements for ephemeral containers.
InferForUpdate always returns false, although InferForPodUpdate evaluates it for kubelet pod updates. Return true when the new pod requires VolumeBindMountOptions and the old pod did not, including when an ephemeralcontainers update adds BindMountOptions. Otherwise the update bypasses the node's MountOptions requirement. The admission plugin also skips the ephemeralcontainers subresource, so include that subresource if admission-time rejection is required.
🤖 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
`@staging/src/k8s.io/component-helpers/nodedeclaredfeatures/features/volumebindmountoptions/feature.go`
around lines 80 - 82, Update volumeBindMountOptionsFeature.InferForUpdate to
return true when newPodInfo requires VolumeBindMountOptions but oldPodInfo does
not, including additions of BindMountOptions through ephemeral container
updates. Ensure the related admission handling includes the ephemeralcontainers
subresource if needed to enforce the node MountOptions requirement.
| // mount_options specifies additional bind mount options (e.g., noexec, | ||
| // nodev, nosuid) that the runtime must apply when mounting this volume | ||
| // into the container. These are passed as OCI mount options. | ||
| repeated string mount_options = 11; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the generated CRI Go bindings expose the new fields.
set -uo pipefail
fd -t f 'api.pb.go' staging/src/k8s.io/cri-api || true
fd -t f 'api.pb.go' staging/src/k8s.io/cri-api --exec rg -n 'MountOptions' {} \;
# Confirm the field tags used in the generated struct tags.
fd -t f 'api.pb.go' staging/src/k8s.io/cri-api --exec rg -n 'mount_options,(3|11)' {} \;Repository: openshift/kubernetes
Length of output: 635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- proto fields ---'
sed -n '340,370p' staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
sed -n '1870,1890p' staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
printf '%s\n' '--- feature and mount option usage ---'
rg -n -C 6 'MountOptions|RuntimeFeatures' \
pkg/kubelet/kuberuntime \
staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.goRepository: openshift/kubernetes
Length of output: 28587
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- BindMountOptions assignments and feature checks ---'
rg -n -C 5 'BindMountOptions|MountOptions' \
pkg staging/src/k8s.io/cri-api/pkg/apis/runtime/v1 \
-g '*.go' -g '*.proto'
printf '%s\n' '--- RuntimeFeatures.mount_options checks ---'
rg -n -C 8 'MountOptions' pkg/kubelet -g '*.go'Repository: openshift/kubernetes
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- kubelet MountOptions references ---'
rg -n 'MountOptions|BindMountOptions' pkg/kubelet/kuberuntime pkg/kubelet/container -g '*.go'
printf '%s\n' '--- runtime feature propagation and status consumers ---'
rg -n -C 4 'RuntimeFeatures|RuntimeStatus|Features:' pkg/kubelet -g '*.go' | head -n 220
printf '%s\n' '--- makeMounts implementation ---'
sed -n '460,510p' pkg/kubelet/kuberuntime/kuberuntime_container.goRepository: openshift/kubernetes
Length of output: 22693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,455p' pkg/kubelet/nodestatus/setters.go
rg -n -C 5 'NodeFeatures\\(|Features\\(.*Runtime|MountOptions' pkg -g '*.go' | head -n 160Repository: openshift/kubernetes
Length of output: 1299
Gate bind mount options on runtime support. makeMounts always copies BindMountOptions into runtimeapi.Mount.MountOptions, but the kubelet does not check RuntimeFeatures.MountOptions. A runtime that does not support the field can ignore noexec, nodev, or nosuid. Reject unsupported requests before creating the container.
🤖 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 `@staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto` around lines 359 -
362, Update the container mount creation path around makeMounts to check
RuntimeFeatures.MountOptions before propagating BindMountOptions into
runtimeapi.Mount.MountOptions. Reject the pod or container request when bind
mount options are specified but runtime support is unavailable, before container
creation; preserve existing behavior for supported runtimes and mounts without
options.
|
@amritansh1502: 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 |
|
@amritansh1502: No Jira issue with key KEP-5855 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. |
…patibility Restore util.go, util_test.go, and api.pb.go from openshift/master base, then surgically add only VolumeBindMountOptions code. Remove upstream-only references (dranodeallocatableresources, EmptyDirVolumeMode, Signal enums) that do not exist in openshift/master.
619147d to
baf96dc
Compare
|
@amritansh1502: 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 |
|
@amritansh1502: No Jira issue with key KEP-5855 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/pod/util.go (1)
805-821: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for init and ephemeral containers.
TestDropVolumeBindMountOptionsonly createsSpec.Containers. It does not verify the init-container and ephemeral-container loops in this block. Add old and new pod cases withBindMountOptionsin both container types for enabled and disabled feature-gate states.🤖 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/api/pod/util.go` around lines 805 - 821, Extend TestDropVolumeBindMountOptions to include old and new pod fixtures containing BindMountOptions in InitContainers and EphemeralContainers. Cover both enabled and disabled VolumeBindMountOptions feature-gate states, asserting options are preserved when enabled and removed when disabled.
🤖 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.
Nitpick comments:
In `@pkg/api/pod/util.go`:
- Around line 805-821: Extend TestDropVolumeBindMountOptions to include old and
new pod fixtures containing BindMountOptions in InitContainers and
EphemeralContainers. Cover both enabled and disabled VolumeBindMountOptions
feature-gate states, asserting options are preserved when enabled and removed
when disabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac09b0f-88b9-4c2a-b4f6-22175b636f77
⛔ Files ignored due to path filters (1)
staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (3)
pkg/api/pod/util.gopkg/api/pod/util_test.gostaging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
🚧 Files skipped from review as they are similar to previous changes (2)
- staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto
- pkg/api/pod/util_test.go
3 participants
Summary by CodeRabbit
New Features
noexec,nodev, andnosuid.VolumeBindMountOptionsfeature gate in Kubernetes 1.37.Bug Fixes
Tests