WIP: OCPNODE-4060: Add additional storage configuration fields to ContainerRuntimeConfig #2681
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@saschagrunert: This pull request references OCPNODE-4060 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughAdds three new fields to ContainerRuntimeConfiguration: AdditionalLayerStores, AdditionalImageStores, and AdditionalArtifactStores, with corresponding public types AdditionalLayerStore, AdditionalImageStore, and AdditionalArtifactStore. Generates deepcopy and Swagger documentation for the new types. Introduces a new feature gate FeatureGateAdditionalStorageConfig and enables it in various feature-gate manifests and the CRD featureGates list. Adds multiple CRD YAMLs for ContainerRuntimeConfig (v1) and a YAML test manifest covering creation and validation scenarios for the new storage fields. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @saschagrunert! Some important instructions when contributing to openshift/api: |
|
@saschagrunert: This pull request references OCPNODE-4060 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. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
openshift-ci
bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
![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
🤖 Fix all issues with AI agents
In `@machineconfiguration/v1/types.go`:
- Around line 895-939: The new storage fields AdditionalLayerStores,
AdditionalImageStores, and AdditionalArtifactStores must be gated behind the
TechPreviewNoUpgrade feature gate; add the openshift feature-gate annotation
+openshift:enable:FeatureGate=TechPreviewNoUpgrade directly above each of those
field declarations in types.go so the CRD schema is only emitted when the
TechPreviewNoUpgrade gate is enabled (i.e., add the annotation to the comment
block for AdditionalLayerStores, AdditionalImageStores and
AdditionalArtifactStores).
🧹 Nitpick comments (1)
machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml (1)
6-173: Add a couple of path-validation cases for image/artifact stores.Layer-store paths are thoroughly validated, but image/artifact store path constraints are only covered by max-items tests. A minimal invalid-path case for each would guard against drift.
✅ Proposed test additions
@@ - name: Should fail if additionalImageStores exceeds maximum of 10 items initial: | apiVersion: machineconfiguration.openshift.io/v1 kind: ContainerRuntimeConfig spec: containerRuntimeConfig: additionalImageStores: - path: /var/lib/store1 - path: /var/lib/store2 - path: /var/lib/store3 - path: /var/lib/store4 - path: /var/lib/store5 - path: /var/lib/store6 - path: /var/lib/store7 - path: /var/lib/store8 - path: /var/lib/store9 - path: /var/lib/store10 - path: /var/lib/store11 expectedError: "additionalImageStores in body should have at most 10 items" + + - name: Should fail if additionalImageStores path is not absolute + initial: | + apiVersion: machineconfiguration.openshift.io/v1 + kind: ContainerRuntimeConfig + spec: + containerRuntimeConfig: + additionalImageStores: + - path: var/lib/images + expectedError: "path must be absolute and contain only alphanumeric characters, '/', '_', and '-'" @@ - name: Should fail if additionalArtifactStores exceeds maximum of 10 items initial: | apiVersion: machineconfiguration.openshift.io/v1 kind: ContainerRuntimeConfig spec: containerRuntimeConfig: additionalArtifactStores: - path: /var/lib/store1 - path: /var/lib/store2 - path: /var/lib/store3 - path: /var/lib/store4 - path: /var/lib/store5 - path: /var/lib/store6 - path: /var/lib/store7 - path: /var/lib/store8 - path: /var/lib/store9 - path: /var/lib/store10 - path: /var/lib/store11 expectedError: "additionalArtifactStores in body should have at most 10 items" + + - name: Should fail if additionalArtifactStores path is not absolute + initial: | + apiVersion: machineconfiguration.openshift.io/v1 + kind: ContainerRuntimeConfig + spec: + containerRuntimeConfig: + additionalArtifactStores: + - path: var/lib/artifacts + expectedError: "path must be absolute and contain only alphanumeric characters, '/', '_', and '-'"
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
37efaf4 to
a5273ad
Compare
…ation This commit addresses CodeRabbit PR review feedback by: 1. Feature Gate Definition (AdditionalStorageConfig): - Created proper feature gate "AdditionalStorageConfig" in features/features.go - Gate enabled in DevPreviewNoUpgrade and TechPreviewNoUpgrade feature sets - Linked to enhancement PR openshift/enhancements#1934 - Contact: saschagrunert, Component: MachineConfigOperator 2. Feature Gating Applied: - Added +openshift:enable:FeatureGate=AdditionalStorageConfig to all three storage field declarations (AdditionalLayerStores, AdditionalImageStores, AdditionalArtifactStores) - Fields now only appear in CRD schema when AdditionalStorageConfig feature gate is enabled (DevPreview/TechPreview) - Generated feature-gated CRD manifest: AdditionalStorageConfig.yaml 3. Enhanced Test Coverage: - Added path validation tests for additionalImageStores (non-absolute path, empty struct/MinProperties) - Added path validation tests for additionalArtifactStores (non-absolute path, empty struct/MinProperties) - Ensures all three store types have consistent validation coverage - Total test count: 15 tests 4. Path Validation Enhancement: - Updated regex to allow dots in paths: ^/[a-zA-Z0-9/._-]+$ - Documentation updated to reflect dot support - Test paths include dots (e.g., /opt/layer_store-v1.0) Verification: - Fields present in AdditionalStorageConfig.yaml (feature-gated) ✓ - Fields NOT present in AAA_ungated.yaml ✓ - Linter passes with 0 issues ✓ Addresses: openshift#2681 (review) Addresses: openshift#2681 (comment) Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
openshift-ci
bot
added
size/XXL
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
ec4b294 to
69ba824
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@features.md`:
- Line 27: The new table row with "AdditionalStorageConfig" violates MD060
(inconsistent pipe spacing); update the row so pipe characters align with the
table's existing spacing style (match spaces around pipes used by other rows),
ensuring consistent left/right spacing around each '|' and the same number of
cells as the header to fix the lint error for the line containing
"AdditionalStorageConfig".
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 224-227: Fix the typo in the CRD description for
machineConfigPoolSelector under the ContainerRuntimeConfig resource: change
"shoud" to "should" in the description string for machineConfigPoolSelector to
read "machineConfigPoolSelector selects which pools the ContainerRuntimeConfig
should apply to."
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-Default.crd.yaml`:
- Around line 98-101: Typo: the description for machineConfigPoolSelector in the
ContainerRuntimeConfig CRD contains "shoud" instead of "should"; update the
source type definition that generates these CRDs (the ContainerRuntimeConfig
description field / machineConfigPoolSelector docstring) to correct "shoud" →
"should" so all generated CRD variants (machineConfigPoolSelector in
ContainerRuntimeConfig) get the fixed wording.
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml`:
- Around line 52-97: The CRD is missing the containerRuntimeConfig fields
additionalLayerStores, additionalImageStores, and additionalArtifactStores; add
these three properties under containerRuntimeConfig mirroring the
CustomNoUpgrade CRD's schema: each property should be an array of strings with
item length constraints (1-256 chars), pattern/validation matching the existing
CRD, and min/max item constraints (additionalLayerStores: minItems 1 maxItems 5;
additionalImageStores and additionalArtifactStores: minItems 1 maxItems 10);
ensure types and x-kubernetes validations align with
machineconfiguration/v1/types.go and the CustomNoUpgrade CRD definitions so
schema and validation are consistent.
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 98-101: Fix the typo in the CRD description for the field
machineConfigPoolSelector by changing "shoud" to "should" in both the
ContainerRuntimeConfig and the CustomNoUpgrade variant descriptions; locate the
description block under the machineConfigPoolSelector key and update the wording
to "machineConfigPoolSelector selects which pools the ContainerRuntimeConfig
should apply to." for both occurrences.
- Around line 1-97: The TechPreviewNoUpgrade CRD is missing the storage fields
controlled by the AdditionalStorageConfig feature gate (additionalLayerStores,
additionalImageStores, additionalArtifactStores); update the
ContainerRuntimeConfig schema under spec.containerRuntimeConfig in this CRD to
add those three fields with the same types/anyOf/x-kubernetes-int-or-string and
validation as the CustomNoUpgrade variant and annotate them the same way they
are annotated for the feature gate (i.e. the
+openshift:enable:FeatureGate=AdditionalStorageConfig behavior reflected in the
OpenAPI schema), or alternatively restrict the feature gate scope in
features/features.go to exclude TechPreviewNoUpgrade to keep schemas consistent
with the gate.
🧹 Nitpick comments (2)
machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml (2)
102-142: Consider adding positive test case for additionalImageStores.The tests for
additionalImageStorescover validation errors well (max items, non-absolute path, missing path), but there's no positive test case showing successful creation with valid image store paths, unlikeadditionalLayerStoreswhich has one at lines 7-25.
143-183: Consider adding positive test case for additionalArtifactStores.Similarly,
additionalArtifactStoresvalidation tests are present but a positive test case demonstrating successful creation with valid artifact store paths would provide more complete coverage.
...ad-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-Default.crd.yaml
Show resolved
Hide resolved
...anifests/crds/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
...nifests/crds/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
...nifests/crds/0000_80_machine-config_01_containerruntimeconfigs-TechPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
69ba824 to
d9643eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@machineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml`:
- Line 45: Update the test expectations in AdditionalStorageConfig.yaml where
the expectedError string omits the '.' character: change the three occurrences
of expectedError ("path must be absolute and contain only alphanumeric
characters, '/', '_', and '-'") to include the '.' in the allowed-character list
(e.g., "path must be absolute and contain only alphanumeric characters, '/',
'_', '-', and '.'") so they match the CRD regex; the affected values are the
expectedError fields used in the test cases around the three entries currently
failing.
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 164-174: The validation message for the additionalLayerStores
field is inconsistent: the description and the x-kubernetes-validations regular
expression (rule: self.matches('^/[a-zA-Z0-9/._-]+$')) allow the '.' character
but the message omits it; update the x-kubernetes-validations message to list
'/', '.', '_', and '-' (e.g., "path must be absolute and contain only
alphanumeric characters, '/', '.', '_', and '-'") so it matches the description
and the regex for additionalLayerStores and the rule
self.matches('^/[a-zA-Z0-9/._-]+$').
In
`@payload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml`:
- Around line 224-227: Fix the typo in the CRD description for the
machineConfigPoolSelector field: change "shoud" to "should" in the description
string for machineConfigPoolSelector in the containerruntimeconfigs CRD (the
multi-line description under machineConfigPoolSelector).
...tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml
Outdated
Show resolved
Hide resolved
...ad-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
...anifests/crds/0000_80_machine-config_01_containerruntimeconfigs-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
This enhancement extends ContainerRuntimeConfig API with three features for improved container storage flexibility: - additionalLayerStores: Enable lazy image pulling via storage plugins (BYOS approach with stargz-snapshotter, nydus-storage-plugin) - additionalImageStores: Read-only container image caches on shared or high-performance storage (NFS, SSD) for faster startup and reduced network overhead - additionalArtifactStores: Configurable OCI artifact storage locations (SSD-backed storage, pre-populated caches, air-gapped deployments) All features target AI/ML workload performance improvements and will ship as Tech Preview in 4.22 behind TechPreviewNoUpgrade feature gate. Path-based configuration with graceful fallback to standard behavior on failure. Unified API design pattern across all three features. Signed-off-by: Sascha Grunert <sgrunert@redhat.com>
saschagrunert
force-pushed
the
additional-storage-config-crio
branch
from
d9643eb to
50e1e8a
Compare
|
@qodo-code-review:
|
|
@saschagrunert: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
User description
This enhancement extends ContainerRuntimeConfig API with three features for improved container storage flexibility:
additionalLayerStores: Enable lazy image pulling via storage plugins (BYOS approach with stargz-snapshotter, nydus-storage-plugin)
additionalImageStores: Read-only container image caches on shared or high-performance storage (NFS, SSD) for faster startup and reduced network overhead
additionalArtifactStores: Configurable OCI artifact storage locations (SSD-backed storage, pre-populated caches, air-gapped deployments)
All features target AI/ML workload performance improvements and will ship as Tech Preview in 4.22 behind TechPreviewNoUpgrade feature gate.
Path-based configuration with graceful fallback to standard behavior on failure. Unified API design pattern across all three features.
Requires openshift/enhancements#1934
PR Type
Enhancement
Description
Add three new storage configuration fields to ContainerRuntimeConfig API
Implement comprehensive validation with path constraints and item limits
Generate required Kubernetes API machinery code and CRD manifests
Add comprehensive test suite validating all storage configuration scenarios
Diagram Walkthrough
File Walkthrough
types.go
Define storage configuration types and validation rulesmachineconfiguration/v1/types.go
AdditionalLayerStores,AdditionalImageStores,AdditionalArtifactStoresAdditionalLayerStore,AdditionalImageStore,AdditionalArtifactStorewithpathfield(MinLength, MaxLength, XValidation regex, MinItems, MaxItems)
constraints, and behavior of each field
zz_generated.deepcopy.go
Auto-generate deepcopy functions for storage typesmachineconfiguration/v1/zz_generated.deepcopy.go
AdditionalLayerStoretype
AdditionalImageStoretype
AdditionalArtifactStoretype
storage configuration fields
zz_generated.swagger_doc_generated.go
Generate swagger documentation for storage typesmachineconfiguration/v1/zz_generated.swagger_doc_generated.go
AdditionalLayerStore,AdditionalImageStore,AdditionalArtifactStorestorage configuration fields
AdditionalStorageConfig.yaml
Add comprehensive validation test suitemachineconfiguration/v1/tests/containerruntimeconfigs.machineconfiguration.openshift.io/AdditionalStorageConfig.yaml
configuration
additionalArtifactStores max item limits
existing fields
non-absolute paths, spaces, invalid characters, length limits, item
count limits, missing required fields
0000_80_machine-config_01_containerruntimeconfigs.crd.yaml
Update CRD manifest with storage configuration schemamachineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_containerruntimeconfigs.crd.yaml
additionalLayerStores,additionalImageStores,additionalArtifactStoresfieldsminItems, maxItems, x-kubernetes-validations regex
AAA_ungated.yaml
Update featuregated CRD manifest with storage configurationmachineconfiguration/v1/zz_generated.featuregated-crd-manifests/containerruntimeconfigs.machineconfiguration.openshift.io/AAA_ungated.yaml
additionalLayerStores,additionalImageStores,additionalArtifactStoresfieldsminItems, maxItems, x-kubernetes-validations regex
manifest
0000_80_machine-config_01_containerruntimeconfigs.crd.yaml
Update payload CRD manifest with storage configurationpayload-manifests/crds/0000_80_machine-config_01_containerruntimeconfigs.crd.yaml
additionalLayerStores,additionalImageStores,additionalArtifactStoresfieldsminItems, maxItems, x-kubernetes-validations regex
manifest