MCO-2275: Part2 Migrate MCO OCB - #6340
Conversation
WalkthroughThe PR updates extended-privilege MCP and MOSC helpers, adds proxy-aware wait timing and non-Job pod readiness polling, expands OCB validation tolerance, and introduces serial long-duration tests for builds, MCP updates, node boot images, and rendered-image deletion. ChangesOCB validation and helper updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OCBTest
participant MachineOSConfig
participant MachineOSBuild
participant MachineConfigPool
participant NewNodes
OCBTest->>MachineOSConfig: read or update rendered image push spec
OCBTest->>MachineOSBuild: poll build conditions
MachineOSBuild-->>OCBTest: report build state
OCBTest->>MachineConfigPool: wait for MCP update
MachineConfigPool->>NewNodes: apply OCL image
NewNodes-->>OCBTest: report boot image state
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 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 |
57785af to
2b55131
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01 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
![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/extended-priv/mco_ocb_longduration.go (1)
450-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the unused
removeQuayImageUsingSkepohelper or wire it into the test path.
removeQuayImageUsingSkepois defined intest/extended-priv/mco_ocb_longduration.go, but there is no call to it in the Go codebase; onlyremoveImageStreamis used. Remove the dead helper, or if it is intended to run Skopeo, rename the name fromSkepo→Skopeoand wire it in.🤖 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_ocb_longduration.go` at line 450, Remove the unused removeQuayImageUsingSkepo helper, or, if it is required for the test flow, rename it to removeQuayImageUsingSkopeo and add a call from the appropriate test path. Ensure the final code has no dead helper and that only the intended image-removal implementation is used.test/extended-priv/util/pods.go (1)
205-205: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse the canonical Job label selector for consistency.
job-nameis still added by the Kubernetes Job controller for backward compatibility, so this selector still excludes Job pods, but the sibling readiness gate uses!batch.kubernetes.io/job-name. Align the selector with the canonical form to keep the test helpers consistent.🤖 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/util/pods.go` at line 205, Update the pod selector in the Run call to exclude Job pods using the canonical batch.kubernetes.io/job-name label, matching the selector used by the sibling readiness gate while preserving the existing namespace and template behavior.
🤖 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/machineconfigpool.go`:
- Line 588: Update the node-diagnostic command in the surrounding
polling/error-handling flow to capture the error returned by CLI.Execute() for
mcp.oc.Run("get").Args("nodes").Execute(). Preserve that error in the test
context or include it in the final assertion so failures from oc get nodes are
surfaced alongside the polling error.
---
Nitpick comments:
In `@test/extended-priv/mco_ocb_longduration.go`:
- Line 450: Remove the unused removeQuayImageUsingSkepo helper, or, if it is
required for the test flow, rename it to removeQuayImageUsingSkopeo and add a
call from the appropriate test path. Ensure the final code has no dead helper
and that only the intended image-removal implementation is used.
In `@test/extended-priv/util/pods.go`:
- Line 205: Update the pod selector in the Run call to exclude Job pods using
the canonical batch.kubernetes.io/job-name label, matching the selector used by
the sibling readiness gate while preserving the existing namespace and template
behavior.
🪄 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: Pro Plus
Run ID: 1e62a24b-94f0-4d4e-bc28-b8da47079c63
📒 Files selected for processing (5)
test/extended-priv/machineconfigpool.gotest/extended-priv/machineosconfig.gotest/extended-priv/mco_ocb.gotest/extended-priv/mco_ocb_longduration.gotest/extended-priv/util/pods.go
| } | ||
| if totalUpdating > maxUnavailable { | ||
| // print nodes for debug | ||
| mcp.oc.Run("get").Args("nodes").Execute() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C2 --type go 'func\s*\([^)]*\)\s*Execute\s*\(' .
rg -n -C2 --type go '\.Run\("get"\)\.Args\("nodes"\)\.Execute\(\)' test/extended-priv/machineconfigpool.goRepository: openshift/machine-config-operator
Length of output: 8796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## client.go around CLI Execute"
sed -n '820,885p' test/extended-priv/util/client.go
echo
echo "## machineconfigpool.go around diagnostic call"
sed -n '560,605p' test/extended-priv/machineconfigpool.go
echo
echo "## same repository calls to CLI Execute without error check"
rg -n --type go '\.+Execute\(\)' test/extended-priv -g '*.go'Repository: openshift/machine-config-operator
Length of output: 10952
Handle the diagnostic command failure.
CLI.Execute() returns the underlying oc get error, but line 588 discards it and the test still asserts the polling error. Capture the error, keep it in the context or prepend it to the final assertion, so node-dump failures surface useful diagnostics instead of being absorbed.
🤖 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` at line 588, Update the
node-diagnostic command in the surrounding polling/error-handling flow to
capture the error returned by CLI.Execute() for
mcp.oc.Run("get").Args("nodes").Execute(). Preserve that error in the test
context or include it in the final assertion so failures from oc get nodes are
surfaced alongside the polling error.
Source: Path instructions
|
/test unit |
- Add tests: 77497, 77498, 77576, 77977, 78001, 78196, 79137, 79172, 82536, 83136, 83137, 83139, 83755, 85843, 85980, 87176, 88801 - Add supporting functions: checkUpdatedLists, SkipIfCompactOrSNO, GetAllApplicableExtensionsToMCPOrFail, AssertAllNonJobPodsToBeReadyWithPollerParams - Add MCP methods: SetMaxUnavailable, RemoveMaxUnavailable, GetSortedUpdatedNodes - Add MOSC methods: SetRenderedImagePushspec, GetRenderedImagePushspec, IsUsingInternalRegistry - Preserve function ordering to match otp3 source
2b55131 to
1f393eb
Compare
|
4.23 Jobs (image: ci-ln-wqm0vh2/release:latest)
5.0 Jobs (image: ci-ln-9j50f32/release:latest)
The TC 83755 and TC 82536 is failing because of bug https://redhat.atlassian.net/browse/OCPBUGS-85094 hence, excluding it. |
openshift-ci-robot
added
the
jira/valid-reference
|
@ptalgulk01: This pull request references MCO-2275 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. |
1f393eb to
8c3aa0e
Compare
8c3aa0e to
61fc8ec
Compare
|
@ptalgulk01: 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. |
|
/hold Holding to allow the Kube rebase to land in #6321. Please ensure this will not cause merge conflicts for the Kube rebase before unholding this PR. |
openshift-ci
Bot
added
the
do-not-merge/hold
3 participants
MCO-2275: Migrate OCB longduration test cases (Part 2)
Completes the OCB test migration from openshift-tests-private to machine-config-operator. This is Part 2, following PR #6080 which migrated 11 OCB test cases and all shared helpers/infrastructure.
This PR adds the remaining 6 longduration OCB test cases:
Tests 83755 and 82536 are excluded via g.Label("Exclude: ...") until OCPBUGS-85094 is resolved. Both tests fail consistently across 4.23 and 5.0 on AWS shards due to an upstream bug, not a test migration issue.
Also includes:
Summary by CodeRabbit