MCO-2380: MCO-2381: Expose MachineOSBuild Status Conditions on Paused MachineConfigPools and add E2E Tests for Paused Pool Builds#6282
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@anandram2: This pull request references MCO-2380 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. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe node status controller now reports MachineOSBuild progress through MachineConfigPoolUpdating for paused pools, with separate handling for prepared, building, failed, succeeded, and missing-build states. Unit tests, helper builders, and an end-to-end test were updated to cover the paused-pool flow. ChangesPaused pool MachineOSBuild status reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant MachineConfigPool
participant MachineOSBuildBuilder
participant NodeImage
E2ETest->>MachineConfigPool: pause pool
E2ETest->>MachineOSBuildBuilder: create prepared/building/failed/successful build states
E2ETest->>MachineConfigPool: poll MachineConfigPoolUpdating while paused
E2ETest->>MachineConfigPool: unpause pool
MachineConfigPool->>NodeImage: apply completed build image
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandram2 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 review |
✅ Action performedReview finished.
|
![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 (1)
test/helpers/machineosbuildbuilder.go (1)
107-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
apihelperscondition helpers here too.MachineOSBuildRunningConditions()andMachineOSBuildFailedConditions()already encode these slices, so switchingWithBuildInProgress()andWithFailedBuild()to them avoids condition drift and keeps the builder consistent withWithBuildPrepared().🤖 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/helpers/machineosbuildbuilder.go` around lines 107 - 141, The manual condition slice in WithBuildInProgress is duplicating MachineOSBuild status logic and can drift from the shared helpers. Update WithBuildInProgress to use the existing apihelpers condition helper, matching the pattern already used by WithBuildPrepared and the failed-state builder, so the MachineOSBuildBuilder stays consistent with MachineOSBuildRunningConditions and MachineOSBuildFailedConditions.
🤖 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 `@pkg/controller/node/status.go`:
- Around line 286-287: The switch in node status handling has an empty
IsBuildPrepared() branch, which causes the non-paused pool path to skip setting
the Updating condition; update the logic in the status computation around the
MOSB state switch so the IsBuildPrepared() case uses the same build-specific
Updating message as the building path, and remove the now-dead IsBuildPrepared()
check from the following IsBuilding() case. Also add a unit test covering the
non-paused branch when the MOSB is in the prepared state to verify the
build-specific condition is returned.
---
Nitpick comments:
In `@test/helpers/machineosbuildbuilder.go`:
- Around line 107-141: The manual condition slice in WithBuildInProgress is
duplicating MachineOSBuild status logic and can drift from the shared helpers.
Update WithBuildInProgress to use the existing apihelpers condition helper,
matching the pattern already used by WithBuildPrepared and the failed-state
builder, so the MachineOSBuildBuilder stays consistent with
MachineOSBuildRunningConditions and MachineOSBuildFailedConditions.
🪄 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: e610e29c-b093-4ea2-bbc1-bb7b5aba4ffb
📒 Files selected for processing (4)
pkg/controller/node/status.gopkg/controller/node/status_test.gotest/e2e-ocl-2of2/onclusterlayering_test.gotest/helpers/machineosbuildbuilder.go
anandram2
force-pushed
the
decouple-image-and-build
branch
from
de3832d to
87498fa
Compare
|
@anandram2: 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. |
2 participants
- What I did
- How to verify it
Run the unit tests:
go test ./pkg/controller/node/ -run TestCalculateStatusWithImageModeReportingor
Run the e2e test (requires a cluster with OCL enabled):
go test -v ./test/e2e-ocl-2of2/ -run TestPausedMCPSurfacesMachineOSBuildStatusManual verification on a cluster:
- Description for the changelog
Surface MachineOSBuild status on paused MachineConfigPools and add e2e coverage
MCO-2380
MCO-2381
Summary by CodeRabbit
New Features
MachineConfigPoolUpdatingreporting when a MachineConfigPool is paused, surfacing distinctMachineOSBuildlifecycle stages (waiting, prepared, in progress) and clearer, stage-specific messages.Bug Fixes
MachineConfigPoolUpdatingcorrectly transitions to completion states (success/failure) while paused, with accurate condition status and messaging aligned toMachineOSBuild.Tests
MachineOSBuildlifecycle control and new helpers to set build condition states.