OCPNODE-4074: Add additional storage configuration support for CRI-O#5666
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
2 times, most recently
from
ad2ac06 to
d28f84f
Compare
saschagrunert
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
6 times, most recently
from
f8f2e95 to
39db941
Compare
|
couple of nits, but they can be ignored methinks. LGTM generally |
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
39db941 to
b634714
Compare
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
b634714 to
dd5ad73
Compare
saschagrunert
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@saschagrunert: This pull request references OCPNODE-4074 which is a valid jira issue. 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. |
|
/approve |
|
PTAL @djoshy @dkhater-redhat @yuqi-zhang @cheesesashimi @umohnani8 @RishabhSaini @isabella-janssen @pablintino for approval |
|
/hold We have a kube rebase in the pipeline that's high priority. Will unhold once that merges. |
openshift-ci
bot
added
the
do-not-merge/hold
|
/unhold Kube rebase landed, so this can go whenever it is ready now. |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
Extract helper functions for storage and CRI-O drop-in condition checks to deduplicate logic between controller and bootstrap paths. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
dd5ad73 to
2dd1904
Compare
Thanks! Rebased on top of the latest |
openshift-merge-robot
removed
the
needs-rebase
WalkthroughThis PR implements feature-gated support for additional container runtime storage configuration. It propagates a feature gates handler through bootstrap and controller layers, introduces conditional logic for storage and CRI-O drop-in updates based on the AdditionalStorageConfig feature gate, extends CRIO configuration to support additional artifact stores, adds path validation for storage entries, and includes comprehensive test coverage for both gated and ungated scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
|
/lgtm |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pkg/controller/container-runtime-config/container_runtime_config_controller.go`:
- Around line 366-368: The controller currently only checks the feature gate in
additionalStorageConfigEnabled()
(ctrl.fgHandler.Enabled(features.FeatureGateAdditionalStorageConfig) in
ContainerRuntimeConfig controller) but does not watch FeatureGate changes, so
flipping the gate later won't re-enqueue existing ContainerRuntimeConfig CRs;
either add a FeatureGate watch and enqueue existing ContainerRuntimeConfig
objects on FeatureGate change (follow the kubelet controller pattern: add a
FeatureGate informer/handler and enqueue relevant CRs when FeatureGate toggles)
or enforce the gate at admission/reconcile by rejecting or clearing gated fields
when FeatureGateAdditionalStorageConfig is disabled (validate/return error in
the create/update path or strip/reject the additional storage fields in
reconcile), and apply the same change to the other gated-field handling code
paths referenced around the other gated-field helpers in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 006f35d2-b0fb-44c3-ab3e-f225056ac8a3
⛔ Files ignored due to path filters (14)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/AGENTS.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (9)
examples/containerruntimeconfig.crd.yamlgo.modpkg/controller/bootstrap/bootstrap.gopkg/controller/container-runtime-config/container_runtime_config_bootstrap.gopkg/controller/container-runtime-config/container_runtime_config_bootstrap_test.gopkg/controller/container-runtime-config/container_runtime_config_controller.gopkg/controller/container-runtime-config/container_runtime_config_controller_test.gopkg/controller/container-runtime-config/helpers.gopkg/controller/container-runtime-config/helpers_test.go
| func (ctrl *Controller) additionalStorageConfigEnabled() bool { | ||
| return ctrl.fgHandler.Enabled(features.FeatureGateAdditionalStorageConfig) | ||
| } |
There was a problem hiding this comment.
Re-enabling AdditionalStorageConfig will not reprocess existing CRs.
This only samples the gate during reconcile, but this controller never watches FeatureGate changes and FeatureGatesHandler is read-only. A ContainerRuntimeConfig created while the gate is off becomes a no-op/empty MC, and flipping the gate on later will not enqueue it again, so the additional stores stay unapplied until the CR is edited. Please either add a FeatureGate trigger here (similar to the kubelet controller pattern) or reject these gated fields while the gate is disabled.
Also applies to: 760-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@pkg/controller/container-runtime-config/container_runtime_config_controller.go`
around lines 366 - 368, The controller currently only checks the feature gate in
additionalStorageConfigEnabled()
(ctrl.fgHandler.Enabled(features.FeatureGateAdditionalStorageConfig) in
ContainerRuntimeConfig controller) but does not watch FeatureGate changes, so
flipping the gate later won't re-enqueue existing ContainerRuntimeConfig CRs;
either add a FeatureGate watch and enqueue existing ContainerRuntimeConfig
objects on FeatureGate change (follow the kubelet controller pattern: add a
FeatureGate informer/handler and enqueue relevant CRs when FeatureGate toggles)
or enforce the gate at admission/reconcile by rejecting or clearing gated fields
when FeatureGateAdditionalStorageConfig is disabled (validate/return error in
the create/update path or strip/reject the additional storage fields in
reconcile), and apply the same change to the other gated-field handling code
paths referenced around the other gated-field helpers in this file.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The FeatureGatesHandler is read-only and populated once during Connect(). It does not update dynamically when the FeatureGate CR changes at runtime. The kubelet controller's featInformer watch serves a different purpose: it regenerates kubelet configs because feature gate values are embedded directly in KubeletConfiguration. It's not about toggling MCO's own gating behavior.
The existing sigstoreAPIEnabled() in this same controller follows the same pattern.
OpenShift feature gates are not designed to be toggled at runtime. They are set at install time or through TechPreviewNoUpgrade/CustomNoUpgrade feature sets.
There was a problem hiding this comment.
@saschagrunert, thank you for the detailed explanation. You're right — FeatureGatesHandler is snapshot-style and read-only after Connect(), and OpenShift feature gates are not runtime-toggleable. The kubelet controller's informer watch is specifically to re-render KubeletConfiguration content (where gate values appear inline), not to re-trigger gate-guarded behavior. The existing sigstoreAPIEnabled() precedent in this same controller confirms the established pattern. My concern was misplaced — there's nothing to fix here.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
isabella-janssen
left a comment
isabella-janssen
left a comment
There was a problem hiding this comment.
/approve
Code changes look clean to me, added unit tests are passing, and node team has extensively reviewed the functionality of the PR.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haircommander, isabella-janssen, QiWang19, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/verified later @BhargaviGudi |
openshift-ci-robot
added
verified-later
verified
|
@haircommander: This PR has been marked to be verified later by 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. |
|
/retest |
8 participants
- What I did
Implement MCO support for the AdditionalStorageConfig feature gate, adding three new ContainerRuntimeConfig fields:
All fields are gated behind the AdditionalStorageConfig feature gate (DevPreviewNoUpgrade/TechPreviewNoUpgrade) in both the controller and bootstrap code paths, including internal gating in
updateStorageConfigandcreateCRIODropinFileshelpers. Path validation (absolute paths, allowed character set, no consecutive forward slashes) runs regardless of gate state to catch invalid config early. Also fixes pre-existing klog format bugs. Bumps openshift/api to include the merged StorePath API types.Refers to: openshift/enhancements#1934
Jira: https://issues.redhat.com/browse/OCPNODE-4074
- How to verify it
Unit tests cover storage config generation, CRI-O drop-in creation, path validation, and feature gate gating (both enabled and disabled). E2E tests will follow in a separate PR.
- Description for the changelog
Added support for configuring additional layer, image, and artifact stores via
ContainerRuntimeConfig, gated behind theAdditionalStorageConfigfeature gate.