MCO-2321: adapt osimagestream tests to handle clusters defaulting to rhel-10#6099
MCO-2321: adapt osimagestream tests to handle clusters defaulting to rhel-10#6099sergiordlr wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sergiordlr: This pull request references MCO-2321 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. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughRefactors OSImageStream disruption tests to accept explicit initial and target streams, adds FilterExtensionsForStreams, extracts extension survivability into a helper, introduces ensureOsImageStream, and updates osImageURL tests to assert the computed effective stream after recovery. ChangesOSImageStream test parameterization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
sergiordlr
force-pushed
the
adapt_osimagestreams_to_rhel10_default
branch
from
8df19f3 to
7d33897
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/extended-priv/mco_osimagestream.go`:
- Around line 407-409: The defer cleanup() is called before checking err from
GetCompactCompatibleOrCustomPool which can cause a panic if cleanup is nil;
change the flow in the test so you first call
GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1), check
o.Expect(err).NotTo(o.HaveOccurred()) and only after the error assertion call
defer cleanup() (or nil-check cleanup before deferring) so cleanup is only
scheduled when the call succeeded.
- Around line 441-453: FilterExtensions currently returns extensions compatible
only with targetStream (called as FilterExtensions(AllExtenstions, ...,
targetStream)) which can break reverse-stream tests; call FilterExtensions for
both initialStream and targetStream (e.g., compatibleWithInitial :=
FilterExtensions(AllExtenstions, len(armNodes) > 0, fips, initialStream) and
compatibleWithTarget := FilterExtensions(..., targetStream)) and compute the
intersection to produce compatibleExtensions used for logging, for mc.parameters
(MarshalOrFail(compatibleExtensions)) and for creating the MachineConfig
(mc.create()); update the logger messages to reflect the intersection result so
installation uses only extensions supported on both streams.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2df14b85-22b0-442b-a8ec-22babe2f13b3
📒 Files selected for processing (1)
test/extended-priv/mco_osimagestream.go
| mcp, cleanup, err := GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1) | ||
| defer cleanup() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting pool for testing") |
There was a problem hiding this comment.
Guard cleanup defer behind the error check.
defer cleanup() is scheduled before verifying err. If GetCompactCompatibleOrCustomPool fails and returns a nil cleanup function, this can panic on unwind and obscure the real failure.
Suggested fix
mcp, cleanup, err := GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1)
-defer cleanup()
o.Expect(err).NotTo(o.HaveOccurred(), "Error getting pool for testing")
+if cleanup != nil {
+ defer cleanup()
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mcp, cleanup, err := GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1) | |
| defer cleanup() | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting pool for testing") | |
| mcp, cleanup, err := GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1) | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting pool for testing") | |
| if cleanup != nil { | |
| defer cleanup() | |
| } |
🤖 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-priv/mco_osimagestream.go` around lines 407 - 409, The defer
cleanup() is called before checking err from GetCompactCompatibleOrCustomPool
which can cause a panic if cleanup is nil; change the flow in the test so you
first call GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1), check
o.Expect(err).NotTo(o.HaveOccurred()) and only after the error assertion call
defer cleanup() (or nil-check cleanup before deferring) so cleanup is only
scheduled when the call succeeded.
There was a problem hiding this comment.
If the GetCompactCompatibleOrCustomPool returns an error and defer Cleanup is after Expect, then it will never be executed.
There was a problem hiding this comment.
@sergiordlr, you're absolutely right — that's a valid concern. In Ginkgo, o.Expect(err).NotTo(o.HaveOccurred()) triggers Fail() which panics, so any defer placed after it would never be registered if the assertion fires.
The fix that addresses both issues (nil-panic prevention and cleanup-on-failure guarantee) is to nil-check cleanup before the error assertion:
mcp, cleanup, err := GetCompactCompatibleOrCustomPool(oc.AsAdmin(), 1)
if cleanup != nil {
defer cleanup()
}
o.Expect(err).NotTo(o.HaveOccurred(), "Error getting pool for testing")This way:
cleanupis deferred before any panic can occur, so it runs on all exit paths.- The nil guard prevents a panic if
GetCompactCompatibleOrCustomPoolreturns a nil cleanup on error.
Apologies for the incomplete fix in the original suggestion.
🧠 Learnings used
Learnt from: HarshwardhanPatil07
Repo: openshift/machine-config-operator PR: 6034
File: test/extended-priv/mco_security.go:1011-1015
Timestamp: 2026-05-13T07:41:05.710Z
Learning: In the extended-priv Go/Ginkgo test suite, it’s safe to use synchronous `o.Expect` assertions on node state immediately after calling `mc.create()`.
Reason: `MachineConfig.create()` internally waits for the MachineConfigPool to complete (`mcp.waitForComplete()`) before returning unless `mc.skipWaitForMcp` is explicitly set to `true`. It also adjusts wait timeouts when extensions are present (`HasExtensionsSafe()`) or when kernel type changes (`GetKernelTypeSafe()`).
So, for node-state checks placed directly after `mc.create()` (and with `skipWaitForMcp` not enabled), you should not require `o.Eventually` for correctness. If `mc.skipWaitForMcp` is set to `true`, then `o.Eventually` may be needed.
Learnt from: ptalgulk01
Repo: openshift/machine-config-operator PR: 6081
File: test/extended-priv/mco_layering.go:932-1078
Timestamp: 2026-05-25T15:37:54.265Z
Learning: In this repository, for migration PRs that port existing tests from openshift-tests-private into this codebase (e.g., OS layering tests landing under test/extended-priv), keep the PR strictly focused on migrating the test logic. Do not add new conditional guards, skip logic, or supporting infrastructure/checks (for example, MicroShift-related skip checks) as part of the migration—those should be handled in separate follow-up PRs.
|
Actionable comments posted: 0 |
sergiordlr
force-pushed
the
adapt_osimagestreams_to_rhel10_default
branch
from
7d33897 to
511442a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/extended-priv/machineconfigpool.go (1)
1496-1502: ⚡ Quick winGuard
FilterExtensionsForStreamsagainst emptyosImageStreams(panic onosImageStreams[len-1])
FilterExtensionsForStreamsunconditionally indexesosImageStreams[len(osImageStreams)-1], which would panic if ever called with zero streams. Current call sites pass at least two streams (initialStream, targetStream), so this path isn’t hit today; add alen(osImageStreams)==0early return if the helper can be reused with no args.🤖 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-priv/machineconfigpool.go` around lines 1496 - 1502, The function FilterExtensionsForStreams can panic when osImageStreams is empty because it indexes osImageStreams[len(osImageStreams)-1]; add an early guard at the top of FilterExtensionsForStreams that checks if len(osImageStreams) == 0 and returns the unmodified extensions plus nil (or empty) second and third return slices (e.g., return filtered, nil, nil) to avoid the panic and preserve current semantics.
🤖 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-priv/machineconfigpool.go`:
- Around line 1496-1502: The function FilterExtensionsForStreams can panic when
osImageStreams is empty because it indexes
osImageStreams[len(osImageStreams)-1]; add an early guard at the top of
FilterExtensionsForStreams that checks if len(osImageStreams) == 0 and returns
the unmodified extensions plus nil (or empty) second and third return slices
(e.g., return filtered, nil, nil) to avoid the panic and preserve current
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0ba31eb-7f1a-4b27-af5d-18902d7a8d6a
📒 Files selected for processing (2)
test/extended-priv/machineconfigpool.gotest/extended-priv/mco_osimagestream.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended-priv/mco_osimagestream.go
sergiordlr
force-pushed
the
adapt_osimagestreams_to_rhel10_default
branch
from
511442a to
3ff2e9e
Compare
|
Actionable comments posted: 0 |
sergiordlr
force-pushed
the
adapt_osimagestreams_to_rhel10_default
branch
from
3ff2e9e to
0fc1614
Compare
|
/payload-job periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-1of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-2of3 periodic-ci-openshift-machine-config-operator-release-5.0-periodics-e2e-aws-mco-disruptive-techpreview-3of3 |
|
@sergiordlr: trigger 3 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/85b7ce90-5aa7-11f1-9d70-b47ce184751f-0 |
|
I'm fine with the approach, but I was expecting the initial/target streams to be computed with the initial state of the cluster and not hard-coded. |
|
/retest |
| exutil.By(fmt.Sprintf("Verify %s and %s streams are available", initialStream, targetStream)) | ||
| o.Eventually(osis.GetAvailableStreamNames, "2m", "20s").Should(o.ContainElements(initialStream, targetStream), | ||
| "Both streams '%s' and '%s' should be available", initialStream, targetStream) | ||
| logger.Infof("Both %s and %s streams are available", initialStream, targetStream) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
How about creating a func for verifying available stream, since in my PR and existing TC I think we check is stream is present or not
There was a problem hiding this comment.
This is only one instruction and a debug message. I would prefer not to create a function with one instruction and a debug log.
| exutil.By(fmt.Sprintf("Verify %s and %s streams are available", initialStream, targetStream)) | ||
| o.Eventually(osis.GetAvailableStreamNames, "2m", "20s").Should(o.ContainElements(initialStream, targetStream), | ||
| "Both streams '%s' and '%s' should be available", initialStream, targetStream) | ||
| logger.Infof("Both %s and %s streams are available", initialStream, targetStream) | ||
| logger.Infof("OK!\n") |
| exutil.By(fmt.Sprintf("Check current osImageStream and configure %s if necessary", initialStream)) | ||
| currentStream, err := GetEffectiveOsImageStream(mcp) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting effective osImageStream from %s", mcp) | ||
| logger.Infof("Current stream: '%s'", currentStream) | ||
|
|
||
| if currentStream != initialStream { | ||
| logger.Infof("Configuring osImageStream to '%s' in %s", initialStream, mcp) | ||
| o.Expect(mcp.SetOsImageStream(initialStream)).To(o.Succeed(), "Error setting osImageStream to '%s' in %s", initialStream, mcp) | ||
| mcp.waitForComplete() | ||
| logger.Infof("OK!\n") | ||
| } else { | ||
| logger.Infof("%s is already using %s stream", mcp, initialStream) | ||
| logger.Infof("OK!\n") | ||
| } |
There was a problem hiding this comment.
should we create func for this since I see it is repeating in testKernelTypeAcrossOSImageStreams() func too
There was a problem hiding this comment.
I added a new function to encapsulate this code.
Thank you!!
sergiordlr
changed the title
sergiordlr
force-pushed
the
adapt_osimagestreams_to_rhel10_default
branch
from
0fc1614 to
2055d5a
Compare
|
/retest |
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01, sergiordlr 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 |
|
/test unit |
|
@sergiordlr: 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. |
4 participants
- What I did
Modify the current osImageStream tests so that they are parameterized with the "initialStream" and the "targetStream".
With this modification, once OCP 5.0 defaults to rhel-10, the only change should be to configure the expected default stream in DefaultOSImageStream in the const.go and to swap the initial and target streams.
Current test cases are bound to change from rhel-9 to rhel-10. Should we create new test cases to test rhel10 to rhel-9? or should we just configure a generic test switching from the defaultstream to the non-default stream?
Generic test:
- How to verify it
Tests should pass
Summary by CodeRabbit