NO-ISSUE: Adapt vsphere bootimage tests for mult-vcenter scenarios - #6317
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
approved
WalkthroughThe PR adds MachineSet-based vSphere failure-domain lookup and credential parsing. It passes MachineSet context through boot-image and upload helpers, adds folder-aware vSphere operations, and removes vSphere from ControlPlaneMachineSet boot-image support. ChangesMachineSet-aware vSphere boot-image flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BootImageTest
participant MachineSet
participant vSphereHelpers
participant ImageUploader
BootImageTest->>MachineSet: derive backdated image and workspace
MachineSet->>vSphereHelpers: resolve failure domain and connection info
vSphereHelpers->>ImageUploader: provide connection info and folder
ImageUploader->>ImageUploader: upload OVA into selected folder
BootImageTest->>vSphereHelpers: check updated or unchanged image
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![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_scale.go`:
- Around line 691-698: Update the uploadBaseImageToCloud flow around
GetVSphereConnectionInfoForMachineSet to handle a nil ms before dereferencing
it: use the default GetVSphereConnectionInfo when ms is nil, and retain the
existing MachineSet-specific lookup and workspace folder extraction for non-nil
ms values.
🪄 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: d265aed6-2975-4ffd-aa5f-c1d0bcb64594
📒 Files selected for processing (6)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_bootimages_skew.gotest/extended-priv/mco_controlplanemachineset.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
11dae54 to
cf39800
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
added
the
needs-rebase
cf39800 to
7473dfb
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
7473dfb to
45539e8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
45539e8 to
5cc393d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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_bootimages.go`:
- Around line 1072-1080: Update the consistency assertions around
getReleaseFromVsphereTemplate and bir.GetCoreOsBootImage so that, when
fakeImageName is non-empty, Consistently verifies the MachineSet continues
referencing fakeImageName. Retain the existing release assertion as an
additional check for detecting in-place updates.
- Line 993: Update GetReleaseFromVsphereTemplate and its callers to accept the
MachineSet workspace folder, then resolve the template with
finder.VirtualMachine(ctx, path.Join(folder, vsphereTemplate)) instead of
searching by name across the datacenter. Pass workspace.folder from the
UploadBaseImageToVsphere flow and preserve the existing release lookup behavior
within that folder.
In `@test/extended-priv/util/vsphere.go`:
- Around line 408-418: Validate that both info.User and info.Password are
non-empty immediately after their decoded assignments in the credential-loading
helper. Return a descriptive error when either credential is empty, without
including secret values, while preserving the existing missing-key and
decode-error handling.
- Line 134: The log statements in test/extended-priv/util/vsphere.go at lines
134-134 and 286-286 expose raw vSphere workspace folder paths; update both
logger.Infof calls in the relevant workspace-folder flows to remove folder from
the message or replace it with a safe redacted representation.
- Line 268: Update the deferred cleanup around c.Logout(ctx) to capture and
handle its returned error instead of discarding it. Preserve any earlier
operation error, while returning or explicitly recording the logout error when
no prior error occurred.
- Around line 284-287: Update the error handling after finder.VirtualMachine in
the surrounding cleanup function to return nil only when err is a govmomi
find.NotFoundError; wrap and return all other errors, including permission,
communication, and invalid-path failures, while preserving the existing
missing-template log for the not-found case.
- Around line 255-256: Update DeleteVsphereTemplate to accept a caller-provided
context, derive a bounded timeout context for the cleanup workflow, and pass it
through connection, discovery, VM lookup, destroy-task waiting, and logout
operations. Remove the context.Background usage while preserving cancellation
and timeout propagation across all vSphere calls.
🪄 Autofix
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: 8fc7e046-33b0-4bda-be34-d6d1a35d6e45
📒 Files selected for processing (7)
test/extended-priv/controlplanemachineset.gotest/extended-priv/machineset.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_bootimages_skew.gotest/extended-priv/mco_controlplanemachineset.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
🚧 Files skipped from review as they are similar to previous changes (4)
- test/extended-priv/machineset.go
- test/extended-priv/controlplanemachineset.go
- test/extended-priv/mco_scale.go
- test/extended-priv/mco_bootimages_skew.go
| func DeleteVsphereTemplate(templateName, folder string, vsInfo *VSphereConnectionInfo) error { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/extended-priv/util/vsphere.go --items all --type function
rg -n --type=go -C 4 '\b(DeleteVsphereTemplate|UploadBaseImageToVsphere)\s*\(' test/extended-privRepository: openshift/machine-config-operator
Length of output: 3418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Go module dependency/version clues ---\n'
for f in go.mod test/extended-priv/util/vsphere.go test/extended-priv/mco_scale.go; do
if [ -f "$f" ]; then
echo "*** $f"
sed -n '1,80p' "$f"
fi
done
printf '\n--- DeleteVsphereTemplate implementation ---\n'
sed -n '255,300p' test/extended-priv/util/vsphere.go
printf '\n--- UploadBaseImageToVsphere context usage ---\n'
sed -n '84,180p' test/extended-priv/util/vsphere.go
printf '\n--- DeleteVsphereTemplate call sites ---\n'
rg -n --type=go -C 5 '\bDeleteVsphereTemplate\s*\(' .Repository: openshift/machine-config-operator
Length of output: 262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Go module dependency/version clues ---'
for f in go.mod test/extended-priv/util/vsphere.go test/extended-priv/mco_scale.go; do
if [ -f "$f" ]; then
echo "*** $f"
sed -n '1,80p' "$f"
fi
done
printf '%s\n' ''
printf '%s\n' '--- DeleteVsphereTemplate implementation ---'
sed -n '255,300p' test/extended-priv/util/vsphere.go
printf '%s\n' ''
printf '%s\n' '--- UploadBaseImageToVsphere context usage ---'
sed -n '84,180p' test/extended-priv/util/vsphere.go
printf '%s\n' ''
printf '%s\n' '--- DeleteVsphereTemplate call sites ---'
rg -n --type=go -C 5 '\bDeleteVsphereTemplate\s*\(' .Repository: openshift/machine-config-operator
Length of output: 14762
🌐 Web query:
govmomi Client Logout context parameter signature NewClient context parameter
💡 Result:
In the govmomi library, both the NewClient function and the Client.Logout method include a context.Context parameter as part of their signatures [1][2][3][4]. NewClient Signature The NewClient function is defined as follows: func NewClient(ctx context.Context, u *url.URL, insecure bool) (*Client, error) [1][3] This function creates a new client from a given URL [1]. The context parameter is used to manage the lifecycle of the initial connection and the optional authentication login process if the provided URL contains user credentials [1]. Client.Logout Signature The Logout method on the Client type is defined as follows: func (c *Client) Logout(ctx context.Context) error [1][4] This method dispatches the logout request to the session manager and also triggers the closing of any idle connections maintained by the client [1][2]. The context parameter allows for the cancellation or timeout of the logout request [1].
Citations:
- 1: https://github.com/vmware/govmomi/blob/main/client.go
- 2: https://github.com/vmware/govmomi/blob/v0.53.0/client.go
- 3: https://godocs.io/github.com/vmware/govmomi
- 4: http://godoc.org/github.com/vmware/govmomi
Pass a cancellable context with a timeout to vSphere cleanup operations.
DeleteVsphereTemplate creates context.Background() and passes it through connection, discovery, VM lookup, destroy task wait, and logout calls. If any vSphere request hangs, deferred logout cleanup can remain blocked without a caller-controlled cancellation or deadline. Accept a caller context and derive a timeout for the cleanup workflow.
🤖 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/vsphere.go` around lines 255 - 256, Update
DeleteVsphereTemplate to accept a caller-provided context, derive a bounded
timeout context for the cleanup workflow, and pass it through connection,
discovery, VM lookup, destroy-task waiting, and logout operations. Remove the
context.Background usage while preserving cancellation and timeout propagation
across all vSphere calls.
Source: Path instructions
5cc393d to
a1d7cae
Compare
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
|
/test bootstrap-unit |
a1d7cae to
71d82dd
Compare
|
/test e2e-vsphere-multi-vcenter-mco-bootimages |
|
@sergiordlr: 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. |
| } | ||
|
|
||
| // CheckCurrentOSImageIsNotUpdated checks that the machineset/controlplanemachineset is NOT using the current cluster bootimage, | ||
| // i.e. the MCO has not updated it. On vSphere, where the template name doesn't change during updates, it checks that the |
There was a problem hiding this comment.
Does this also capture cases where the cluster has a template outside the folder? Or are they just not relevant to check for?
There was a problem hiding this comment.
No, it doesn't capture this case.
This test doesn't exist yet. If we add a test case to check that the template name is updated when the template is out of the folder we need to revisit this check and make it compatible.
I've focused only on making the existing tests work.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
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_bootimages.go (1)
960-968: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA reused vSphere template already carries the current release, so positive assertions pass without the MCO. The backdated template name is deterministic, and
UploadBaseImageToVspherereuses an existing template with that name without comparing its release. The MCO updates the OVA in place under the same name, so any later reuse starts from a current template.
test/extended-priv/mco_bootimages.go#L960-L968: add a per-test suffix to the"mcotest-"template name, or verify the release of the existing template and re-upload when it is not the 4.16 release.test/extended-priv/mco_bootimages.go#L106-L112: re-upload the backdated OVA before the opt-in phase, or use a second backdated template name, so the check at line 111 cannot pass on the template already updated at line 80.🤖 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_bootimages.go` around lines 960 - 968, Ensure reused vSphere templates cannot let positive assertions pass without the MCO: in test/extended-priv/mco_bootimages.go lines 960-968, make the “mcotest-” template name unique per test or verify its release and re-upload when it is not 4.16; in lines 106-112, re-upload the backdated OVA before opt-in or use a separate backdated template name so the pre-opt-in check cannot reuse the template updated earlier.
♻️ Duplicate comments (1)
test/extended-priv/mco_bootimages.go (1)
1067-1085: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe vSphere negative check cannot fail.
The callers pass a name that does not exist as a template, for example
fake-noupdate-image-81403.getReleaseFromVsphereTemplatetherefore always returns an error, the closure converts it to"", and""never equalscurrentCoreOsBootImage. The assertion passes on every run, including runs where the MCO did replace the boot image reference. The error-to-""conversion also hides connection, credential, and folder failures.Assert the reference directly, and keep the release check as an additional signal.
🐛 Proposed fix
switch platform { case VspherePlatform: var ( region = getCurrentRegionOrFail(oc) arch = bir.GetArchitectureOrFail() coreosBootimagesCM = NewConfigMap(oc.AsAdmin(), MachineConfigNamespace, "coreos-bootimages") ) currentCoreOsBootImage := getCoreOsBootImageFromConfigMapOrFail(platform, region, arch, coreosBootimagesCM) o.Expect(currentCoreOsBootImage).NotTo(o.BeEmpty(), "Could not find the right coreOS image for this platform") + // The MCO must not change the reference at all. + o.Consistently(bir.GetCoreOsBootImage, "15s", "5s").Should(o.Equal(fakeImageName), + "%s boot image reference was changed but it should NOT have been", bir) + o.Consistently(func() string { release, err := getReleaseFromVsphereTemplate(bir) if err != nil { // A non-existing template means the MCO did not update it return "" } return release }, "15s", "5s").ShouldNot(o.Equal(currentCoreOsBootImage), "%s was updated but it should NOT have been", bir)🤖 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_bootimages.go` around lines 1067 - 1085, Update the vSphere branch around getReleaseFromVsphereTemplate so the negative assertion checks the boot image reference directly rather than converting lookup errors to an empty string. Keep the existing release check as an additional signal, and ensure connection, credential, folder, and other lookup errors are surfaced instead of treated as a successful mismatch.
🧹 Nitpick comments (2)
test/extended-priv/mco_bootimages.go (1)
1001-1004: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
fakeImageNameto reflect both meanings.On AWS, GCP, and Azure the argument is the value that must no longer be present. On vSphere it is the value that must still be present. Callers pass the real backdated template name for vSphere, so the name
fakeImageNameis misleading and invites the wrong argument.expectedPriorImagedescribes both cases.Also applies to: 1021-1033
🤖 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_bootimages.go` around lines 1001 - 1004, Rename the CheckCurrentOSImageIsUpdated parameter fakeImageName to expectedPriorImage and update all references and callers, including the vSphere path where the prior template name must remain present and non-vSphere paths where it must be absent.test/extended-priv/util/vsphere.go (1)
144-148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the folder-qualified lookup for the reuse check.
DeleteVsphereTemplateandGetReleaseFromVsphereTemplatesearch withfolder + "/" + namewhenfolderis set. The reuse check searches by barebaseImageDest, then compares the result’s parent path. If the same template name exists in another folder,finder.VirtualMachinecan return a multiple-match error and skip reuse, causing a duplicate upload into the correct folder. Use the same folder-qualified lookup here.🤖 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/vsphere.go` around lines 144 - 148, Update the reuse check around finder.VirtualMachine to build the lookup name with vmFolder and baseImageDest, matching the folder-qualified lookup used by DeleteVsphereTemplate and GetReleaseFromVsphereTemplate. When vmFolder is set, query using the folder/name form; otherwise retain the existing bare-name lookup, while preserving the existing folder validation and reuse 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/machineset.go`:
- Around line 757-761: The failure-domain lookup in
GetVSphereConnectionInfoForMachineSet must not match only server and datacenter;
also compare the workspace’s datastore and resourcePool values against the
corresponding failure-domain topology fields, or use an equivalent workspace key
uniquely identifying the domain. Preserve returning fd.Raw only for a fully
matching failure domain.
In `@test/extended-priv/mco_bootimages.go`:
- Around line 106-112: Update the opt-in phase around SetCoreOsBootImage and
CheckCurrentOSImageIsUpdated to use a genuinely backdated template: either
re-upload the backdated OVA before setting the image or assign a distinct
backdated template name. Ensure the check observes an MCO-triggered update
rather than an already-current vSphere template.
- Around line 224-231: Run gofmt on the extended-priv test file, including the
variable declarations associated with PolarionID 74239, and retain the resulting
formatting without changing test behavior.
---
Outside diff comments:
In `@test/extended-priv/mco_bootimages.go`:
- Around line 960-968: Ensure reused vSphere templates cannot let positive
assertions pass without the MCO: in test/extended-priv/mco_bootimages.go lines
960-968, make the “mcotest-” template name unique per test or verify its release
and re-upload when it is not 4.16; in lines 106-112, re-upload the backdated OVA
before opt-in or use a separate backdated template name so the pre-opt-in check
cannot reuse the template updated earlier.
---
Duplicate comments:
In `@test/extended-priv/mco_bootimages.go`:
- Around line 1067-1085: Update the vSphere branch around
getReleaseFromVsphereTemplate so the negative assertion checks the boot image
reference directly rather than converting lookup errors to an empty string. Keep
the existing release check as an additional signal, and ensure connection,
credential, folder, and other lookup errors are surfaced instead of treated as a
successful mismatch.
---
Nitpick comments:
In `@test/extended-priv/mco_bootimages.go`:
- Around line 1001-1004: Rename the CheckCurrentOSImageIsUpdated parameter
fakeImageName to expectedPriorImage and update all references and callers,
including the vSphere path where the prior template name must remain present and
non-vSphere paths where it must be absent.
In `@test/extended-priv/util/vsphere.go`:
- Around line 144-148: Update the reuse check around finder.VirtualMachine to
build the lookup name with vmFolder and baseImageDest, matching the
folder-qualified lookup used by DeleteVsphereTemplate and
GetReleaseFromVsphereTemplate. When vmFolder is set, query using the folder/name
form; otherwise retain the existing bare-name lookup, while preserving the
existing folder validation and reuse behavior.
🪄 Autofix
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: b6007d34-47e5-448a-8b4e-af3c3496b3f8
📒 Files selected for processing (5)
test/extended-priv/machineset.gotest/extended-priv/mco_bootimages.gotest/extended-priv/mco_controlplanemachineset.gotest/extended-priv/mco_scale.gotest/extended-priv/util/vsphere.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended-priv/mco_controlplanemachineset.go
- test/extended-priv/mco_scale.go
| for _, fd := range gjson.Parse(failureDomains).Array() { | ||
| if fd.Get("server").String() == wsServer && fd.Get("topology.datacenter").String() == wsDataCenter { | ||
| return fd.Raw, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workspace fields available in MachineSet providerSpec usages.
rg -n --type=go -C 3 'providerSpec\.value\.workspace' test
# Inspect how failure domains are consumed elsewhere for matching criteria.
rg -n --type=go -C 5 'failureDomains' testRepository: openshift/machine-config-operator
Length of output: 3706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'machineset\.go|vsphere\.go' test | sed 's#^\./##'
echo
echo "== machineset GetVSphereFailureDomain and callers =="
sed -n '720,770p' test/extended-priv/machineset.go
echo
rg -n --type=go -C 4 'GetVSphereFailureDomain|VSphereConnectionInfoFromFailureDomain|failureDomains\[' test
echo
echo "== helper function =="
sed -n '420,485p' test/extended-priv/util/vsphere.goRepository: openshift/machine-config-operator
Length of output: 7189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vSphere Machine API schema/docs references in repo =="
rg -n -i "failureDomains|computeCluster|resourcePool|datastore|topology.datacenter" . \
-g '!vendor/**' -g '!docs/**' -g '!build/**' -g '!dist/**' \
| head -n 200
echo
echo "== JSON examples with multiple failure domains or matching topology =="
rg -n -C 4 'failureDomains' . \
-g '*.yaml' -g '*.yml' -g '*.json' -g '!vendor/**' -g '!build/**' -g '!dist/**' \
| head -n 250Repository: openshift/machine-config-operator
Length of output: 7706
Don’t match vSphere failure domains by server and datacenter only.
The workspace contains datastore and resourcePool, but this lookup compares only .spec.template.spec.providerSpec.value.server/datacenter to failureDomain.Topology.Datacenter. Two failure domains can share the same server/datacenter but use different datastore/resourcePool values, so GetVSphereConnectionInfoForMachineSet() may construct connection info from the wrong domain. Match using failureDomain.Topology.Datastore and failureDomain.Topology.ResourcePool from the workspace as well, or require a more specific workspace key unique to the failure domain.
🤖 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/machineset.go` around lines 757 - 761, The failure-domain
lookup in GetVSphereConnectionInfoForMachineSet must not match only server and
datacenter; also compare the workspace’s datastore and resourcePool values
against the corresponding failure-domain topology fields, or use an equivalent
workspace key uniquely identifying the domain. Preserve returning fd.Raw only
for a fully matching failure domain.
There was a problem hiding this comment.
Will be fixed here https://redhat.atlassian.net/browse/OCPBUGS-105430
There was a problem hiding this comment.
@sergiordlr, acknowledged. I will keep this finding open for this pull request. The follow-up is tracked in OCPBUGS-105430.
You are interacting with an AI system.
There was a problem hiding this comment.
will be addressed in https://redhat.atlassian.net/browse/OCPBUGS-105430
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| o.Expect(machineSet.SetCoreOsBootImage(backdatedImageName)).To(o.Succeed(), | ||
| "Error patching the value of the coreos boot image in %s", machineSet) | ||
| logger.Infof("OK!\n") | ||
|
|
||
| exutil.By("Check that the MachineSet is updated by MCO for opt-in") | ||
| o.Eventually(machineSet.GetCoreOsBootImage, "3m", "20s").ShouldNot(o.Equal(fakeImageName), | ||
| "The machineset should not be updated by MCO if the functionality is not enabled in the MachineConfiguration resource. %s", machineSet.PrettyString()) | ||
| CheckCurrentOSImageIsUpdated(machineSet, backdatedImageName) | ||
| logger.Infof("OK!\n") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
On vSphere this opt-in phase can pass without any MCO action.
Line 80 already lets the MCO update the template in place. The template keeps the name backdatedImageName, and its release is now the current release. Line 106 sets the same name again, so CheckCurrentOSImageIsUpdated at line 111 reads the already-updated release and passes immediately. The opt-in behavior is then not exercised on vSphere.
Re-upload the backdated OVA before this phase, or use a second backdated template name for the opt-in check.
🤖 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_bootimages.go` around lines 106 - 112, Update the
opt-in phase around SetCoreOsBootImage and CheckCurrentOSImageIsUpdated to use a
genuinely backdated template: either re-upload the backdated OVA before setting
the image or assign a distinct backdated template name. Ensure the check
observes an MCO-triggered update rather than an already-current vSphere
template.
There was a problem hiding this comment.
Will be fixed here
https://redhat.atlassian.net/browse/OCPBUGS-105431
There was a problem hiding this comment.
@sergiordlr Acknowledged. OCPBUGS-105431 will track the vSphere opt-in test correction. The review comment remains open until that change is implemented.
You are interacting with an AI system.
There was a problem hiding this comment.
will be addressed in https://redhat.atlassian.net/browse/OCPBUGS-105431
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
/lgtm /verified by e2es |
openshift-ci-robot
added
the
verified
|
@djoshy: 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. |
|
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: djoshy, 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 |
|
/retitle NO-ISSUE: Adapt vsphere bootimage tests for mult-vcenter scenarios |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sergiordlr: This pull request explicitly references no jira issue. 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. |
|
This fix needs to be carried as far as https://redhat.atlassian.net/browse/OCPBUGS-92182 is backported(4.20). Since this is not attached to a jira card, my plan is to carry this along with the backport for #6387. |
4 participants
- What I did
Modify the bootimages test cases so that in vsphere the machinesets use the right failure domain to inspect the templates.
Currently the vspehere bootimages logic in MCO have changed and the template is not updated to the canonical name anymore. Hence we removed the code validating that the images have to change when they are updated (in vsphere that's not true because the template is updated using the same name)
This behaviour in vsphere will change again when this other PR is merged #6354 and we will be able to check that when a template is updated the name changes.
- How to verify it
All tests should pass.
Summary by CodeRabbit
New Features
Bug Fixes