OCPBUGS-105802: Use the correct v1 mcfg api for OSImageStream during ignition - #9283
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bennerv: This pull request references Jira Issue OCPBUGS-105802, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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/invalid-bug
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe controller now generates Sequence Diagram(s)sequenceDiagram
participant LifecycleTest
participant NodePool
participant GuestClusterNodes
LifecycleTest->>NodePool: Resolve OSImageStream
LifecycleTest->>NodePool: Wait for expected ready replicas
LifecycleTest->>GuestClusterNodes: List hosted-cluster nodes
GuestClusterNodes-->>LifecycleTest: Return RHCOS image strings
LifecycleTest->>LifecycleTest: Parse and compare RHCOS major versions
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@bennerv: This pull request references Jira Issue OCPBUGS-105802, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
@bennerv: This pull request references Jira Issue OCPBUGS-105802, which is valid. 3 validation(s) were run on this bug
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. |
352a528 to
fe3bf07
Compare
|
/hold for personal review. |
openshift-ci
Bot
added
do-not-merge/hold
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/e2e/v2/tests/nodepool_osimagestream_test.go`:
- Around line 326-330: Update the comment for verifyNodeOSMatchesStream to
replace “guest cluster” with “hosted cluster,” preserving the rest of the
explanation and required V2 terminology.
- Around line 365-366: Rename the Ginkgo test description inside
NodePoolOSImageStreamNodeOSVerificationTest to follow the required “When ... it
should ...” format, while preserving its existing intent about nodes matching
the resolved osImageStream OS version.
- Around line 307-310: In the test flow surrounding verifyNodeOSMatchesStream,
call ValidateHostedClusterClient() after ValidateHostedCluster() and before
verifying node OS images. Keep the existing node validation and expectedStream
checks unchanged.
🪄 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: Enterprise
Run ID: 612402ec-6f14-4892-941f-429e0988239a
📒 Files selected for processing (1)
test/e2e/v2/tests/nodepool_osimagestream_test.go
fe3bf07 to
2ff2425
Compare
csrwng
added
the
approved
|
/test security |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9283 +/- ##
=======================================
Coverage 45.73% 45.73%
=======================================
Files 781 781
Lines 97837 97837
=======================================
Hits 44744 44744
Misses 50024 50024
Partials 3069 3069
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Reason for the hold:
need to investigate more. |
|
/cc |
|
This needs rebased to pick up the changes from #9229, the current code won't compile... |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments on the e2e test. Core fix LGTM — correct and minimal. Thanks!
| resolvedStream := pool.Status.OSImageStream.Name | ||
| if resolvedStream == "" { | ||
| resolvedStream = pool.Spec.OSImageStream.Name | ||
| } | ||
| if resolvedStream == "" { | ||
| resolvedStream = hyperv1.OSImageStreamRHEL10 | ||
| } |
There was a problem hiding this comment.
The resolution order here is inverted from what the controller does in GetRHELStreamForBootImage — the controller checks Spec first (user's explicit choice wins), then Status (preserve what's running), then derives from version. This checks Status first, then Spec, then hardcodes RHEL10.
This produces wrong expectations in at least two concrete scenarios:
- User sets spec=rhel-10, status=rhel-9: controller resolves rhel-10, but this test would verify against rhel-9
- Both empty on OCP 4.x: controller resolves rhel-9 (version-derived), but this assumes rhel-10
I'd suggest either calling the controller's resolution function directly, or failing explicitly if the stream can't be determined:
resolvedStream := pool.Spec.OSImageStream.Name
if resolvedStream == "" {
resolvedStream = pool.Status.OSImageStream.Name
}
Expect(resolvedStream).NotTo(BeEmpty(),
"could not determine osImageStream from NodePool %s spec or status", pool.Name)|
|
||
| pool := &hyperv1.NodePool{} | ||
| Expect(testCtx.MgmtClient.Get(ctx, crclient.ObjectKeyFromObject(defaultNP), pool)). | ||
| To(Succeed(), "failed to get NodePool %s", defaultNP.Name) |
There was a problem hiding this comment.
Unlike NodePoolOSImageStreamDefaultStatusTest above — which polls with osImageStreamSetPredicate() before reading status — this does a single MgmtClient.Get() without waiting. If the controller hasn't reconciled yet, status.osImageStream.Name is empty and the fallback chain kicks in with wrong assumptions.
Adding an EventuallyObject wait for status.osImageStream.Name to be non-empty before reading it would make this reliable — same pattern already used in the test above.
| default: | ||
| return "9" | ||
| } |
There was a problem hiding this comment.
nit: this silently returns "9" for any unknown stream. Per the controller's resolution table, unknown streams produce an error — they shouldn't map to a valid version. A Fail() here would catch future stream additions that aren't handled:
default:
Fail(fmt.Sprintf("unexpected osImageStream: %s", stream))
return ""
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some more comments. Thanks!
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some more comments. Thanks!
| @@ -49,6 +50,7 @@ func RegisterNodePoolOSImageStreamLifecycleTests(getTestCtx internal.TestContext | |||
| NodePoolOSImageStreamRHEL10RejectionTest(getTestCtx) | |||
| NodePoolOSImageStreamRHEL10RuncRejectionTest(getTestCtx) | |||
| NodePoolOSImageStreamExplicitDefaultNoRolloutTest(getTestCtx) | |||
There was a problem hiding this comment.
This will fail deterministically. ExplicitDefaultNoRolloutTest is registered at line 51 and patches spec.osImageStream on the default NodePool. Since the field is immutable (CEL rejects removal), by the time NodeOSVerificationTest runs, Expect(defaultNP.Spec.OSImageStream.Name).To(BeEmpty()) will always fail.
Two options: (1) register NodeOSVerificationTest before ExplicitDefaultNoRolloutTest, or (2) remove the BeEmpty() assertion and handle the case where spec is already set — read whatever stream is there (spec or status) and verify against that.
There was a problem hiding this comment.
I put it at the top, if it belongs lower down let me know.
| fmt.Sprintf("NodePool %s/%s to have ready nodes", np.Namespace, np.Name), | ||
| func(pollCtx context.Context) (*hyperv1.NodePool, error) { | ||
| pool := &hyperv1.NodePool{} | ||
| err := testCtx.MgmtClient.Get(pollCtx, crclient.ObjectKeyFromObject(np), pool) |
There was a problem hiding this comment.
This only checks the first node. For a multi-replica NodePool, a partial OS mismatch on other nodes would go undetected. All nodes share the same ignition config so the risk is low, but iterating all items costs almost nothing:
for _, node := range nodeList.Items {
// ... regex match and assert
}There was a problem hiding this comment.
Updated. Also had to add a new predicate for all nodes ready since the nodesInfoPopulatedPredicate only checks for a single node. I'm not sure when the osInfo on the node's status is populated, but just in case we wait until they're ready.
| switch stream { | ||
| case hyperv1.OSImageStreamRHEL10: | ||
| return "10", nil | ||
| case hyperv1.OSImageStreamRHEL9: |
There was a problem hiding this comment.
Missing GinkgoHelper(). Without it, test failures inside this function report line numbers here rather than at the caller. Other helpers in this file (buildTestNodePool, getDefaultNodePool, cleanupNodePool) all use it.
There was a problem hiding this comment.
I'm confused, the function returns error and makes callers deal with it, GinkgoHelper would only apply if the function tried to do things like Expect internally (which this one doesn't), maybe you meant to comment elsewhere?
There was a problem hiding this comment.
I added it (since there's no harm in adding it), but agree with @ironcladlou .
I also put it in It shouldn't be in that function. Ignore thatverifyNodeOSMatchesStream which has ginkgo assertions. If it doesn't belong there, let me know.
There was a problem hiding this comment.
@bennerv @jparrill be aware that calling GinkgoHelper() has its own side effects and assumptions which can lead to surprising behavior. If you call it and then call e.g. Expect(), the assertion happens in the Default context, which means if you use that helper within an Eventually() it's not going to use the correct nested assertor instance (which is provided in the Eventually closure) and will lead to subtle issues. Although GitHub is making it impossible to view now, we had a lot of discussion about approaches to deal with this on #9229 which originally introduced some helpers along these lines, but coming up with a friendly UX needed more consensus building and so I just punted.
All that is to say I wouldn't just add it without at least a comment that the helper should not be used within an Eventually() (for example)...
Eventually(func(g Gomega) { // inside here, your helper needs to call g.Expect(), not the implicit Default Gomega)There was a problem hiding this comment.
@ironcladlou I wasn't aware of the implictaions, apologies and thanks for the explanation. If you're good with it (since the function errors are meant to be handled by the caller), I removed it from the helper function.
There was a problem hiding this comment.
no problem and thanks, I wasn't familiar with the nuances either until that PR, just wanted to share what I learned to prevent any surprises
The MCO removed v1alpha1 support for OSImageStream in openshift/machine-config-operator#6076. Update the ignition server to render the CR with the v1 API version so the MCO bootstrap processes it correctly.
27acbdc to
2365d9d
Compare
|
/test e2e-v2-aws-techpreview-osimagestream |
…dePools Expand NodePoolOSImageStreamNodeOSVerificationTest to create dedicated NodePools for each osImageStream value and verify nodes boot with the correct RHCOS version: 1. Default NodePool (no osImageStream set) — validates implicit default 2. New 1-replica NodePool with osImageStream=rhel-9 3. New 1-replica NodePool with osImageStream=rhel-10 Move the test from status (read-only) to lifecycle registration since it now creates additional NodePools. Also fix helper signatures to match current TestContext API (GetHostedCluster returns error, GetHostedClusterClient requires hc argument). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2365d9d to
a39839a
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: bennerv 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 |
|
/test e2e-v2-aws-techpreview-osimagestream |
|
/lgtm |
|
/verified by e2e passing |
|
Scheduling tests matching the |
openshift-ci-robot
added
the
verified
|
@jparrill: 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 |
Test Resultse2e-aws
e2e-aks
Failed TestsTotal failed tests: 3
|
|
The tests are broken on version skew issues. |
|
@sdodson: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
@sdodson: Overrode contexts on behalf of sdodson: ci/prow/e2e-aks, ci/prow/e2e-v2-azure-self-managed 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 kubernetes-sigs/prow repository. |
|
@bennerv: 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. |
|
@bennerv: Jira Issue Verification Checks: Jira Issue OCPBUGS-105802 Jira Issue OCPBUGS-105802 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
|
@sdodson: new pull request created: #9319 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 kubernetes-sigs/prow repository. |
8 participants
What this PR does / why we need it:
fixes the selection of rhel-9 vs. rhel-10 in OCP 5.0 by using the api that machineconfig understands for OS Stream.
Which issue(s) this PR fixes:
Fixes OCPBUGS-105802
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
machineconfiguration.openshift.io/v1API version.