MCO-2275: Migrate OCB test cases from openshift-tests-private#6080
Open
ptalgulk01 wants to merge 1 commit into
Open
MCO-2275: Migrate OCB test cases from openshift-tests-private#6080ptalgulk01 wants to merge 1 commit into
ptalgulk01 wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@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. |
WalkthroughThis PR adds test infrastructure for extended-priv MCO tests: MachineConfigPool helpers (maxUnavailable mutation, node update polling, extension filtering), MachineOSConfig helpers (rendered image push spec and registry detection), general test utilities (cluster skipping, node list validation, pod readiness polling), and a large expansion of the OCB disruptive test suite with supporting helpers. ChangesMCO extended-priv test infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ptalgulk01 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.
Actionable comments posted: 3
🤖 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/machineosconfig.go`:
- Around line 333-335: The SetRenderedImagePushspec function currently
concatenates the raw rips string into a JSON patch body which breaks when rips
contains quotes or backslashes; update this by JSON-escaping the value before
embedding (e.g., use json.Marshal or construct the patch as a struct/slice and
json.Marshal it) and pass the marshaled patch string or bytes into
MachineOSConfig.Patch instead of raw concatenation; reference the
SetRenderedImagePushspec function and the rips variable and ensure the final
patch contains a valid JSON string value for "/spec/renderedImagePushSpec".
In `@test/extended-priv/mco_ocb.go`:
- Around line 418-429: The goroutine closure captures the loop variable mosc
which gets reassigned each iteration, causing races; to fix, ensure the
goroutine captures a stable reference by passing mosc as a parameter or copying
it to a new local before launching the goroutine (e.g., change the launch to go
func(moscCopy /*type optional*/){ defer g.GinkgoRecover(); defer wg.Done();
ValidateSuccessfulMOSC(moscCopy, nil) }(mosc)); update the closure around
ValidateSuccessfulMOSC, keeping wg.Add(1) and g.GinkgoRecover() as-is.
In `@test/extended-priv/mco.go`:
- Around line 320-328: The loop in function checkUpdatedLists uses "for i := 0;
i < len(l); i += step" which will hang or behave incorrectly when step <= 0; add
a guard at the start of checkUpdatedLists validating that step is > 0 (return
false and log an error via logger.Errorf if not) so the loop cannot run with
zero or negative increments, ensuring safe iteration over l and r.
🪄 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: a4781355-f7a0-4a91-b6e6-d142ebe844b8
📒 Files selected for processing (6)
test/extended-priv/machineconfigpool.gotest/extended-priv/machineosconfig.gotest/extended-priv/mco.gotest/extended-priv/mco_ocb.gotest/extended-priv/util.gotest/extended-priv/util/pods.go
Comment on lines
+418
to
+429
| mosc, err := CreateMachineOSConfigUsingExternalOrInternalRegistry(oc.AsAdmin(), MachineConfigNamespace, moscName, infraMcp.GetName(), nil) | ||
| defer mosc.CleanupAndDelete() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error creating the MachineOSConfig resource") | ||
| moscList = append(moscList, mosc) | ||
|
|
||
| wg.Add(1) | ||
| go func() { | ||
| defer g.GinkgoRecover() | ||
| defer wg.Done() | ||
|
|
||
| ValidateSuccessfulMOSC(mosc, nil) | ||
| }() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify goroutines in loops that capture outer variables directly.
rg -n -C3 'for .*range .*{|\bgo func\(' test/extended-priv/mco_ocb.goRepository: openshift/machine-config-operator
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact lines around 418-429
nl -ba test/extended-priv/mco_ocb.go | sed -n '400,460p'
# Also find the nearest 'for' loop and subsequent goroutines in that region
rg -n --context 3 'go func\(' test/extended-priv/mco_ocb.go | head -n 50Repository: openshift/machine-config-operator
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba test/extended-priv/mco_ocb.go | sed -n '380,460p'
echo '--- goroutines in file (first 50 matches) ---'
rg -n --context 3 'go func\(' test/extended-priv/mco_ocb.go | head -n 50
echo '--- potential loop variable closures around MOSC validation ---'
rg -n --context 5 'ValidateSuccessfulMOSC\(mosc' test/extended-priv/mco_ocb.goRepository: openshift/machine-config-operator
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba test/extended-priv/mco_ocb.go | sed -n '380,460p'
echo '--- goroutines in file (first 50 matches) ---'
rg -n --context 3 'go func\(' test/extended-priv/mco_ocb.go | head -n 50
echo '--- potential loop variable closures around MOSC validation ---'
rg -n --context 5 'ValidateSuccessfulMOSC\(mosc' test/extended-priv/mco_ocb.goRepository: openshift/machine-config-operator
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba test/extended-priv/mco_ocb.go | sed -n '380,460p'
echo '--- goroutines in file (first 50 matches) ---'
rg -n --context 3 'go func\(' test/extended-priv/mco_ocb.go | head -n 50
echo '--- validate successful mosc goroutine blocks ---'
rg -n --context 6 'ValidateSuccessfulMOSC\(' test/extended-priv/mco_ocb.goRepository: openshift/machine-config-operator
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba test/extended-priv/mco_ocb.go | sed -n '380,460p'
echo '--- goroutines in file (first 50 matches) ---'
rg -n --context 3 'go func\(' test/extended-priv/mco_ocb.go | head -n 50
echo '--- validate successful mosc goroutine blocks ---'
rg -n --context 6 'ValidateSuccessfulMOSC\(' test/extended-priv/mco_ocb.goRepository: openshift/machine-config-operator
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant region without `nl` (not available in this sandbox).
echo "=== File excerpt: lines 400-450 ==="
sed -n '400,450p' test/extended-priv/mco_ocb.go
echo
echo "=== Line numbers for ValidateSuccessfulMOSC occurrences ==="
rg -n 'ValidateSuccessfulMOSC\(' test/extended-priv/mco_ocb.go
echo
echo "=== Line numbers for go func() blocks near ValidateSuccessfulMOSC ==="
rg -n --context 2 'go func\(\)' test/extended-priv/mco_ocb.go
echo
echo "=== Exact snippet around 'ValidateSuccessfulMOSC(mosc' if present ==="
rg -n --context 6 'ValidateSuccessfulMOSC\(mosc' test/extended-priv/mco_ocb.go || trueRepository: openshift/machine-config-operator
Length of output: 9708
Fix goroutine closure capturing mosc in MOSC validation loop
In test/extended-priv/mco_ocb.go, the goroutine calls ValidateSuccessfulMOSC(mosc, nil) while mosc is reassigned each loop iteration, so goroutines may validate the wrong MOSC. Pass mosc into the goroutine as a parameter.
Proposed fix
- wg.Add(1)
- go func() {
+ moscForValidation := mosc
+ wg.Add(1)
+ go func(m *MachineOSConfig) {
defer g.GinkgoRecover()
defer wg.Done()
- ValidateSuccessfulMOSC(mosc, nil)
- }()
+ ValidateSuccessfulMOSC(m, nil)
+ }(moscForValidation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mosc, err := CreateMachineOSConfigUsingExternalOrInternalRegistry(oc.AsAdmin(), MachineConfigNamespace, moscName, infraMcp.GetName(), nil) | |
| defer mosc.CleanupAndDelete() | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Error creating the MachineOSConfig resource") | |
| moscList = append(moscList, mosc) | |
| wg.Add(1) | |
| go func() { | |
| defer g.GinkgoRecover() | |
| defer wg.Done() | |
| ValidateSuccessfulMOSC(mosc, nil) | |
| }() | |
| mosc, err := CreateMachineOSConfigUsingExternalOrInternalRegistry(oc.AsAdmin(), MachineConfigNamespace, moscName, infraMcp.GetName(), nil) | |
| defer mosc.CleanupAndDelete() | |
| o.Expect(err).NotTo(o.HaveOccurred(), "Error creating the MachineOSConfig resource") | |
| moscList = append(moscList, mosc) | |
| moscForValidation := mosc | |
| wg.Add(1) | |
| go func(m *MachineOSConfig) { | |
| defer g.GinkgoRecover() | |
| defer wg.Done() | |
| ValidateSuccessfulMOSC(m, nil) | |
| }(moscForValidation) |
🤖 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 418 - 429, The goroutine closure
captures the loop variable mosc which gets reassigned each iteration, causing
races; to fix, ensure the goroutine captures a stable reference by passing mosc
as a parameter or copying it to a new local before launching the goroutine
(e.g., change the launch to go func(moscCopy /*type optional*/){ defer
g.GinkgoRecover(); defer wg.Done(); ValidateSuccessfulMOSC(moscCopy, nil)
}(mosc)); update the closure around ValidateSuccessfulMOSC, keeping wg.Add(1)
and g.GinkgoRecover() as-is.
Contributor
Author
There was a problem hiding this comment.
wont this change the functionality or usage?
There was a problem hiding this comment.
@ptalgulk01 — Good catch questioning this. The fix would not change functionality, but more importantly, the original code is actually correct — my review comment was a false positive.
Here's why:
The mosc variable is declared inside the loop body with :=:
for _, infraMcp := range mcpList {
moscName := infraMcp.GetName()
mosc, err := CreateMachineOSConfigUsingExternalOrInternalRegistry(...) // <-- new variable each iteration
...
go func() {
ValidateSuccessfulMOSC(mosc, nil) // captures THIS iteration's mosc
}()
}The classic closure-over-loop-variable bug only applies when a goroutine captures a variable declared in the for clause itself (e.g., for _, v := range slice capturing v). In that case, all goroutines share the same v slot that gets mutated each iteration.
Since mosc is a fresh := declaration on every loop iteration, each goroutine already captures its own distinct mosc pointer — no race condition exists. The proposed fix (passing mosc as a parameter) produces identical behavior and is unnecessary.
You can safely keep the code as-is. I'll mark this comment as resolved.
🧠 Learnings used
Learnt from: HarshwardhanPatil07
Repo: openshift/machine-config-operator PR: 6034
File: test/extended-priv/mco_security.go:1011-1015
Timestamp: 2026-05-13T07:41:05.710Z
Learning: In the extended-priv Go/Ginkgo test suite, it’s safe to use synchronous `o.Expect` assertions on node state immediately after calling `mc.create()`.
Reason: `MachineConfig.create()` internally waits for the MachineConfigPool to complete (`mcp.waitForComplete()`) before returning unless `mc.skipWaitForMcp` is explicitly set to `true`. It also adjusts wait timeouts when extensions are present (`HasExtensionsSafe()`) or when kernel type changes (`GetKernelTypeSafe()`).
So, for node-state checks placed directly after `mc.create()` (and with `skipWaitForMcp` not enabled), you should not require `o.Eventually` for correctness. If `mc.skipWaitForMcp` is set to `true`, then `o.Eventually` may be needed.
Learnt from: ptalgulk01
Repo: openshift/machine-config-operator PR: 6081
File: test/extended-priv/mco_layering.go:932-1078
Timestamp: 2026-05-25T15:37:54.265Z
Learning: In this repository, for migration PRs that port existing tests from openshift-tests-private into this codebase (e.g., OS layering tests landing under test/extended-priv), keep the PR strictly focused on migrating the test logic. Do not add new conditional guards, skip logic, or supporting infrastructure/checks (for example, MicroShift-related skip checks) as part of the migration—those should be handled in separate follow-up PRs.
Comment on lines
+320
to
+328
| func checkUpdatedLists(l, r []*Node, step int) bool { | ||
| if len(l) != len(r) { | ||
| logger.Errorf("Compared lists have different size") | ||
| return false | ||
| } | ||
|
|
||
| indexStart := 0 | ||
| for i := 0; i < len(l); i += step { | ||
| indexEnd := i + step |
There was a problem hiding this comment.
Guard against step <= 0 before iterating.
At Line 327, i += step will never progress when step is 0 (or goes backwards if negative), causing a hang.
Proposed fix
func checkUpdatedLists(l, r []*Node, step int) bool {
+ if step <= 0 {
+ logger.Errorf("step must be > 0, got %d", step)
+ return false
+ }
if len(l) != len(r) {
logger.Errorf("Compared lists have different size")
return false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func checkUpdatedLists(l, r []*Node, step int) bool { | |
| if len(l) != len(r) { | |
| logger.Errorf("Compared lists have different size") | |
| return false | |
| } | |
| indexStart := 0 | |
| for i := 0; i < len(l); i += step { | |
| indexEnd := i + step | |
| func checkUpdatedLists(l, r []*Node, step int) bool { | |
| if step <= 0 { | |
| logger.Errorf("step must be > 0, got %d", step) | |
| return false | |
| } | |
| if len(l) != len(r) { | |
| logger.Errorf("Compared lists have different size") | |
| return false | |
| } | |
| indexStart := 0 | |
| for i := 0; i < len(l); i += step { | |
| indexEnd := i + step |
🤖 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.go` around lines 320 - 328, The loop in function
checkUpdatedLists uses "for i := 0; i < len(l); i += step" which will hang or
behave incorrectly when step <= 0; add a guard at the start of checkUpdatedLists
validating that step is > 0 (return false and log an error via logger.Errorf if
not) so the loop cannot run with zero or negative increments, ensuring safe
iteration over l and r.
Contributor
Author
There was a problem hiding this comment.
This fix is not required and we dont need to check the step count here
There was a problem hiding this comment.
@ptalgulk01, understood — acknowledged and moving on.
✏️ Learnings added
Learnt from: ptalgulk01
Repo: openshift/machine-config-operator PR: 6080
File: test/extended-priv/mco.go:320-328
Timestamp: 2026-05-25T17:06:17.684Z
Learning: In `test/extended-priv/mco.go`, the `checkUpdatedLists(l, r []*Node, step int)` function does not need a `step <= 0` guard. The `step` parameter always comes from `maxUnavailable` (an MCP configuration value), which is guaranteed to be positive by the time it reaches this function. The maintainer has explicitly confirmed this guard is not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: HarshwardhanPatil07
Repo: openshift/machine-config-operator PR: 6034
File: test/extended-priv/mco_security.go:1011-1015
Timestamp: 2026-05-13T07:41:05.710Z
Learning: In the extended-priv Go/Ginkgo test suite, it’s safe to use synchronous `o.Expect` assertions on node state immediately after calling `mc.create()`.
Reason: `MachineConfig.create()` internally waits for the MachineConfigPool to complete (`mcp.waitForComplete()`) before returning unless `mc.skipWaitForMcp` is explicitly set to `true`. It also adjusts wait timeouts when extensions are present (`HasExtensionsSafe()`) or when kernel type changes (`GetKernelTypeSafe()`).
So, for node-state checks placed directly after `mc.create()` (and with `skipWaitForMcp` not enabled), you should not require `o.Eventually` for correctness. If `mc.skipWaitForMcp` is set to `true`, then `o.Eventually` may be needed.
Learnt from: ptalgulk01
Repo: openshift/machine-config-operator PR: 6081
File: test/extended-priv/mco_layering.go:932-1078
Timestamp: 2026-05-25T15:37:54.265Z
Learning: In this repository, for migration PRs that port existing tests from openshift-tests-private into this codebase (e.g., OS layering tests landing under test/extended-priv), keep the PR strictly focused on migrating the test logic. Do not add new conditional guards, skip logic, or supporting infrastructure/checks (for example, MicroShift-related skip checks) as part of the migration—those should be handled in separate follow-up PRs.
- 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
ptalgulk01
force-pushed
the
migrate-mco-ocb
branch
from
56ad443 to
a4634f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/extended-priv/machineconfigpool.go (1)
564-564: 💤 Low valueTypo in comment: "thatn" should be "than"
- // Check that there aren't more thatn maxUpdatingNodes updating at the same time + // Check that there aren't more than maxUpdatingNodes updating at the same time🤖 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 564, Fix the typo in the comment that currently reads "// Check that there aren't more thatn maxUpdatingNodes updating at the same time" by changing "thatn" to "than" so it reads "// Check that there aren't more than maxUpdatingNodes updating at the same time"; this comment is associated with the maxUpdatingNodes check in the machineconfigpool test logic.test/extended-priv/mco_ocb.go (1)
13-16: 💤 Low valueImports are positioned after other imports, breaking standard grouping
The new imports (
os,path/filepath,sync,time) are placed after the package imports block, which breaks the conventional import grouping (stdlib first, then external packages).import ( "fmt" + "os" + "path/filepath" "regexp" "slices" "strings" + "sync" + "time" g "github.com/onsi/ginkgo/v2" o "github.com/onsi/gomega" exutil "github.com/openshift/machine-config-operator/test/extended-priv/util" logger "github.com/openshift/machine-config-operator/test/extended-priv/util/logext" - "os" - "path/filepath" - "sync" - "time" )🤖 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 13 - 16, The import statements for os, path/filepath, sync, and time are placed after the package-import block; move these standard-library imports into the main import block and group imports in canonical order (standard library first, then external packages) so the import list in mco_ocb.go contains os, path/filepath, sync, time together with other stdlib imports at the top of the import block.
🤖 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 1104-1183: The test incorrectly assumes MachineSet.GetNodes()
returns nodes sorted by creation time; update the test to explicitly select the
most-recent node instead of using the last slice element: after calling
existingMS.GetNodes() and duplicateMS.GetNodes() (used in existingNode /
duplicateMSNode), iterate the returned nodes and pick the one with the latest
creation timestamp (use the node object's creation timestamp accessor) or update
MachineSet.GetNodes() implementation to sort by creationTimestamp before
returning; replace usages that index the last element with this explicit
selection to ensure the newest node is validated regardless of GetNodes()
ordering.
---
Nitpick comments:
In `@test/extended-priv/machineconfigpool.go`:
- Line 564: Fix the typo in the comment that currently reads "// Check that
there aren't more thatn maxUpdatingNodes updating at the same time" by changing
"thatn" to "than" so it reads "// Check that there aren't more than
maxUpdatingNodes updating at the same time"; this comment is associated with the
maxUpdatingNodes check in the machineconfigpool test logic.
In `@test/extended-priv/mco_ocb.go`:
- Around line 13-16: The import statements for os, path/filepath, sync, and time
are placed after the package-import block; move these standard-library imports
into the main import block and group imports in canonical order (standard
library first, then external packages) so the import list in mco_ocb.go contains
os, path/filepath, sync, time together with other stdlib imports at the top of
the import block.
🪄 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: a5285284-556e-4ba9-b618-0863cd2fe7e4
📒 Files selected for processing (6)
test/extended-priv/machineconfigpool.gotest/extended-priv/machineosconfig.gotest/extended-priv/mco.gotest/extended-priv/mco_ocb.gotest/extended-priv/util.gotest/extended-priv/util/pods.go
Comment on lines
+1104
to
+1183
| func ValidateNewNodesBootDirectlyWithOCLImage(oc *exutil.CLI, mosc *MachineOSConfig, mcp *MachineConfigPool) { | ||
| // First validate MOSC build and application | ||
| ValidateSuccessfulMOSC(mosc, nil) | ||
|
|
||
| exutil.By("Get the OCL image") | ||
| oclImage := OrFail[string](mosc.GetStatusCurrentImagePullSpec()) | ||
| logger.Infof("OCL image: %s\n", oclImage) | ||
|
|
||
| // Detect if using internal registry - internal registry always requires 2 reboots | ||
| isInternalRegistry := OrFail[bool](mosc.IsUsingInternalRegistry()) | ||
|
|
||
| exutil.By("Check able to scale the node from existing Machineset") | ||
| msl, err := NewMachineSetList(oc.AsAdmin(), MachineAPINamespace).GetAll() | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Get machinesets failed") | ||
| o.Expect(msl).ShouldNot(o.BeEmpty(), "Machineset list is empty") | ||
| existingMS := msl[0] | ||
|
|
||
| o.Expect(existingMS.AddToScale(1)).NotTo(o.HaveOccurred()) | ||
|
|
||
| defer func() { | ||
| exutil.By("Scale down the node from existing Machineset") | ||
| existingMS.AddToScale(-1) | ||
| mcp.waitForComplete() | ||
| }() | ||
|
|
||
| exutil.By("Create duplicate machineset and scale new node") | ||
| machineset := OrFail[*MachineSet](GetScalableMachineSet(oc.AsAdmin())) | ||
| duplicateMSName := machineset.GetName() + "-ocl" | ||
| duplicateMS, err := machineset.Duplicate(duplicateMSName) | ||
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| defer func() { | ||
| if duplicateMS.Exists() { | ||
| logger.Infof("Deleting duplicate machineset %s and scale down the node", duplicateMS.GetName()) | ||
| o.Expect(duplicateMS.ScaleTo(0)).To(o.Succeed()) | ||
| o.Expect(duplicateMS.WaitUntilReady("10m")).To(o.Succeed()) | ||
| o.Expect(duplicateMS.Delete()).To(o.Succeed()) | ||
| } | ||
| }() | ||
| o.Expect(duplicateMS.ScaleTo(1)).To(o.Succeed()) | ||
|
|
||
| exutil.By("Wait for both existing new node added and for duplicate new node added to get ready") | ||
|
|
||
| o.Eventually(func(gm o.Gomega) { | ||
| gm.Expect(existingMS.GetIsReady()).To(o.BeTrue(), "MachineSet %s is not ready", existingMS.GetName()) | ||
| gm.Expect(duplicateMS.GetIsReady()).To(o.BeTrue(), "MachineSet %s is not ready", duplicateMS.GetName()) | ||
| }, "20m", "2m").Should(o.Succeed(), "MachineSets are not ready") | ||
|
|
||
| // For internal registry, wait for MCP to complete as it always requires 2 reboots | ||
| if isInternalRegistry { | ||
| logger.Infof("Internal registry detected, waiting for MCP to complete (requires 2 reboots)") | ||
| mcp.waitForComplete() | ||
| } | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Get the new node created by the machine set") | ||
|
|
||
| existingMSNodes, nErr := existingMS.GetNodes() | ||
| o.Expect(nErr).NotTo(o.HaveOccurred(), "Error getting the nodes created by MachineSet %s", existingMS.GetName()) | ||
| // Get the most recently created node (last in the sorted list) | ||
| existingNode := existingMSNodes[len(existingMSNodes)-1] | ||
| logger.Infof("Existing node: %s\n", existingNode.GetName()) | ||
| logger.Infof("OK!\n") | ||
|
|
||
| duplicateMSNodes, nErr := duplicateMS.GetNodes() | ||
| o.Expect(nErr).NotTo(o.HaveOccurred(), "Error getting the nodes created by MachineSet %s", duplicateMS.GetName()) | ||
| // Get the most recently created node (last in the sorted list) | ||
| duplicateMSNode := duplicateMSNodes[len(duplicateMSNodes)-1] | ||
| logger.Infof("Duplicate node: %s\n", duplicateMSNode.GetName()) | ||
| logger.Infof("OK!\n") | ||
|
|
||
| getNewNodeRebootValueForOCL(duplicateMSNode, oclImage) | ||
| getNewNodeRebootValueForOCL(existingNode, oclImage) | ||
|
|
||
| exutil.By("Wait for MCP to complete before scaling down nodes") | ||
| // This wait is necessary to prevent MCP from becoming temporarily degraded when using external registry | ||
| // (internal registry already waits via waitForComplete earlier in the test) | ||
| mcp.waitForComplete() | ||
| logger.Infof("OK!\n") | ||
| } |
There was a problem hiding this comment.
Verify assumption that GetNodes returns nodes sorted by creation time
At lines 1164 and 1171, the code assumes that GetNodes() returns nodes sorted by creation timestamp so that the last element is the most recently created node. If this assumption is incorrect, the test may validate the wrong node.
#!/bin/bash
# Verify that MachineSet.GetNodes() returns nodes sorted by creation time
# Check the GetNodes implementation in machineset.go
ast-grep --pattern 'func ($_ *MachineSet) GetNodes() $_ {
$$$
}'
# Also check if there's any sorting applied
rg -n -C5 'GetNodes\(\)' test/extended-priv/machineset.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 1104 - 1183, The test incorrectly
assumes MachineSet.GetNodes() returns nodes sorted by creation time; update the
test to explicitly select the most-recent node instead of using the last slice
element: after calling existingMS.GetNodes() and duplicateMS.GetNodes() (used in
existingNode / duplicateMSNode), iterate the returned nodes and pick the one
with the latest creation timestamp (use the node object's creation timestamp
accessor) or update MachineSet.GetNodes() implementation to sort by
creationTimestamp before returning; replace usages that index the last element
with this explicit selection to ensure the newest node is validated regardless
of GetNodes() ordering.
Contributor
|
@ptalgulk01: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit