OCPNODE-4055: Add e2e testcase for additional storage support feature#31399
OCPNODE-4055: Add e2e testcase for additional storage support feature#31399BhargaviGudi 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
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds reusable single-node MachineConfigPool lifecycle and polling helpers, plus a feature-gated E2E suite that configures and validates CRI-O additional artifact, image, and layer stores, including registry fallback behavior. ChangesAdditional storage validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdditionalStorageE2E
participant MachineConfigOperator
participant Node
participant CRIO
participant Registry
AdditionalStorageE2E->>Node: Create storage directories and preload image
AdditionalStorageE2E->>MachineConfigOperator: Create ContainerRuntimeConfig
MachineConfigOperator->>Node: Roll out storage configuration
AdditionalStorageE2E->>CRIO: Verify storage configuration and service status
AdditionalStorageE2E->>Node: Start pod from additional image store
AdditionalStorageE2E->>Node: Remove stored image and start fallback pod
Node->>Registry: Pull image after additional store miss
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
test/extended/node/node_utils.go (1)
1174-1265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep one MCP readiness implementation.
This duplicates the existing
waitForMCPlogic at Lines 556-618. Export or wrap the existing helper so readiness semantics cannot diverge.As per coding guidelines, favor clarity and maintainability over cleverness.
🤖 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 1174 - 1265, Remove the duplicated readiness logic from WaitForMCP and reuse the existing waitForMCP implementation near the earlier MCP wait code by exporting or wrapping it. Preserve the current options, timeout, draining behavior, degraded-pool handling, and readiness semantics through that single implementation.Source: Coding guidelines
test/extended/node/node_mcp_helpers.go (1)
75-83: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a bounded, cancellation-detached context in the cleanup examples.
context.Background()loses test context values and leaves cleanup API calls unbounded. Showcontext.WithTimeout(context.WithoutCancel(ctx), ...)instead.Based on learnings, deferred test cleanup should detach cancellation from the existing context and apply an explicit timeout.
Also applies to: 172-181
🤖 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_mcp_helpers.go` around lines 75 - 83, Update the deferred CleanupCustomMCP examples around CreateCustomMCPForNode to pass a context derived from the existing ctx using context.WithoutCancel and an explicit timeout, rather than context.Background(). Apply the same bounded, cancellation-detached context pattern to the additional cleanup example referenced by the comment, preserving deferred cleanup behavior.Source: Learnings
🤖 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 `@test/extended/node/additional_storage_e2e.go`:
- Around line 52-60: Deferred cleanup uses contexts that may be canceled or lack
a deadline. In test/extended/node/additional_storage_e2e.go:52-60, create a
bounded context with context.WithTimeout(context.WithoutCancel(ctx), ...) and
use it for cleanup; at 265, avoid passing the potentially canceled spec context
directly. Update the creation example in
test/extended/node/node_mcp_helpers.go:75-83 and cleanup example at 172-181 to
demonstrate the same cancellation-detached, timeout-bounded cleanup pattern.
- Around line 158-197: The test currently validates only configuration and
direct filesystem access, not CRI-O consumption of the layer and artifact
stores. Extend the Phase 4 and Phase 5 flow around the existing storage.conf and
artifact checks to run runtime workloads that resolve content through each
configured store, and assert successful consumption; otherwise narrow the test
name and final assertions to configuration validation rather than functional
store verification.
In `@test/extended/node/node_mcp_helpers.go`:
- Around line 138-151: Make custom MCP setup transactional in
test/extended/node/node_mcp_helpers.go:138-151 by checking rollback errors and
deleting the created MCP plus removing the node label when creation or
WaitForMCP readiness fails; preserve and return the original setup error while
reporting cleanup failures appropriately. In
test/extended/node/node_utils.go:1091-1092, clean up any non-nil configuration
returned by the setup helper before asserting the error.
In `@test/extended/node/node_utils.go`:
- Around line 1122-1127: Return non-NotFound Kubernetes API errors immediately
instead of converting them into polling timeouts: update the pod lookup callback
in test/extended/node/node_utils.go:1122-1127, preserve node lookup errors in
test/extended/node/node_mcp_helpers.go:201-209, and return non-NotFound pod
lookup errors in test/extended/node/additional_storage_e2e.go:248-250. At
test/extended/node/additional_storage_e2e.go:274-285, handle and propagate the
Events List error rather than discarding it; retain successful NotFound polling
behavior.
- Around line 981-985: Update createTestPod in test/extended/node/node_utils.go
to accept and apply an ImagePullPolicy parameter. In
test/extended/node/additional_storage_e2e.go, pass Never for the
prepopulated-image pod at lines 208-215 and Always for the fallback pod at lines
260-268, ensuring each scenario validates its intended image source.
---
Nitpick comments:
In `@test/extended/node/node_mcp_helpers.go`:
- Around line 75-83: Update the deferred CleanupCustomMCP examples around
CreateCustomMCPForNode to pass a context derived from the existing ctx using
context.WithoutCancel and an explicit timeout, rather than context.Background().
Apply the same bounded, cancellation-detached context pattern to the additional
cleanup example referenced by the comment, preserving deferred cleanup behavior.
In `@test/extended/node/node_utils.go`:
- Around line 1174-1265: Remove the duplicated readiness logic from WaitForMCP
and reuse the existing waitForMCP implementation near the earlier MCP wait code
by exporting or wrapping it. Preserve the current options, timeout, draining
behavior, degraded-pool handling, and readiness semantics through that single
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1dbe9f9f-8f29-447c-8085-45191c385c58
📒 Files selected for processing (3)
test/extended/node/additional_storage_e2e.gotest/extended/node/node_mcp_helpers.gotest/extended/node/node_utils.go
98abffa to
a5cb0a3
Compare
b8a6f32 to
cfcd6f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/extended/node/additional_storage_e2e.go`:
- Around line 86-89: Update the podman pull execution in the additional
image-store setup to use a context-aware helper with a bounded timeout, rather
than the unbounded ExecOnNodeWithChroot call. Ensure registry or network
failures cancel the command and propagate the timeout error through the existing
assertion.
- Around line 82-95: Update the additional image-store setup for testPod1 to use
a uniquely generated local image tag, and pre-populate that tag only in the
additional store. Ensure the tag is not available in CRI-O’s primary storage
before exercising PullNever, while preserving the existing registry-backed image
reference for the fallback phase.
- Around line 51-59: Update the cleanup registration in the test setup so the
ContainerRuntimeConfig deletion and cleanupSingleNodeMCP rollback execute before
the backing-directory cleanup. Because DeferCleanup runs in LIFO order, register
the directory cleanup first or combine both operations in a single cleanup while
preserving the existing error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: dab5fff7-2044-43dc-8578-d8923b7111cb
📒 Files selected for processing (4)
test/extended/node/README.mdtest/extended/node/additional_storage_e2e.gotest/extended/node/node_mcp_helpers.gotest/extended/node/node_utils.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/extended/node/README.md
- test/extended/node/node_mcp_helpers.go
- test/extended/node/node_utils.go
cfcd6f9 to
b96eb5d
Compare
b96eb5d to
df3a6e4
Compare
|
/test all |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 |
|
@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/4608ada0-81df-11f1-93fb-d45cb0af2638-0 |
3 participants
This PR adds E2E testcase for the AdditionalStorageConfig feature (TechPreviewNoUpgrade)
Test Coverage
E2E Test (additional_storage_e2e.go)
1 comprehensive test in openshift/disruptive-longrunning suite:
"should functionally verify all three storage types work together"
Implementation Details
Single-Node MCP Approach
Testing:
Test case passed on local cluster
Summary by CodeRabbit