OCPNODE-4055: Add Additional Storage Support - API validation test cases#31384
OCPNODE-4055: Add Additional Storage Support - API validation test cases#31384BhargaviGudi wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@BhargaviGudi: This pull request references OCPNODE-4055 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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
jira/valid-reference
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a feature-gated OpenShift conformance suite validating Additional Storage API path and count constraints through dry-run requests, with image and artifact smoke coverage and README documentation. ChangesAdditional Storage API validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GinkgoSuite
participant SkipHelper
participant ClusterConfig
participant ContainerRuntimeConfigAPI
GinkgoSuite->>SkipHelper: check AdditionalStorageConfig prerequisites
SkipHelper->>ClusterConfig: read platform and FeatureGate
ClusterConfig-->>SkipHelper: return platform and enabled feature gates
GinkgoSuite->>ContainerRuntimeConfigAPI: submit Additional*Stores request with DryRun
ContainerRuntimeConfigAPI-->>GinkgoSuite: return validation result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
/assign @saschagrunert @cpmeadors |
|
/label acknowledge-critical-fixes-only |
|
/test e2e-gcp-ovn-techpreview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/node/node_utils.go (1)
908-947: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSkip-on-fetch-error masks real cluster/RBAC problems as "skipped" instead of "failed".
If
IsMicroShiftCluster,Infrastructures().Get, orFeatureGates().Getfail (e.g. transient API error or an RBAC issue), the test silently skips viag.Skip(...)rather than failing. That hides genuine test-infrastructure problems behind a green/skipped result. Note this diverges from the analogous existing helper (isFeatureGateEnabled/SkipWhenFeatureGateEnabledintest/extended/machine_config/helpers.go), which useso.Expect(err).NotTo(o.HaveOccurred(), ...)to fail loudly on fetch errors rather than skip.🔧 Suggested fix: fail loudly on fetch errors, skip only on confirmed condition
isMicroShift, err := exutil.IsMicroShiftCluster(oc.AdminKubeClient()) - if err != nil { - framework.Logf("Failed to detect MicroShift cluster: %v", err) - g.Skip("Cannot verify cluster type") - } + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to detect MicroShift cluster") if isMicroShift { g.Skip("Skipping test on MicroShift cluster - MachineConfig resources are not available") } - // Skip on Microsoft + // Skip on Microsoft Azure infra, err := oc.AdminConfigClient().ConfigV1().Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - framework.Logf("Failed to get Infrastructure resource: %v", err) - g.Skip("Cannot verify platform type") - } + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get Infrastructure resource") if infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.Type == configv1.AzurePlatformType { g.Skip("Skipping test on Microsoft Azure cluster") } // Verify AdditionalStorageConfig feature gate is enabled g.By("Verifying AdditionalStorageConfig feature gate is enabled") fgs, err := oc.AdminConfigClient().ConfigV1().FeatureGates().Get(ctx, "cluster", metav1.GetOptions{}) - if err != nil { - framework.Logf("Failed to get FeatureGate resource: %v", err) - g.Skip("Cannot verify AdditionalStorageConfig feature gate requirement") - } + o.Expect(err).NotTo(o.HaveOccurred(), "Failed to get FeatureGate resource")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/node/node_utils.go` around lines 908 - 947, Update skipUnlessAdditionalStorageConfigEnabled to fail loudly when IsMicroShiftCluster, Infrastructures().Get, or FeatureGates().Get returns an error, using the established expectation pattern from isFeatureGateEnabled/SkipWhenFeatureGateEnabled rather than g.Skip. Retain skipping only for confirmed MicroShift, Azure, or disabled AdditionalStorageConfig conditions.test/extended/node/additional_storage_api.go (1)
33-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSignificant boilerplate duplication across all 13 test cases.
Each
Itrepeats the samemcClientconstruction,ObjectMeta, andMachineConfigPoolSelectorscaffolding, differing only in theContainerRuntimeConfigfields under test. A small helper that builds the baseContainerRuntimeConfig/client and accepts a mutator (or just the store-type spec) would cut this file's size substantially and make future test additions less error-prone.♻️ Sketch of a shared helper
func newAdditionalStorageCtrCfg(name string, cfg *machineconfigv1.ContainerRuntimeConfiguration) *machineconfigv1.ContainerRuntimeConfig { return &machineconfigv1.ContainerRuntimeConfig{ ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: machineconfigv1.ContainerRuntimeConfigSpec{ MachineConfigPoolSelector: &metav1.LabelSelector{ MatchLabels: map[string]string{ "pools.operator.machineconfiguration.openshift.io/worker": "", }, }, ContainerRuntimeConfig: cfg, }, } }This is a nice-to-have; the existing verbose style is consistent with other node e2e tests (e.g.
test/extended/node/node_e2e/container_runtime_config.go), so treat as optional cleanup rather than a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/extended/node/additional_storage_api.go` around lines 33 - 493, Reduce repeated setup across the additional storage tests by introducing a shared helper, such as newAdditionalStorageCtrCfg, that accepts the test name and ContainerRuntimeConfiguration while constructing the common metadata and worker MachineConfigPoolSelector. Update the 13 test cases in the Combined Additional Stores, Additional Layer Stores, Additional Image Stores, and Additional Artifact Stores contexts to use it; keep each test’s client creation and validation-specific store fields unchanged.
🤖 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 `@test/extended/node/additional_storage_api.go`:
- Around line 33-493: Reduce repeated setup across the additional storage tests
by introducing a shared helper, such as newAdditionalStorageCtrCfg, that accepts
the test name and ContainerRuntimeConfiguration while constructing the common
metadata and worker MachineConfigPoolSelector. Update the 13 test cases in the
Combined Additional Stores, Additional Layer Stores, Additional Image Stores,
and Additional Artifact Stores contexts to use it; keep each test’s client
creation and validation-specific store fields unchanged.
In `@test/extended/node/node_utils.go`:
- Around line 908-947: Update skipUnlessAdditionalStorageConfigEnabled to fail
loudly when IsMicroShiftCluster, Infrastructures().Get, or FeatureGates().Get
returns an error, using the established expectation pattern from
isFeatureGateEnabled/SkipWhenFeatureGateEnabled rather than g.Skip. Retain
skipping only for confirmed MicroShift, Azure, or disabled
AdditionalStorageConfig conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 963c6316-91bc-4607-824c-c33012a10933
📒 Files selected for processing (3)
test/extended/node/README.mdtest/extended/node/additional_storage_api.gotest/extended/node/node_utils.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-ci-4.22-e2e-gcp-ovn-techpreview |
|
@BhargaviGudi: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/77b3d950-7fa6-11f1-896a-c1d5ee81f276-0 |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 2507d15
New tests seen in this PR at sha: 2507d15
|
|
/verified by CI All 13 testcases are passed on 5.0 e2e-gcp-ovn-techpreview and 4.22 4.22-e2e-gcp-ovn-techpreview |
openshift-ci-robot
added
the
verified
|
@BhargaviGudi: This PR has been marked as verified 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-required |
BhargaviGudi
force-pushed
the
additionStorageSupportAPI
branch
from
2507d15 to
cc37cba
Compare
openshift-ci-robot
removed
the
verified
|
/verified by CI All 13 testcases are passed on 5.0 e2e-gcp-ovn-techpreview and 4.22 4.22-e2e-gcp-ovn-techpreview |
openshift-ci-robot
added
the
verified
|
@BhargaviGudi: This PR has been marked as verified 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. |
|
@BhargaviGudi: This pull request references OCPNODE-4055 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
Scheduling required tests: |
|
@BhargaviGudi: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi, saschagrunert 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 |
3 participants
Summary
Add comprehensive API validation tests for Additional Storage Support (additionalArtifactStores, additionalImageStores, additionalLayerStores) feature. These tests validate ContainerRuntimeConfig API behavior
using DryRun, ensuring proper validation without triggering MCO reconciliation.
Test Coverage
13 API validation tests across 4 categories:
Combined Additional Stores (3 tests)
Additional Layer Stores (8 tests)
Additional Image Stores (1 test)
Additional Artifact Stores (1 test)
Test Characteristics
openshift/conformance/parallel[Feature:AdditionalStorageSupport]AdditionalStorageConfigFiles Changed
New Files
test/extended/node/additional_storage_api.go- 13 API validation testsModified Files
test/extended/node/node_utils.go- AddedskipUnlessAdditionalStorageConfigEnabled()helper function with platform and feature gate checkstest/extended/node/README.md- Added documentation for API validation test suitePrerequisites
Related
Summary by CodeRabbit
New Features
Tests
Documentation