CNTRLPLANE-3032: test: add e2e v2 tests for NodePool OSImageStream#9033
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/testing
|
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:
📝 WalkthroughWalkthroughAdds legacy and e2e v2 tests for Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant ManagementCluster
participant NodePoolController
participant HostedClusterGuestClient
E2ETest->>ManagementCluster: Create NodePool with osImageStream
ManagementCluster->>NodePoolController: Reconcile NodePool
NodePoolController-->>ManagementCluster: Update ValidMachineConfig and status
E2ETest->>HostedClusterGuestClient: Wait for node readiness
HostedClusterGuestClient-->>E2ETest: Return ready nodes
E2ETest->>ManagementCluster: Read status.osImageStream.name
Possibly related PRs
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
test/e2e/v2/tests/nodepool_osimagestream_test.go (2)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a plain
Describename and retain the Ginkgo label.Remove the legacy string annotations; v2 tests use
Label(...)for filtering.Suggested change
-var _ = Describe("[sig-hypershift][Jira:Hypershift][Feature:NodePoolOSImageStream] NodePool OSImageStream", Label("lifecycle", "nodepool-osimagestream"), func() { +var _ = Describe("NodePool OSImageStream", Label("lifecycle", "nodepool-osimagestream"), func() {Based on learnings: v2 Ginkgo
Describenames should remain plain and filtering should use Ginkgo labels.🤖 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/e2e/v2/tests/nodepool_osimagestream_test.go` at line 42, Update the NodePool OSImageStream test’s Describe declaration to use a plain descriptive name without bracketed legacy annotations, while retaining the existing Label("lifecycle", "nodepool-osimagestream") for filtering.Source: Learnings
96-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the suite context in polling callbacks.
Rename each callback parameter (for example,
pollCtx) so it does not hide the outer test context.
test/e2e/v2/tests/nodepool_osimagestream_test.go#L96-L99: rename callbackctxand use it forMgmtClient.Get.test/e2e/v2/tests/nodepool_osimagestream_test.go#L170-L173: rename callbackctxand use it forMgmtClient.Get.test/e2e/v2/tests/nodepool_osimagestream_test.go#L192-L195: rename callbackctxand use it forMgmtClient.Get.test/e2e/v2/tests/nodepool_osimagestream_test.go#L233-L236: rename callbackctxand use it forMgmtClient.Get.As per coding guidelines: “Avoid variable shadowing.”
🤖 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/e2e/v2/tests/nodepool_osimagestream_test.go` around lines 96 - 99, Rename the polling callback parameter from ctx to a distinct name such as pollCtx at all four sites in test/e2e/v2/tests/nodepool_osimagestream_test.go (lines 96-99, 170-173, 192-195, and 233-236), and pass the renamed parameter to testCtx.MgmtClient.Get so the outer suite context is not shadowed.Source: Coding guidelines
🤖 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/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Around line 227-249: Before waiting for the default NodePool status in the
test, verify that its OS image stream is unset and skip the test when it is
explicitly configured, preserving the requirement that this scenario exercises
default resolution. Add no mutation or setup to create the prerequisite; use the
existing defaultNP object and test framework skip mechanism around the NodePool
status assertion.
---
Nitpick comments:
In `@test/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Line 42: Update the NodePool OSImageStream test’s Describe declaration to use
a plain descriptive name without bracketed legacy annotations, while retaining
the existing Label("lifecycle", "nodepool-osimagestream") for filtering.
- Around line 96-99: Rename the polling callback parameter from ctx to a
distinct name such as pollCtx at all four sites in
test/e2e/v2/tests/nodepool_osimagestream_test.go (lines 96-99, 170-173, 192-195,
and 233-236), and pass the renamed parameter to testCtx.MgmtClient.Get so the
outer suite context is not shadowed.
🪄 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: 1dab87aa-99bc-497d-95c5-7dfa5c6e2642
📒 Files selected for processing (1)
test/e2e/v2/tests/nodepool_osimagestream_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9033 +/- ##
==========================================
+ Coverage 44.44% 44.51% +0.06%
==========================================
Files 774 774
Lines 96977 96988 +11
==========================================
+ Hits 43105 43170 +65
+ Misses 50897 50830 -67
- Partials 2975 2988 +13
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/test e2e-v2-aws |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/e2e/nodepool_osimagestream_test.go`:
- Around line 84-90: Ensure all OSImageStream-related subtests, including the
default-status scenario, skip when the management cluster rejects the
osImageStream field instead of polling and failing. Reuse the existing error
detection around NodePool creation or perform a single capability check before
launching the three subtests, preserving the current skip message and failure
handling.
- Around line 28-29: Remove t.Parallel() from the affected subtests so they
execute before the parent test’s deferred cancel() invalidates the shared ctx.
Add the existing OSStreams feature-gate skip to testOSImageStreamDefaultStatus,
matching the skip behavior used by the other OSImageStream tests.
🪄 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: a6941345-647c-47c1-b985-4db905844830
📒 Files selected for processing (1)
test/e2e/nodepool_osimagestream_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/v2/tests/nodepool_osimagestream_test.go (1)
230-231: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winVerify that the default NodePool is actually unset before asserting default resolution.
The test claims "When no osImageStream is set" but does not verify this precondition. If the default NodePool explicitly sets
osImageStream=rhel-9, the test passes without exercising default resolution. As per path instructions, non-lifecycle tests must skip when required preconditions are absent rather than proceeding.Proposed fix
defaultNP := getDefaultNodePool(ctx, testCtx.MgmtClient, hc) Expect(defaultNP).NotTo(BeNil(), "default NodePool should exist") + if defaultNP.Spec.OSImageStream.Name != "" { + Skip("default NodePool explicitly sets osImageStream=" + defaultNP.Spec.OSImageStream.Name) + } // The default OS stream is currently hardcoded to rhel-9 for all OCP versions.🤖 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/e2e/v2/tests/nodepool_osimagestream_test.go` around lines 230 - 231, In the test setup around getDefaultNodePool, verify that the default NodePool’s osImageStream is unset before asserting default resolution. If the required precondition is absent, skip the non-lifecycle test rather than continuing; preserve the existing NotTo(BeNil()) assertion for confirming the NodePool exists.Source: Path instructions
🤖 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.
Duplicate comments:
In `@test/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Around line 230-231: In the test setup around getDefaultNodePool, verify that
the default NodePool’s osImageStream is unset before asserting default
resolution. If the required precondition is absent, skip the non-lifecycle test
rather than continuing; preserve the existing NotTo(BeNil()) assertion for
confirming the NodePool exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: be4d0bf2-d7b7-4408-add1-09f5e33242e2
📒 Files selected for processing (1)
test/e2e/v2/tests/nodepool_osimagestream_test.go
1f644b4 to
736fc8d
Compare
sdminonne
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/e2e/nodepool_osimagestream_test.go`:
- Around line 24-26: Remove the suite-level OCP version skip and add the same
guard at the start of testOSImageStreamRHEL10Rejection. Keep the RHEL 9 and
default-status tests running on OCP 5.0+, while skipping only the RHEL 10
rejection scenario there.
🪄 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: f0c39885-fc16-48b9-8b0f-d354c4b8f08e
📒 Files selected for processing (2)
test/e2e/nodepool_osimagestream_test.gotest/e2e/v2/tests/nodepool_osimagestream_test.go
736fc8d to
92487fa
Compare
|
/test e2e-v2-aws |
|
/test e2e-aws-4-22 |
2da2fa6 to
9853b06
Compare
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
Depends on openshift/machine-config-operator#6308
Do we need both the v1 and v2 versions of this test? The v2 framework is the direction we're going, and maintaining both means ~90% duplicated logic. If this is net-new test code (not a migration of an existing v1 test), it might make more sense to only add the v2 version.
| crclient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| ) | ||
|
|
||
| func TestNodePoolOSImageStream(t *testing.T) { |
There was a problem hiding this comment.
This creates a standalone HostedCluster just for these 3 subtests. Most NodePool tests are registered as NodePoolTestCase inside TestNodePool (in nodepool_test.go), which shares one cluster across ~15 subtests and amortizes the ~15-20 min creation cost. Since these tests create their own NodePools with specific replicas (0 or 1), they'd fit naturally as NodePoolTestCase entries there.
The other concern is that HasFieldInCRDSchema runs inside the NewHypershiftTest callback — i.e., after the cluster is already created. If OSStreams isn't enabled, you pay the full cluster creation time just to skip.
| t.Run("TestOSImageStreamRHEL10Rejection", func(t *testing.T) { | ||
| testOSImageStreamRHEL10Rejection(t, ctx, mgtClient, hostedCluster, *defaultNodePool) | ||
| }) | ||
|
|
There was a problem hiding this comment.
nit: the v2 It(...) blocks already use Ginkgo-style names, but these v1 subtests use PascalCase. For consistency with the project convention, something like "When osImageStream is set to rhel-10 on OCP < 5.0, it should set ValidMachineConfig to False" would be more descriptive and match the v2 wording.
| }, | ||
| e2eutil.WithTimeout(5*time.Minute), | ||
| e2eutil.WithInterval(10*time.Second), | ||
| ) |
There was a problem hiding this comment.
This re-fetches the NodePool and manually iterates conditions to check the message, but EventuallyObject above already confirmed ValidMachineConfig=False with ValidationFailed. Could the message check be a second predicate in the same EventuallyObject call instead?
| pool := &hyperv1.NodePool{} | ||
| err := mgtClient.Get(ctx, crclient.ObjectKeyFromObject(np), pool) | ||
| return pool, err | ||
| }, |
There was a problem hiding this comment.
This predicate appears 4 times across v1 and v2 (here, line 204, and twice in the v2 file). It follows the same shape as ConditionPredicate in e2eutil — might be worth extracting an OSImageStreamPredicate(expected string) helper there so all four call sites become one-liners.
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
Addressing the two remaining CodeRabbit nitpicks from the review: ctx shadowing in polling callbacks — Fixed: renamed all 4 callback parameters from
|
9853b06 to
c7eb5c7
Compare
|
@jparrill thanks for the thorough review. All points addressed:
|
|
/test e2e-azure-v2-self-managed |
|
/test e2e-kubevirt-aws-ovn-reduced |
|
/test e2e-aws-4-22 |
|
/test e2e-v2-gke |
|
/test e2e-aws |
|
/test e2e-aws-upgrade-hypershift-operator |
The NodePoolOSImageStreamDefaultStatusTest was failing on AWS because status.osImageStream was never populated within the 5-minute timeout. The controller infers status.osImageStream from CAPI Machine NodeInfo.OSImage, which requires CAPI to have copied NodeInfo from the guest cluster nodes — a step that can take longer than 5 minutes. Changes: - Add a nodesInfo pre-condition wait before asserting on status.osImageStream. This confirms CAPI Machines have NodeInfo populated (the same prerequisite the controller needs) and gives clear diagnostic output when it is not. - Add status.osImageStream CRD schema check alongside the existing spec.osImageStream check, so tests skip with a clear message if the feature gate is not active for the status field. - Increase timeout from 5m to 10m and interval from 10s to 15s for both the status test and verifyOSImageStreamAfterUpgrade. - Remove the GCP platform skip now that the nodesInfo pre-condition provides proper diagnostics on all platforms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/test e2e-v2-aws |
|
/test e2e-v2-gke |
OCP 5.0 changed the RHCOS NodeInfo.OSImage format from the legacy 3-digit scheme (e.g. "Red Hat Enterprise Linux CoreOS 419.97.202505081234-0") to a RHEL major.minor.date scheme (e.g. "Red Hat Enterprise Linux CoreOS 9.8.20260721-0"). The regex only matched the legacy format, causing status.osImageStream to never be populated on OCP 5.0 clusters. Update the regex to match both formats and add unit test coverage for the new version strings (RHEL 9, RHEL 10, unknown major). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
Bot
added
the
area/hypershift-operator
|
/test e2e-v2-aws |
|
/test e2e-v2-gke |
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
|
/test e2e-aks-4-22 |
|
@sdminonne: 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. |
|
/verified by unit-tests |
openshift-ci-robot
added
the
verified
|
@sdminonne: 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. |
|
/hold cancel After the conversation in slack, we can say current test are hardcoding RHEL-9 in all cases, so this is safe to be merged. The PR LGTM, PTAL when you have some time @enxebre 🙏 |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
My understanding and the rationale behind this is: When openshift/machine-config-operator#6308 will be merged tests in this PR will keep pass since we hardcode rhel9. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, jparrill, sdminonne 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 |
6 participants
Summary
Adds Ginkgo v2 lifecycle tests for the dual-stream RHEL NodePool feature (
OSStreamsfeature gate) intest/e2e/v2/tests/nodepool_osimagestream_test.go.Test scenarios
rhel-10rejected on OCP < 5.0 (ValidMachineConfig=False, reasonValidationFailed)rhel-10with runcContainerRuntimeConfigrejected on OCP >= 5.0 (ValidMachineConfig=False, reasonValidationFailed, message mentions runc incompatibility)osImageStreamset) reportsrhel-9in statusosImageStreamassertion after upgrade inNodePoolReplaceUpgradeTestandNodePoolInPlaceUpgradeTestFeature gate handling
Uses
e2eutil.HasFieldInCRDSchema()to check ifspec.osImageStreamexists in the NodePool CRD schema before any test runs. When theOSStreamsfeature gate is disabled (Default feature set), the+openshift:enable:FeatureGate=OSStreamsmarker strips the field from the CRD entirely, so the check returnsfalseand all tests skip cleanly. When the gate is enabled (TechPreviewNoUpgrade), the field is present and tests proceed.The upgrade test assertion (
verifyOSImageStreamAfterUpgrade) also checks the CRD schema and skips the assertion gracefully when OSStreams is not enabled, without failing the upgrade test itself.Changes
test/e2e/v2/tests/nodepool_osimagestream_test.go: three test cases registered viaRegisterNodePoolOSImageStreamTeststest/e2e/v2/tests/nodepool_lifecycle_test.go:verifyOSImageStreamAfterUpgradehelper added after upgrade completes in both Replace and InPlace upgrade teststest/e2e/util/eventually.go: extractOSImageStreamPredicatehelper for reuse across testsRelated PRs
ExternalTopologyModeguard so the MCO bootstrap processes theOSImageStreamCR that HyperShift writes. Required for the full end-to-end rhel-10 pipeline to work.OSStreamsfeature gate enabled.Test plan
make e2ev2compiles without errorsgo vetpassesmake verifypasses--ginkgo.label-filter="nodepool-osimagestream"e2e-v2-awsCI job)🤖 Generated with Claude Code