NO-ISSUE: Skip Additional Storage tests on HyperShift - MachineConfig API not available - #31489
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@BhargaviGudi: This pull request explicitly references no 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. |
|
/test e2e-aws-ovn-hypershift |
|
/test e2e-hypershift-conformance |
WalkthroughNode tests now use shared HyperShift detection before flows that require the MachineConfig API. Additional storage tests perform platform skips in setup, while feature-gate validation only evaluates ChangesNode test gating
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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 |
|
/assign @saschagrunert @asahay19 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/node/additional_storage_api.go (1)
28-32: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail closed when
Infrastructure.Status.ControlPlaneTopologyis empty.
GetControlPlaneTopology()returns an address to the parsed field, socontrolPlaneTopology != nil, and its address comparison with the string literal remains true when the field is empty. GateAdditionalStorageConfigonly when the value equalsconfigv1.ExternalTopologyMode; otherwise skip or fail so empty topology does not reach MachineConfig-backed tests.🤖 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 28 - 32, Update the topology guard in AdditionalStorageConfig to fail closed when GetControlPlaneTopology returns nil or an empty value, and only allow the test when the value is explicitly configv1.ExternalTopologyMode; ensure empty or unrecognized topology skips or fails before reaching MachineConfig-backed tests.
🤖 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_api.go`:
- Around line 28-29: Update IsAdditionalStorageConfigEnabled to use a
context-aware, bounded topology lookup instead of
exutil.GetControlPlaneTopology, passing its received ctx through the API
request. Add or reuse a helper that performs the infrastructure GET with ctx and
an appropriate timeout, while preserving the existing error assertion and
topology behavior.
---
Nitpick comments:
In `@test/extended/node/additional_storage_api.go`:
- Around line 28-32: Update the topology guard in AdditionalStorageConfig to
fail closed when GetControlPlaneTopology returns nil or an empty value, and only
allow the test when the value is explicitly configv1.ExternalTopologyMode;
ensure empty or unrecognized topology skips or fails before reaching
MachineConfig-backed tests.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c5c3e9c-bbb6-41f2-ba68-cb44975fe452
📒 Files selected for processing (1)
test/extended/node/additional_storage_api.go
|
Scheduling required tests: |
d575362 to
cd2afc1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/node_utils.go`:
- Around line 41-49: Update SkipOnHyperShift and its callers in the BeforeEach
flows to accept and pass context.Context; make GetControlPlaneTopology and its
underlying infrastructure lookup context-aware with a bounded timeout, replacing
context.Background(). Apply the changes in the shared helper plus
additional_storage_api.go and additional_storage_e2e.go, preserving the existing
topology check and skip behavior.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 02476261-2975-48ee-b930-59feb3c7d047
📒 Files selected for processing (3)
test/extended/node/additional_storage_api.gotest/extended/node/additional_storage_e2e.gotest/extended/node/node_utils.go
|
/test e2e-hypershift-conformance |
cd2afc1 to
fd724b9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/test e2e-hypershift-conformance |
|
Scheduling required tests: |
|
/retest |
|
/lgtm |
|
/retest-required |
|
/test e2e-hypershift-techpreview |
|
/test e2e-aws-ovn-hypershift-conformance |
|
/lgtm |
|
/verified by @BhargaviGudi |
|
@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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asahay19, BhargaviGudi, cpmeadors, ngopalak-redhat, 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 |
|
/retest-required |
|
@BhargaviGudi: all tests passed! 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. |
Summary
Add HyperShift detection to Additional Storage tests and consolidate HyperShift skip logic across all node tests into a reusable helper function.
Problem
Additional Storage E2E and API validation tests were failing on HyperShift clusters with:
the server could not find the requested resource (post containerruntimeconfigs.machineconfiguration.openshift.io)
Root cause: HyperShift clusters use External topology mode where the control plane runs outside the cluster. These clusters do not have the MachineConfig API (ContainerRuntimeConfig, MachineConfigPool,
etc.) available.
Changes
1. Added
SkipOnHyperShift()helper innode_utils.goexutil.IsHypershift(ctx, configClient)SkipOnMicroShift()context.Contextto allow API call cancellation2. Updated 5 node test files to use the helper
Replaced inline HyperShift detection with the reusable helper:
additional_storage_api.go- API validation testsadditional_storage_e2e.go- E2E functional testskubeletconfig_tls.go- TLS config testsnode_swap.go- Swap configuration testsrunc_upgrade_cases.go- Runtime upgrade testsBenefits
DRY principle - Eliminates duplicate HyperShift detection code (5 instances → 1 helper)
Context-aware - API calls can be canceled if test context is canceled
Consistent - All node tests use the same skip pattern
Maintainable - Future HyperShift detection changes only need to update one function
Testing
Related
Summary by CodeRabbit