OCPNODE-4494: e2e test case for RHCOS upgrade from 9 → 10#31393
OCPNODE-4494: e2e test case for RHCOS upgrade from 9 → 10#31393asahay19 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@asahay19: This pull request references OCPNODE-4494 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. |
|
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:
WalkthroughThe upgrade test suite now covers runc guard behavior and the crun happy path for RHCOS 9-to-10 upgrades. Cleanup is parameterized by the active test pool, runtime checks use polling helpers, and documentation describes both scenarios and health assertions. ChangesRuntime upgrade test coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpgradeTest
participant MachineConfigPool
participant WorkerNode
participant ClusterOperator
UpgradeTest->>MachineConfigPool: Create rhel-9 runtime-specific pool
UpgradeTest->>WorkerNode: Apply pool label and verify runtime
UpgradeTest->>MachineConfigPool: Set osImageStream to rhel-10
MachineConfigPool->>WorkerNode: Roll out upgraded configuration
UpgradeTest->>MachineConfigPool: Assert pool is not RenderDegraded
UpgradeTest->>ClusterOperator: Check DegradedPool does not block upgradeability
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
bitoku
left a comment
bitoku
left a comment
There was a problem hiding this comment.
overall lgtm,
One comment about the style.
There's some functions like assert*, wait*, expect*, verify*.
I don't mind either, but want them to be consistent or follow a rule (for example, expect calls Expect in the function, something like that).
| nodeName = "" | ||
| testPoolName = "" | ||
| cleanupCRC = false |
There was a problem hiding this comment.
nit: I don't think they are needed. They should be initialized when it's defined.
There was a problem hiding this comment.
Dropped redundant BeforeEach resets for nodeName / testPoolName / cleanupCRC in the latest commit
|
|
||
| g.By("Upgrading RHCOS version to RHCOS 10 via osImageStream") | ||
| o.Expect(setPoolOSImageStream(ctx, mcClient, crunRHCOS10UpgradePool, streamRHEL10)).To(o.Succeed()) | ||
| o.Expect(waitForMCP(ctx, mcClient, crunRHCOS10UpgradePool, 45*time.Minute)).To(o.Succeed(), |
There was a problem hiding this comment.
nit: I think 45 mins is too long. It's supposed to be finished much earlier.
There was a problem hiding this comment.
Reduced post-rhel-10 MCP and node waits from 45m to 30m
| "pool should roll out to rhel-10 without runc guard RenderDegraded") | ||
|
|
||
| g.By("Verifying node rolled out to RHCOS 10 with crun and pool stayed healthy") | ||
| o.Expect(waitForNodeRHELMajorVersion(ctx, oc, nodeName, "10", 45*time.Minute)).To(o.Succeed(), |
There was a problem hiding this comment.
nit: I think 45 mins is too long. It's supposed to be finished much earlier.
| if upgradeable != nil && | ||
| upgradeable.Status == configv1.ConditionFalse && | ||
| upgradeable.Reason == degradedPoolUpgradeableReason { | ||
| return fmt.Errorf("ClusterOperator %s Upgradeable=False (reason %s) after crun rhel-10 upgrade: %q", |
There was a problem hiding this comment.
Too specific error message, this may be confusing in the future when someone uses this function.
| return fmt.Errorf("ClusterOperator %s Upgradeable=False (reason %s) after crun rhel-10 upgrade: %q", | |
| return fmt.Errorf("ClusterOperator %s Upgradeable=False (reason %s): %q", |
There was a problem hiding this comment.
Generalized assertMachineConfigNotBlockedByDegradedPool error message in the latest commit
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-2of2 |
|
@bitoku: trigger 2 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/07d107d0-8112-11f1-980f-e0e7979d9599-0 |
dfe3764 to
c6aab91
Compare
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-2of2 |
|
@asahay19: 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/bc3277f0-81d4-11f1-91a5-e78e2058153d-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 |
|
@asahay19: 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/cab5adb0-81d4-11f1-9135-6cfa88222756-0 |
|
Scheduling required tests: |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
| func waitForCrunRuntimeOnNode(ctx context.Context, oc *exutil.CLI, nodeName string, timeout time.Duration) error { | ||
| err := wait.PollUntilContextTimeout(ctx, 10*time.Second, timeout, true, func(ctx context.Context) (bool, error) { | ||
| err := assertCrunRuntimeOnNode(ctx, oc, nodeName) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "debug unavailable") { | ||
| framework.Logf("Transient debug error checking crun on node %s: %v", nodeName, err) | ||
| return false, nil | ||
| } | ||
| return false, err | ||
| } | ||
| return true, nil | ||
| }) | ||
| if err != nil { | ||
| if wait.Interrupted(err) { | ||
| return fmt.Errorf("node %s does not have crun as default runtime within %s", nodeName, timeout) | ||
| } | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Comment from Claude. Please check it.
- waitForCrunRuntimeOnNode doesn't actually wait — The function wraps assertCrunRuntimeOnNode in a poll loop, but assertCrunRuntimeOnNode returns an error (not a bool) when runc is still present. The poll treats this as a permanent failure and aborts immediately instead of retrying.
Compare with waitForRuncRuntimeOnNode which correctly calls hasRuncRuntimeOnNode directly and returns (false, nil) to keep polling. Fix: call hasRuncRuntimeOnNode directly and invert the logic, matching the sibling pattern.
There was a problem hiding this comment.
Fixed waitForCrunRuntimeOnNode to poll via hasCrunRuntimeOnNode (same pattern as waitForRuncRuntimeOnNode) in the latest commit
| clusterDefaultStream string | ||
| ) | ||
|
|
||
| g.BeforeEach(func(ctx context.Context) { |
There was a problem hiding this comment.
Restored BeforeEach resets for nodeName / testPoolName / cleanup
| mcp := &machineconfigv1.MachineConfigPool{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: crunRHCOS10UpgradePool, | ||
| Labels: map[string]string{ | ||
| poolOperatorLabel(crunRHCOS10UpgradePool): "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
nit: I guess we can reuse createRuncUpgradeMCP?
There was a problem hiding this comment.
Deduplicated MCP creation into createRHEL9UpgradeMCP, reused by both guard and crun paths
|
Scheduling required tests: |
| clusterDefaultStream string | ||
| ) | ||
|
|
||
| g.BeforeEach(func(ctx context.Context) { |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Scheduling required tests: |
| @@ -807,17 +872,16 @@ func waitForNodeRHELMajorVersion(ctx context.Context, oc *exutil.CLI, nodeName, | |||
| } | |||
|
|
|||
| func assertCrunRuntimeOnNode(ctx context.Context, oc *exutil.CLI, nodeName string) error { | |||
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-2of2 |
|
@bitoku: trigger 2 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/f4665d40-8516-11f1-874e-f8074f7d5322-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-2of2 |
|
@asahay19: 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/ba9ac310-8518-11f1-8407-ff29209175a6-0 |
|
@asahay19: 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/c827d900-8518-11f1-82a8-f93ef54c43b1-0 |
|
/retest |
1 similar comment
|
/retest |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-1of2 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning-techpreview-2of2 |
|
@asahay19: 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/983e8b40-85d5-11f1-8ede-0260f2967932-0 |
|
@asahay19: 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/a1470370-85d5-11f1-9f40-5a94e5ca5011-0 |
|
Scheduling required tests: |
|
/verified by @asahay19 |
openshift-ci-robot
added
the
verified
|
@asahay19: 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, bitoku, cpmeadors 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 |
openshift-ci
Bot
added
the
approved
|
/retest |
|
@asahay19: The following test failed, say
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. |
5 participants
Summary
Adds the crun happy-path e2e test for RHCOS 9 → 10
osImageStreamupgrades, complementing the runc guard test merged in #31266 (UC6A).On OCP 5.0–5.2 clusters with dual OS streams and crun as the default runtime, moving a pool from
rhel-9torhel-10should succeed without triggering the runc-on-RHCOS-10 guard. This test validates that recommended upgrade path.What the test does
MachineConfigPoolpinned torhel-9(no runcContainerRuntimeConfig)rhel-10RenderDegraded/Degradedco/machine-configis notUpgradeable=False(reasonDegradedPool)Changes
test/extended/node/runc_upgrade_cases.go— newIt+ helpers; sharedAfterEachparameterized by pool nametest/extended/node/runc_upgrade_cases.md— documents both UC6A and UC5/UC14Epic: OCPNODE-4013
Locally executed on Openshift cluster 5.0, it got executed successfully:
Summary by CodeRabbit
Tests
crun(RHCOS 9→10).crunhappy-path upgrade case that validates pinned MCP behavior, successful upgrade torhel-10, node reboot onto the target OS image, and post-upgrade health checks (including that cluster upgradeability is not blocked by the isolated degraded-pool reason).Documentation
RenderDegradedversus delayedDegradedbehavior and supported runtime configuration.