MCO-2275: Part2 Migrate MCO OCB - #6340
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds an OCB long-duration E2E suite with image, node-boot, and MOSB rebuild checks. It also increases MCP wait durations by 30% for HTTP-proxy clusters. A duplicate package declaration causes compilation errors. ChangesOCB validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OCBLongDurationSuite
participant MachineOSConfig
participant MachineConfigPool
participant MachineSets
participant NewNodes
OCBLongDurationSuite->>MachineOSConfig: Configure OCB image and build settings
MachineOSConfig->>MachineConfigPool: Apply machine configuration
OCBLongDurationSuite->>MachineSets: Scale existing and duplicated MachineSets
MachineSets->>NewNodes: Create replacement nodes
OCBLongDurationSuite->>NewNodes: Validate expected OCL image
OCBLongDurationSuite->>MachineConfigPool: Wait for MCP completion
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 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
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 |
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. |
8c3aa0e to
61fc8ec
Compare
|
/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
|
/unhold Kube rebase landed |
openshift-ci
Bot
removed
the
do-not-merge/hold
61fc8ec to
76dc665
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended-priv/mco_ocb_longduration.go (1)
583-596: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDuplicate
packagedeclaration and duplicate function definitions break the build.The file contains the full content twice. A second
package extendedclause and import block start at line 583, and the entire body repeats through line 1463, including a secondvar _ = g.Describe(...)block and byte-identical redefinitions ofcheckNewBuildIsTriggered,getNewNodeRebootValueForOCL,ValidateNewNodesBootDirectlyWithOCLImage,removeImageStream,removeQuayImageUsingSkepo, andverifyMOSBRebuildAfterImageDeletion.A Go source file cannot contain two
packageclauses, and a package cannot declare the same function name twice. This is a compile-breaking defect, not a style issue.The PR description states it migrates six long-duration test cases, matching only the first
Describeblock (83137,79137,83139,83755,85843,82536). The second block's eleven test cases (79172,83136,78001,77498,77497,77576,77977,78196,88801,85980,87176) and the duplicated helpers must be merged into a singlepackageclause, a single import block (retainingsync, which the concurrent-build test needs), and a singleDescribebody containing allItblocks, with each helper function defined exactly once.🤖 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` around lines 583 - 596, Remove the duplicated second package/import block and repeated content in the file, retaining one package clause and one merged import block including sync. Consolidate both g.Describe bodies so all It cases remain, and keep exactly one definition each of checkNewBuildIsTriggered, getNewNodeRebootValueForOCL, ValidateNewNodesBootDirectlyWithOCLImage, removeImageStream, removeQuayImageUsingSkepo, and verifyMOSBRebuildAfterImageDeletion.
🤖 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 279: Update the proxy detection logic used by estimateWaitDuration to
consider both status.httpProxy and status.httpsProxy, applying the 1.3
proxyModifier when either is configured. When the proxy lookup returns proxyErr,
log the error and adjust behavior consistently with the adjacent SNO check
instead of silently retaining the default modifier.
In `@test/extended-priv/mco_ocb_longduration.go`:
- Around line 354-360: Replace the existingMS assignment from msl[0] with
GetScalableMachineSet(oc.AsAdmin()), preserving error handling and the
subsequent AddToScale(1) call. Use the same scalable-selection helper as the
duplicate-machineset path, while retaining the existing machineset-list
validation if still required.
- Around line 509-518: Update the image deletion flow around skopeoCommand and
fullCommand to avoid interpolating tmpAuthFile or digestedImage into a shell
command executed with “sh -c”. Pass these values as discrete arguments through
node.DebugNodeWithChrootStd, or apply robust shell quoting/escaping if the shell
and proxy environment sourcing must be retained.
- Around line 158-214: Replace the immediate MOSB condition assertions after
mc.create() in the “Check that the build is triggered” block with polling via
o.Eventually (or Consistently where appropriate). Re-fetch the current MOSB
inside the polling callback and verify Building becomes false and Succeeded
becomes true, preserving the existing expected build-state behavior while
accounting for mc.skipWaitForMcp = true.
---
Outside diff comments:
In `@test/extended-priv/mco_ocb_longduration.go`:
- Around line 583-596: Remove the duplicated second package/import block and
repeated content in the file, retaining one package clause and one merged import
block including sync. Consolidate both g.Describe bodies so all It cases remain,
and keep exactly one definition each of checkNewBuildIsTriggered,
getNewNodeRebootValueForOCL, ValidateNewNodesBootDirectlyWithOCLImage,
removeImageStream, removeQuayImageUsingSkepo, and
verifyMOSBRebuildAfterImageDeletion.
🪄 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: f8fd4e3b-4764-4753-8d48-7ff86dd0992d
📒 Files selected for processing (2)
test/extended-priv/machineconfigpool.gotest/extended-priv/mco_ocb_longduration.go
| totalNodes int | ||
| guessedNodes = 3 // the number of nodes that we will use if we cannot get the actual number of nodes in the cluster | ||
| masterAdjust = 1.0 | ||
| proxyModifier = 1.0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Proxy detection is incomplete and silently swallows lookup errors.
The check only reads .status.httpProxy. Clusters that configure only .status.httpsProxy (a common HTTPS-only egress setup) do not get the 1.3 wait-time multiplier, so estimateWaitDuration can under-estimate wait time on proxied clusters this check was meant to cover.
Also, when proxyErr != nil, the code proceeds silently with proxyModifier at its default. The SNO check just above (lines 315-322) logs the error and adjusts behavior instead of failing silently. Apply the same pattern here for consistency and diagnosability.
🔧 Proposed fix
proxy := NewResource(mcp.GetOC(), "proxy", "cluster")
- httpProxy, proxyErr := proxy.Get(`{.status.httpProxy}`)
- if proxyErr == nil && httpProxy != "" {
+ httpProxy, proxyErr := proxy.Get(`{.status.httpProxy}`)
+ httpsProxy, httpsProxyErr := proxy.Get(`{.status.httpsProxy}`)
+ if proxyErr != nil {
+ logger.Errorf("Not able to get the httpProxy status from the proxy resource. Err: %s", proxyErr)
+ }
+ if httpsProxyErr != nil {
+ logger.Errorf("Not able to get the httpsProxy status from the proxy resource. Err: %s", httpsProxyErr)
+ }
+ if (proxyErr == nil && httpProxy != "") || (httpsProxyErr == nil && httpsProxy != "") {
logger.Infof("Increase waiting time because the cluster is proxied")
proxyModifier = 1.3
}Also applies to: 328-335
🤖 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 279, Update the proxy
detection logic used by estimateWaitDuration to consider both status.httpProxy
and status.httpsProxy, applying the 1.3 proxyModifier when either is configured.
When the proxy lookup returns proxyErr, log the error and adjust behavior
consistently with the adjacent SNO check instead of silently retaining the
default modifier.
| g.It("[PolarionID:83755][OTP] In OCL check no new image is applied on node after applying ssh/password/file MC .[Disruptive]", g.Label("Exclude: excluded until OCPBUGS-85094 is fixed"), func() { | ||
| var ( | ||
| mcp = GetCompactCompatiblePool(oc.AsAdmin()) | ||
| node = mcp.GetSortedNodesOrFail()[0] | ||
|
|
||
| moscName = mcp.GetName() | ||
| mcName = fmt.Sprintf("test-ssh-%s", GetCurrentTestPolarionIDNumber()) | ||
|
|
||
| _, key = GenerateSSHKeyPairOrFail() | ||
| user = ign32PaswdUser{Name: "core", SSHAuthorizedKeys: []string{key}} | ||
| ) | ||
|
|
||
| exutil.By("Configure OCB functionality for the new worker MCP") | ||
| mosc, err := CreateMachineOSConfigUsingExternalOrInternalRegistry(oc.AsAdmin(), MachineConfigNamespace, moscName, mcp.GetName(), nil) | ||
| defer DisableOCL(mosc) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error creating the MachineOSConfig resource") | ||
| logger.Infof("Applied MOSC!\n") | ||
|
|
||
| ValidateSuccessfulMOSC(mosc, nil) | ||
| logger.Infof("MOSC is applied!\n") | ||
|
|
||
| exutil.By("Get the image that is currently applied on nodes") | ||
| initialImage := OrFail[string](node.GetRpmOstreeStatus(false)) | ||
| logger.Infof("Initial image: %s", initialImage) | ||
| logger.Infof("Got the initial image!\n") | ||
|
|
||
| exutil.By("Create a new MC to deploy new authorized keys") | ||
| mc := NewMachineConfig(oc.AsAdmin(), mcName, mcp.GetName()) | ||
| mc.parameters = []string{fmt.Sprintf(`PWDUSERS=[%s]`, MarshalOrFail(user))} | ||
| mc.skipWaitForMcp = true | ||
|
|
||
| mc.create() | ||
| defer mc.DeleteWithWait() | ||
| logger.Infof("Created MC!\n") | ||
|
|
||
| exutil.By("Check that the build is triggered with succeed status and not building") | ||
| mosb, err := mosc.GetCurrentMachineOSBuild() | ||
| logger.Infof("MOSB: %s\n", mosb) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "Error getting MOSB from MOSC") | ||
| o.Expect(mosb).To(HaveConditionField("Building", "status", FalseString), "Build is still building") | ||
| o.Expect(mosb).To(HaveConditionField("Succeeded", "status", TrueString), "Build didn't succeed") | ||
| logger.Infof("Checked that the build does not take place!\n") | ||
|
|
||
| mcp.waitForComplete() | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Check that the image is not updated") | ||
| o.Expect(OrFail[string](node.GetRpmOstreeStatus(false))).To(o.Equal(initialImage), "Image was updated") | ||
| logger.Infof("Image is not updated!\n") | ||
|
|
||
| exutil.By("Check that all expected keys are present and with the right permissions and owners") | ||
| currentMc := OrFail[*MachineConfig](mcp.GetConfiguredMachineConfig()) | ||
| initialKeys := OrFail[[]string](currentMc.GetAuthorizedKeysByUserAsList("core")) | ||
| checkAuthorizedKeyInNode(node, append(initialKeys, key)) | ||
| logger.Infof("MC is configured with the expected keys!\n") | ||
|
|
||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Synchronous assertion after mc.create() with skipWaitForMcp = true risks a race.
mc.skipWaitForMcp = true is set before mc.create() (lines 187-189). The code then immediately does a synchronous o.Expect on the MOSB Building/Succeeded conditions (lines 194-199) without o.Eventually. Since skipWaitForMcp bypasses the internal wait for pool completion, the controller may not yet have reconciled the MOSB conditions at the moment of the check, producing an intermittent false pass or false fail.
Based on learnings, "If mc.skipWaitForMcp is set to true, then o.Eventually may be needed" for node/build-state checks placed after mc.create(). Wrap the MOSB condition checks in o.Eventually (or o.Consistently, since the test intends to confirm no rebuild happens) to eliminate the race.
🤖 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` around lines 158 - 214, Replace
the immediate MOSB condition assertions after mc.create() in the “Check that the
build is triggered” block with polling via o.Eventually (or Consistently where
appropriate). Re-fetch the current MOSB inside the polling callback and verify
Building becomes false and Succeeded becomes true, preserving the existing
expected build-state behavior while accounting for mc.skipWaitForMcp = true.
Source: Learnings
| 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()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use GetScalableMachineSet instead of msl[0] for the existing-machineset scale-up.
existingMS := msl[0] picks the first machineset returned by NewMachineSetList(...).GetAll() without verifying it can actually be scaled. Two lines later, the duplicate machineset path correctly uses GetScalableMachineSet(oc.AsAdmin()) for the same purpose. If msl[0] happens to be a machineset without available capacity, existingMS.AddToScale(1) at line 360 can fail or hang, causing flaky test failures.
Use the same scalable-selection helper for both machinesets.
🤖 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` around lines 354 - 360, Replace
the existingMS assignment from msl[0] with GetScalableMachineSet(oc.AsAdmin()),
preserving error handling and the subsequent AddToScale(1) call. Use the same
scalable-selection helper as the duplicate-machineset path, while retaining the
existing machineset-list validation if still required.
| skopeoCommand := fmt.Sprintf("skopeo delete --authfile %s docker://%s", tmpAuthFile, digestedImage) | ||
| fullCommand := fmt.Sprintf("set -a; source /etc/mco/proxy.env; %s", skopeoCommand) | ||
|
|
||
| logger.Infof("Executing command on node: %s", skopeoCommand) | ||
|
|
||
| stdout, stderr, err := node.DebugNodeWithChrootStd("sh", "-c", fullCommand) | ||
| if err != nil { | ||
| logger.Errorf("Error deleting Quay image via skopeo on node. Stdout: %s, Stderr: %s, Error: %s", stdout, stderr, err) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Shell command built via string interpolation and executed through sh -c.
skopeoCommand and fullCommand are built with fmt.Sprintf and executed via node.DebugNodeWithChrootStd("sh", "-c", fullCommand). digestedImage (from mosb.GetStatusDigestedImagePullSpec()) and tmpAuthFile are interpolated directly into the shell string rather than passed as discrete, quoted arguments. If either value ever contains shell metacharacters, this executes arbitrary commands with root access on the node via oc debug.
Practical risk is reduced because these values currently originate from cluster-generated resources rather than raw external input, but the pattern itself is the kind of shell-interpolation construction that should be avoided. Pass arguments without going through a shell, or quote/escape interpolated values before building the command string.
🤖 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` around lines 509 - 518, Update
the image deletion flow around skopeoCommand and fullCommand to avoid
interpolating tmpAuthFile or digestedImage into a shell command executed with
“sh -c”. Pass these values as discrete arguments through
node.DebugNodeWithChrootStd, or apply robust shell quoting/escaping if the shell
and proxy environment sourcing must be retained.
Source: Path instructions
76dc665 to
9ed67c7
Compare
|
/cherry-pick release-4.22 |
|
@ptalgulk01: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
| proxy := NewResource(mcp.GetOC(), "proxy", "cluster") | ||
| httpProxy, proxyErr := proxy.Get(`{.status.httpProxy}`) | ||
| if proxyErr == nil && httpProxy != "" { | ||
| logger.Infof("Increase waiting time because the cluster is proxied") | ||
| proxyModifier = 1.3 | ||
| } |
There was a problem hiding this comment.
Same as part 1, proxy should not increase the time needed to update a pool. Let's remove this code.
| wMcp.SetMaxUnavailable(2) | ||
| defer wMcp.RemoveMaxUnavailable() |
There was a problem hiding this comment.
Let's restore the original maxUnavailable value here.
In the original code we don't do it, but in the MCO repository there was a PR to guarantee that maxUnavailable was always restored #6336
| o.Expect(err).NotTo(o.HaveOccurred(), "Error creating the MachineOSConfig resource") | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Configure OCB functionality for the new worker MCP") |
There was a problem hiding this comment.
Wrong step name, it looks like we copy pasted the previous one.
| ValidateNewNodesBootDirectlyWithOCLImage(oc.AsAdmin(), mosc, mcp) | ||
| }) | ||
|
|
||
| g.It("[PolarionID:82536][OTP][Skipped:Disconnected] Internal Registry In OCB to check when a image is removed the old build is triggered again and the MC should start updating directly. [Disruptive]", g.Label("Exclude: excluded until OCPBUGS-85094 is fixed"), func() { |
There was a problem hiding this comment.
OCPBUGS-85094 is making these 2 tests fail, but those tests failing are not leaving the cluster in an unrecoverable status. Hence, we shouldn't skip the failing tests, we should let them fail and report the failures so that it is visible.
If those tests were leaving the cluster in an unrecoverable status it would be different.
875c990 to
5b88c4b
Compare
826cc41 to
dd9e989
Compare
Add the remaining OCB long-duration test coverage to mco_ocb_longduration.go, including internal-registry and multi-MCP scenarios, and clean up helper naming for readability.
dd9e989 to
c19a014
Compare
|
/test unit |
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ptalgulk01, sergiordlr 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 |
|
/verified by @ptalgulk01 |
openshift-ci-robot
added
the
verified
|
@ptalgulk01: This PR has been marked as verified by 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. |
|
@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. |
|
@ptalgulk01: #6340 failed to apply on top of branch "release-4.22": 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 kubernetes-sigs/prow repository. |
MCO-2275: Part2 Migrate MCO OCB
5 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
Bug Fixes
Tests