Add new image-mode mcp status reporting TC - #6346
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughChangesMCP Updating condition coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 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 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
🤖 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_ocb.go`:
- Around line 1124-1152: Update the expected MCP Updating message substrings in
ValidateMOSBBuildingConditionOnMCP to match the controller’s Building-state
wording: use “Pool is paused; OS image build in progress (mosb: %s)” for paused
pools and “Pool is waiting for OS image build to complete (mosb: %s)” for
non-paused pools. Remove the Prepared-state “to start” and extra-“is” variants
while preserving the existing Eventually assertions and formatting.
🪄 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: 588bb876-6dec-4f15-8956-96fe600c2876
📒 Files selected for processing (1)
test/extended-priv/mco_ocb.go
| func ValidateMOSBBuildingConditionOnMCP(mosc *MachineOSConfig, mosb *MachineOSBuild, mcp *MachineConfigPool) { | ||
| exutil.By("Validate that MOSB building state is surfaced on MCP Updating condition") | ||
|
|
||
| o.Eventually(mosb, "5m", "20s").Should(HaveConditionField("Building", "status", TrueString), | ||
| "MachineOSBuild didn't report that the build has begun") | ||
|
|
||
| if mcp.IsPaused() { | ||
| o.Eventually(mcp, "5m", "20s").Should(HaveConditionField("Updating", "status", TrueString), | ||
| "MCP Updating status should be True when build is in progress (paused)") | ||
| o.Eventually(mcp, "2m", "20s").Should(HaveConditionField("Updating", "message", | ||
| o.Or( | ||
| o.ContainSubstring(fmt.Sprintf("Pool is paused; waiting for a new OS image build to start (mosc: %s)", mosc.GetName())), | ||
| o.ContainSubstring(fmt.Sprintf("Pool is paused; waiting for OS image build to start (mosb: %s)", mosb.GetName())), | ||
| o.ContainSubstring(fmt.Sprintf("Pool is paused; OS image build is in progress (mosb: %s)", mosb.GetName())), | ||
| )), | ||
| "MCP Updating message was not the expected one during building (paused)") | ||
| } else { | ||
| o.Eventually(mcp, "5m", "20s").Should(HaveConditionField("Updating", "status", TrueString), | ||
| "MCP Updating status should be True when build is in progress") | ||
| o.Eventually(mcp, "2m", "20s").Should(HaveConditionField("Updating", "message", | ||
| o.Or( | ||
| o.ContainSubstring(fmt.Sprintf("Pool is waiting for a new OS image build to start (mosc: %s)", mosc.GetName())), | ||
| o.ContainSubstring(fmt.Sprintf("Pool is waiting for OS image build to start (mosb: %s)", mosb.GetName())), | ||
| o.ContainSubstring(fmt.Sprintf("OS image build is in progress (mosb: %s)", mosb.GetName())), | ||
| )), | ||
| "MCP Updating message was not the expected one during building") | ||
| } | ||
| logger.Infof("OK!\n") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expected Updating message substrings don't match the controller's actual "Building" messages — this assertion will always fail.
Per the upstream contract in pkg/controller/node/status.go, once mosb is in the Building state:
- Non-paused message is
"Pool is waiting for OS image build to complete (mosb: %s)". - Paused message is
"Pool is paused; OS image build in progress (mosb: %s)".
Neither of these appears in the Or() substring lists here. The non-paused list checks for "...to start (mosb: %s)" and "OS image build is in progress (mosb: %s)" (extra "is", never emitted by the controller), and the paused list checks "...waiting for OS image build to start..." (that's the Prepared-state wording, not Building) plus the same extra-"is" variant. ContainSubstring only succeeds if the actual string literally contains the given substring, so once mosb reaches Building and the MCP reconciles, this Eventually will time out and fail deterministically — breaking the new [PolarionID:89821] disruptive test (call sites at Lines 400 and 429).
🐛 Proposed fix aligning expected substrings with controller-generated messages
if mcp.IsPaused() {
o.Eventually(mcp, "5m", "20s").Should(HaveConditionField("Updating", "status", TrueString),
"MCP Updating status should be True when build is in progress (paused)")
o.Eventually(mcp, "2m", "20s").Should(HaveConditionField("Updating", "message",
o.Or(
o.ContainSubstring(fmt.Sprintf("Pool is paused; waiting for a new OS image build to start (mosc: %s)", mosc.GetName())),
- o.ContainSubstring(fmt.Sprintf("Pool is paused; waiting for OS image build to start (mosb: %s)", mosb.GetName())),
- o.ContainSubstring(fmt.Sprintf("Pool is paused; OS image build is in progress (mosb: %s)", mosb.GetName())),
+ o.ContainSubstring(fmt.Sprintf("Pool is paused; OS image build has been prepared but will not rollout (mosb: %s)", mosb.GetName())),
+ o.ContainSubstring(fmt.Sprintf("Pool is paused; OS image build in progress (mosb: %s)", mosb.GetName())),
)),
"MCP Updating message was not the expected one during building (paused)")
} else {
o.Eventually(mcp, "5m", "20s").Should(HaveConditionField("Updating", "status", TrueString),
"MCP Updating status should be True when build is in progress")
o.Eventually(mcp, "2m", "20s").Should(HaveConditionField("Updating", "message",
o.Or(
o.ContainSubstring(fmt.Sprintf("Pool is waiting for a new OS image build to start (mosc: %s)", mosc.GetName())),
o.ContainSubstring(fmt.Sprintf("Pool is waiting for OS image build to start (mosb: %s)", mosb.GetName())),
- o.ContainSubstring(fmt.Sprintf("OS image build is in progress (mosb: %s)", mosb.GetName())),
+ o.ContainSubstring(fmt.Sprintf("Pool is waiting for OS image build to complete (mosb: %s)", mosb.GetName())),
)),
"MCP Updating message was not the expected one during building")
}Please double-check the exact current wording in the controller since it's authoritative:
#!/bin/bash
rg -n "Pool is (paused; )?(OS image build|waiting)" pkg/controller/node/status.go🤖 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.go` around lines 1124 - 1152, Update the expected
MCP Updating message substrings in ValidateMOSBBuildingConditionOnMCP to match
the controller’s Building-state wording: use “Pool is paused; OS image build in
progress (mosb: %s)” for paused pools and “Pool is waiting for OS image build to
complete (mosb: %s)” for non-paused pools. Remove the Prepared-state “to start”
and extra-“is” variants while preserving the existing Eventually assertions and
formatting.
|
@ptalgulk01: 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. |
|
/hold Holding to allow the Kube rebase to land in #6321 |
openshift-ci
Bot
added
the
do-not-merge/hold
2 participants
Updating condition
ValidateInterruptedMOSBConditionOnMCP, ValidateMOSBBuildingConditionOnMCP, ValidateSucceededMOSBConditionOnMCP
Summary by CodeRabbit