OCPNODE-4623: e2e for Block runc on RHEL 10 via OSImageURL stream class inspection - #31433
OCPNODE-4623: e2e for Block runc on RHEL 10 via OSImageURL stream class inspection#31433asahay19 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-4623 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. |
WalkthroughThe test suite adds RHEL 9→10 osImageURL upgrade-guard coverage when runc is the default runtime, including resource setup, RenderDegraded and Upgradeable assertions, node-state checks, cleanup, and documentation. ChangesRunc osImageURL upgrade guard
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RuncUpgradeTest
participant MachineConfigPool
participant ClusterVersionOperator
participant Node
RuncUpgradeTest->>MachineConfigPool: apply RHEL 10 osImageURL override
MachineConfigPool-->>RuncUpgradeTest: report osImageURL+runc RenderDegraded guard
RuncUpgradeTest->>ClusterVersionOperator: inspect Upgradeable condition
ClusterVersionOperator-->>RuncUpgradeTest: report upgrade blocked
RuncUpgradeTest->>Node: inspect readiness, OS version, and runtime
Node-->>RuncUpgradeTest: remain ready on RHCOS 9 with runc
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asahay19 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/node/runc_upgrade_cases.go (1)
298-316: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCleanup drain-wait will short-circuit for this test because the pool is still degraded.
This test (unlike UC6A) never recovers the pool to a healthy state before the
Itends — it's leftRenderDegraded/Upgradeable=Falseby design. After deleting the CRC/MCs here, the shared drain wait at Line 314 callsWaitForMCP(..., WaitMCPWithMachineCount(0))withoutWaitMCPAllowDegraded(). PerWaitForMCP's contract, it returns an error on the first poll that still observesdegraded/renderDegraded == true, before ever checking whether the pool actually drained — so for this test the "wait for machine count 0" step will effectively no-op (logged, not asserted) instead of confirming a real drain beforedeleteMachineConfigPoolruns.🔧 Proposed fix
if nodeName != "" && testPoolName != "" { - if err := WaitForMCP(ctx, mcClient, testPoolName, 10*time.Minute, WaitMCPWithMachineCount(0)); err != nil { + if err := WaitForMCP(ctx, mcClient, testPoolName, 10*time.Minute, WaitMCPWithMachineCount(0), WaitMCPAllowDegraded()); err != nil { framework.Logf("cleanup: failed waiting for MCP %s machine count 0: %v", testPoolName, err) }🤖 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/runc_upgrade_cases.go` around lines 298 - 316, Update the cleanup drain wait in the nodeName/testPoolName block to call WaitForMCP with WaitMCPAllowDegraded() alongside WaitMCPWithMachineCount(0), allowing the test’s intentionally degraded pool to be polled until its machine count reaches zero before deleteMachineConfigPool proceeds.
🧹 Nitpick comments (1)
test/extended/node/runc_upgrade_cases.go (1)
593-598: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
createRuncURLGuardPoolhelper.The URL-guard test creates the pool via
createOSImageURLUpgradeMCPand the CRC viacreateRuncGuardCRCdirectly, and there are no other calls to this helper. Keeping unused helpers makes the test harder to follow.🤖 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/runc_upgrade_cases.go` around lines 593 - 598, Remove the unused createRuncURLGuardPool helper. Keep the URL-guard test flow using createOSImageURLUpgradeMCP and createRuncGuardCRC directly, since there are no callers that require the wrapper.
🤖 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.
Outside diff comments:
In `@test/extended/node/runc_upgrade_cases.go`:
- Around line 298-316: Update the cleanup drain wait in the
nodeName/testPoolName block to call WaitForMCP with WaitMCPAllowDegraded()
alongside WaitMCPWithMachineCount(0), allowing the test’s intentionally degraded
pool to be polled until its machine count reaches zero before
deleteMachineConfigPool proceeds.
---
Nitpick comments:
In `@test/extended/node/runc_upgrade_cases.go`:
- Around line 593-598: Remove the unused createRuncURLGuardPool helper. Keep the
URL-guard test flow using createOSImageURLUpgradeMCP and createRuncGuardCRC
directly, since there are no callers that require the wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cb03e40-faa0-4c10-ab7c-2c39016aa8d6
📒 Files selected for processing (2)
test/extended/node/runc_upgrade_cases.gotest/extended/node/runc_upgrade_cases.md
openshift-ci
Bot
added
the
ready-for-human-review
|
Scheduling required tests: |
|
@asahay19: The following tests 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: d0144a6
New tests seen in this PR at sha: d0144a6
|
| degradedPoolUpgradeableMessage = "One or more machine config pools are degraded" | ||
|
|
||
| // MCO rejects MachineConfig osImageURL override when MCP spec.osImageStream is also set. | ||
| osImageURLStreamConflictMessage = "cannot override MachineConfig osImageURL and set MachineConfigPool spec.osImageStream.name simultaneously" |
There was a problem hiding this comment.
we don't need to check this, just checking osImageURLRuncRHEL10GuardMessage is enough
| useRHEL9BaselineMC := clusterDefaultStream == streamRHEL10 | ||
| if useRHEL9BaselineMC { | ||
| g.By("Pinning pool to RHCOS 9 via osImageURL because cluster OSImageStream default is rhel-10") | ||
| rhel9Image, err := osImageFromStream(ctx, mcClient, streamRHEL9) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| o.Expect(createPoolOSImageURLMachineConfig(ctx, mcClient, runcURLGuardRHEL9MCName, runcRHCOS10URLGuardPool, rhel9Image)).To(o.Succeed()) | ||
| cleanupRHEL9URLMC = true | ||
| } |
There was a problem hiding this comment.
I don't get why we need this. Can you explain?
Why don't we use just one mcp?
| return strings.Contains(message, osImageURLRuncRHEL10GuardMessage) && | ||
| strings.Contains(strings.ToLower(message), "runc") |
There was a problem hiding this comment.
This is redundant. osImageURLRuncRHEL10GuardMessage has "runc"
3 participants
Summary
This PR adds disruptive e2e coverage for the osImageURL runc-on-RHEL-10 guard (OCPNODE-4518 / MCO stream-class inspection), in the existing runc RHCOS 10 upgrade guard suite alongside the osImageStream cases.
The new test verifies that when a pool uses runc via ContainerRuntimeConfig and targets RHCOS 10 through a MachineConfig osImageURL override (with no MachineConfigPool.spec.osImageStream), MCO blocks at render time (RenderDegraded), sets cluster upgradeability to Upgradeable=False, and leaves the node on RHCOS 9 with runc.
Key design points:
Summary by CodeRabbit
Tests
osImageURLwhenruncis the default runtime.Documentation
osImageURLupgrade-guard scenario, expected behavior, configuration constraints, and related references.