OCPBUGS-91668: vSphere machinepool: preserve old-style RHCOS template for upgraded clusters - #2921
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chdeshpa-hue: This pull request references HIVE-2846 which is a valid 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. |
|
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:
📝 WalkthroughWalkthroughvSphere failure-domain templates are backfilled from remote MachineSets and exempted from immutable-field checks. Vendored machine-api-operator, gcfg, warnings, and cloud-provider-vsphere metadata was pruned from the module and vendor listings. ChangesvSphere template flow
Dependency pruning
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chdeshpa-hue 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2921 +/- ##
==========================================
+ Coverage 50.39% 50.42% +0.02%
==========================================
Files 281 281
Lines 34367 34430 +63
==========================================
+ Hits 17320 17360 +40
- Misses 15695 15712 +17
- Partials 1352 1358 +6
🚀 New features to boost your workflow:
|
|
Thanks for working on this, @chdeshpa-hue Somehow your commit/PR got attached to an unrelated jira card. Would you please update both to reference https://redhat.atlassian.net/browse/OCPBUGS-91668 ? I don't love the idea of essentially porting code from an ancient version of installer to generate a very specific template format. There are several potential problems with this, including that we still need an answer for the case where that format isn't valid. While I appreciate that this only triggers if we find that template on an existing MachineSet, I'm not convinced the additional code maintenance burden is worth it. For instance, what if the matching MachineSet is itself bogus for that reason? And sure, we can add yet more code to check on that... but you see what I'm getting at: we're throwing increasingly messy code at this issue. In other actuators -- and in this one prior to the big zonal changes -- we try to copy the template from an existing master machine. However, that probably doesn't work here since the FD name is part of that template: we would need to find a master in the right FD, and I'm not sure that's always possible. I'm thinking the least-worst approach here may be to make it easier for the user to set the Template in the ClusterDeployment. We have precedent for easing our webhooks to allow setting certain values if previously unset, but still disallow edits if already set. That, plus some documentation (hive repo docs; a KCS article; something else that can easily be found by customers/TAMs) describing the issue and the remedy, would be clean and crisp, if not the ultimate ideal UX. WDYT? |
chdeshpa-hue
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@chdeshpa-hue: This pull request references Jira Issue OCPBUGS-91668, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Thanks for the detailed feedback @2uasimojo — appreciate you taking the time. Acknowledging your concernYou're right that the current implementation ports installer-era naming logic ( The workaround we've been providingWe've already been advising customers on exactly the manual approach you're describing. In OCPBUGS-91668, the documented workaround is:
spec:
platform:
vsphere:
infrastructure:
failureDomains:
- topology:
template: "{clusterID}-rhcos-generated-region-generated-zone"
# ... rest of failure domain configThis is safe on all MCE versions and prevents the issue regardless of Hive version. We would also prepare a KCS article documenting this workaround and the upgrade impact so it's discoverable through the support knowledge base. Why manual workaround alone isn't sufficientThat said, with the growing adoption of RHACM/Hive for vSphere
Two active SFDC cases are linked to this bug, which suggests the workaround documentation alone isn't catching the affected population in time. Revised code proposal: "Read, Don't Reconstruct"Rather than constructing any template name format, the simplified approach reads the template directly from existing remote MachineSet provider specs: What this changes:
Proposed path forward (two small PRs)
Both together give us defense in depth: the code catches it automatically for new MachinePool creates, and the webhook relaxation lets customers (or support) manually override if needed. The KCS article would document both paths. Happy to push the simplified diff and discuss further. |
|
Follow-up — tested locally: While validating the manual workaround path, I found that This applies whether the value is a bare template name or a full vSphere inventory path. This means for the affected population (existing clusters installed pre-Dec 2024, now on MCE 2.9+), there is currently no manual path to fix this — the ClusterDeployment already exists with an empty The auto-backfill code fix is unaffected (it operates on a |
|
Okay, I still don't love this, but I'll concede it's a lesser evil than forcing consumers to notice, discover, and remedy by hand. To summarize:
Thanks. |
da94488 to
e647cb0
Compare
|
Thanks @2uasimojo — confirmed on all three points. The force-push ( 1. FD-specific template matching: Correct — we cycle through remote MachineSets, decode each provider spec, and match by datacenter (same datacenter as the failure domain) + infraID prefix (template name starts with On "more than one template in a given FD": In practice, a single FD has one RHCOS template per cluster (the installer creates exactly one per failure domain at install time). However, if somehow two different templates existed for the same FD (e.g., a leftover from a failed install attempt), we'd pick the first one found in the remote MachineSet list. Since all existing MachineSets in a healthy cluster reference the same template for a given FD, this isn't a realistic failure mode — but the KCS will document it as an edge case with the manual override as the remedy. 2. Webhook relaxation: Included in this commit. 3. KCS article: In progress. Will cover: the failure mode (upgrade path that leaves Also — Jira target version is being set to 5.0.0 so |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/machinepool/vsphereactuator.go`:
- Around line 133-139: The template ownership check in the vSphere actuator is
too loose because `strings.HasPrefix(templateBaseName(spec.Template), infraID)`
can match another cluster with a longer infraID sharing the same prefix. Update
the check in the template selection logic to anchor on the installer template
naming pattern by matching `infraID + "-"` (or equivalent separator-based
prefix) before returning `spec.Template`, so only templates created for the
exact cluster are accepted.
In
`@pkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go`:
- Around line 896-897: The current
`cmpopts.IgnoreFields(installervsphere.Topology{}, "Template")` in the cluster
deployment validating hook is too broad because it hides all `Template` changes.
Update the immutability check in
`clusterdeployment_validating_admission_hook.go` to use conditional logic around
`Topology.Template`, so only an unset-to-set transition is allowed and any
change when the old value is already non-empty is still rejected. Prefer a
custom `cmp.FilterValues`/comparer or an explicit pre-check near the existing
`IgnoreFields` setup that preserves the diff unless `oldValue == ""`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d5eb6075-48b1-4a32-bb7f-51fda7d501a0
📒 Files selected for processing (4)
pkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/machinepool/machinepool_controller.go
| // Verify the template belongs to this cluster. Installer-generated templates are | ||
| // always prefixed with the cluster's infraID. This prevents picking up templates | ||
| // from other clusters sharing the same vCenter datacenter. | ||
| if !strings.HasPrefix(templateBaseName(spec.Template), infraID) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't think it's possible to get a different infra ID's Template because the source of the data is a list of remote MachineSets already retrieved from the target cluster. So I don't think we need this check (and coderabbit's comment below becomes n/a).
Furthermore, it is at least theoretically possible for the user to have built those MachineSets with a specified Template that doesn't have the infra ID in it at all, in which scenario this would have caused us not to grab it, resulting in the original failure mode.
@coderabbitai please confirm.
There was a problem hiding this comment.
@2uasimojo Confirmed — your reasoning is sound, and my earlier concern should be withdrawn.
Because a.remoteMachineSets are already sourced from the target cluster, the infraID prefix guard here does not add meaningful protection against selecting another cluster’s template. And as you noted, it can incorrectly reject a valid user-specified spec.Template that does not include the infra ID, which would preserve the original failure mode.
So yes: in this context, the strings.HasPrefix(templateBaseName(spec.Template), infraID) check looks unnecessary and potentially harmful.
(ᵔᴥᵔ)
✏️ Learnings added
Learnt from: 2uasimojo
Repo: openshift/hive PR: 2921
File: pkg/controller/machinepool/vsphereactuator.go:133-138
Timestamp: 2026-06-29T16:36:56.894Z
Learning: In `openshift/hive` PR `#2921`, for `pkg/controller/machinepool/vsphereactuator.go`, the `VSphereActuator.remoteMachineSets` input is already retrieved from the target cluster by the machinepool controller. When backfilling `FailureDomain.Topology.Template` from those remote MachineSets, do not assume the template name must be prefixed with the cluster infra ID; user-specified vSphere template names may legitimately omit the infra ID, so such a prefix check can block valid backfill and preserve the original provisioning failure.
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: 2uasimojo
Repo: openshift/hive PR: 2915
File: hack/modcheck.go:74-74
Timestamp: 2026-06-01T17:43:41.782Z
Learning: When reviewing Go code that calls `os.WriteFile(name, data, perm)`: note that the `perm` argument is only used if the file is created (i.e., when the underlying open uses an `O_CREATE`-style path). If `name` already exists, the existing file permissions are preserved by `open(2)` (on Linux), so `perm` will not change the mode. Therefore: (1) don’t rely on `perm` to update permissions for an existing file; (2) if the code passes a harmless `perm` value (e.g., `0`), verify via surrounding logic/guarantees that the file is already present—otherwise this can lead to incorrect permissions on initial creation.
| if spec.Workspace.Datacenter != fd.Topology.Datacenter { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Need to phone a friend here. @jcpowermac is Datacenter always sufficient to disambiguate two FDs? I thought we could have e.g. same Datacenter, different Datastores? Folders? Other?
There was a problem hiding this comment.
nope, vm-host zonal will always be the same datacenter. prior-4.22 zonal could be the same datacenter but isn't required.
really it should be the fd, region,zone names, those shouldn't be duplicated
e647cb0 to
d7d1911
Compare
|
v2: Addressing review feedback 1. Removed infraID prefix check (per @2uasimojo) 2. Webhook: spoof pattern for Template (per @2uasimojo) 3. Architecture: backfill as preprocessing (per Eric's suggestion) 4. MCO-style 5-field FD matching (per @jcpowermac)
The fd name does not appear in MachineSet metadata (name is 5. Known limitation: pre-corrupted state @jcpowermac — does the MCO-style 5-field match satisfy the FD matching concern? This is the same approach MCO landed in PR #5745 after fixing the identical class of bug. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/machinepool/vsphereactuator.go (1)
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftBackfill is decoupled from
GenerateMachineSets, relying on callers to invoke it first.
NewVSphereActuator/GenerateMachineSetshave no knowledge ofremoteMachineSetsorinfraID; the backfill only happens if the caller (createActuator) remembers to callbackfillVSphereTemplatesbeforehand and mutate the sharedFailureDomainsslice. Any future/alternate caller that constructs aVSphereActuatorand callsGenerateMachineSetsdirectly (as the existingTestVSphereActuatorunit test already does) will silently skip backfilling. Consider storingremoteMachineSets/infraIDon the actuator (mirroring the GCP actuator's constructor pattern) and invoking backfill from withinGenerateMachineSetsitself so the behavior is self-contained.Also applies to: 38-82
🤖 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 `@pkg/controller/machinepool/vsphereactuator.go` around lines 30 - 34, The backfill logic for vSphere templates is currently dependent on the caller mutating shared state before calling GenerateMachineSets, so direct callers can skip it. Update VSphereActuator to retain the needed remoteMachineSets and infraID from NewVSphereActuator (similar to the GCP actuator pattern), and move the backfill invocation into GenerateMachineSets itself so template backfilling always happens internally. Make sure the constructor and GenerateMachineSets use the existing VSphereActuator symbols consistently so the behavior is self-contained for tests and future callers.
🤖 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.
Nitpick comments:
In `@pkg/controller/machinepool/vsphereactuator.go`:
- Around line 30-34: The backfill logic for vSphere templates is currently
dependent on the caller mutating shared state before calling
GenerateMachineSets, so direct callers can skip it. Update VSphereActuator to
retain the needed remoteMachineSets and infraID from NewVSphereActuator (similar
to the GCP actuator pattern), and move the backfill invocation into
GenerateMachineSets itself so template backfilling always happens internally.
Make sure the constructor and GenerateMachineSets use the existing
VSphereActuator symbols consistently so the behavior is self-contained for tests
and future callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fcaf58e5-ae02-4b76-a3e8-a886ea45fbcd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
go.modpkg/controller/machinepool/machinepool_controller.gopkg/controller/machinepool/vsphereactuator.gopkg/controller/machinepool/vsphereactuator_test.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook.gopkg/validating-webhooks/hive/v1/clusterdeployment_validating_admission_hook_test.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/actuator.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/machine_scope.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/reconciler.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/session.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/tag_ids_caching_client.govendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/util.govendor/github.com/openshift/machine-api-operator/pkg/util/ipam/util.govendor/gopkg.in/gcfg.v1/LICENSEvendor/gopkg.in/gcfg.v1/READMEvendor/gopkg.in/gcfg.v1/doc.govendor/gopkg.in/gcfg.v1/errors.govendor/gopkg.in/gcfg.v1/read.govendor/gopkg.in/gcfg.v1/scanner/errors.govendor/gopkg.in/gcfg.v1/scanner/scanner.govendor/gopkg.in/gcfg.v1/set.govendor/gopkg.in/gcfg.v1/token/position.govendor/gopkg.in/gcfg.v1/token/serialize.govendor/gopkg.in/gcfg.v1/token/token.govendor/gopkg.in/gcfg.v1/types/bool.govendor/gopkg.in/gcfg.v1/types/doc.govendor/gopkg.in/gcfg.v1/types/enum.govendor/gopkg.in/gcfg.v1/types/int.govendor/gopkg.in/gcfg.v1/types/scan.govendor/gopkg.in/warnings.v0/LICENSEvendor/gopkg.in/warnings.v0/READMEvendor/gopkg.in/warnings.v0/warnings.govendor/k8s.io/cloud-provider-vsphere/LICENSEvendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_ini_legacy.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_yaml.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/consts_and_errors.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_common.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_ini_legacy.govendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_yaml.govendor/modules.txt
💤 Files with no reviewable changes (35)
- vendor/k8s.io/cloud-provider-vsphere/LICENSE
- vendor/gopkg.in/warnings.v0/README
- vendor/gopkg.in/gcfg.v1/README
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_common.go
- vendor/gopkg.in/gcfg.v1/types/scan.go
- vendor/gopkg.in/gcfg.v1/token/token.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_yaml.go
- vendor/gopkg.in/gcfg.v1/types/enum.go
- vendor/gopkg.in/warnings.v0/LICENSE
- vendor/gopkg.in/gcfg.v1/set.go
- vendor/gopkg.in/gcfg.v1/LICENSE
- vendor/gopkg.in/gcfg.v1/errors.go
- vendor/gopkg.in/gcfg.v1/types/bool.go
- vendor/gopkg.in/gcfg.v1/types/doc.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/types_ini_legacy.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/actuator.go
- vendor/gopkg.in/gcfg.v1/types/int.go
- vendor/gopkg.in/gcfg.v1/doc.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/reconciler.go
- vendor/gopkg.in/gcfg.v1/scanner/scanner.go
- vendor/gopkg.in/gcfg.v1/read.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/session.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/consts_and_errors.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/util.go
- vendor/gopkg.in/gcfg.v1/scanner/errors.go
- vendor/gopkg.in/gcfg.v1/token/serialize.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_yaml.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config_ini_legacy.go
- vendor/gopkg.in/warnings.v0/warnings.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/session/tag_ids_caching_client.go
- vendor/k8s.io/cloud-provider-vsphere/pkg/common/config/config.go
- vendor/github.com/openshift/machine-api-operator/pkg/util/ipam/util.go
- vendor/gopkg.in/gcfg.v1/token/position.go
- vendor/github.com/openshift/machine-api-operator/pkg/controller/vsphere/machine_scope.go
- vendor/modules.txt
| } | ||
|
|
||
| func testVSphereMachineSpec(machineType string) machineapi.MachineSpec { | ||
| rawVSphereProviderSpec, err := vsphereutil.RawExtensionFromProviderSpec(testVSphereProviderSpec()) |
There was a problem hiding this comment.
IIUC inlining this func is what got rid of ~7KLOC worth of vendored files. Cool 👍
| // Derive expected VMGroup: only host-group zonal FDs have a VMGroup set on their MachineSets. | ||
| vmGroup := "" | ||
| if fd.ZoneType == installervsphere.HostGroupFailureDomain { | ||
| vmGroup = fmt.Sprintf("%s-%s", infraID, fd.Name) |
There was a problem hiding this comment.
Is this pattern reliable? The whole reason we're here is because such a pattern changed upstream.
| case cd.Spec.Platform.OpenStack != nil: | ||
| return NewOpenStackActuator(masterMachine, r.Client, logger) | ||
| case cd.Spec.Platform.VSphere != nil: | ||
| if cd.Spec.Platform.VSphere.Infrastructure != nil && cd.Spec.ClusterMetadata != nil { |
There was a problem hiding this comment.
I don't agree with positioning this in the actuator creation flow. It's especially problematic that we're modifying the cd here, as nothing guarantees something downstream won't Update() it, resulting in unintended side effects (though arguably "good" ones in some cases).
The move appears to be related to:
3. Architecture: backfill as preprocessing (per Eric's suggestion)
Moved template backfill out of the actuator struct. A standalonebackfillVSphereTemplates()function enriches the in-memory install config in the controller before actuator creation. The actuator stays stateless about remote cluster state (aligned with GCP's scalar-extraction pattern and Nutanix/OpenStack actuators).
I don't think I made such a suggestion -- can you point to it? If I did, I must have worded it poorly :)
The part about "in-memory install config" is valid... but we don't have that yet here. But "before actuator creation" is something I wouldn't have suggested intentionally.
TL;DR: This logic should live in the vsphere actuator, modifying the in-memory install-config via the copy of the Infrastructure from the CD.
| if spec.Workspace.Datacenter != fd.Topology.Datacenter || | ||
| spec.Workspace.Datastore != fd.Topology.Datastore || | ||
| spec.Workspace.Server != fd.Server || | ||
| spec.Workspace.VMGroup != vmGroup || | ||
| path.Clean(spec.Workspace.ResourcePool) != path.Clean(expectedRP) { |
There was a problem hiding this comment.
@jcpowermac Please vet?
For reference: https://github.com/openshift/machine-config-operator/pull/5723/changes
In particular, I can't tell if the computation of vmGroup is valid.
| } | ||
|
|
||
| // Allow setting Topology.Template on failure domains where it was previously blank. | ||
| // This permits the machinepool controller's backfill to persist and allows customers |
There was a problem hiding this comment.
See, this implies that we're deliberately/explicitly updating the CD after the backfill, which a) I don't see; and b) we shouldn't do.
| if oldObject.Spec.Platform.VSphere != nil && oldObject.Spec.Platform.VSphere.Infrastructure != nil && | ||
| cd.Spec.Platform.VSphere != nil && cd.Spec.Platform.VSphere.Infrastructure != nil { |
There was a problem hiding this comment.
nit: Could fold this into the block above where we've already checked that old/new have Spec.Platform.VSphere.
|
Thanks @2uasimojo — working through each point. 1. Architecture: backfill positioning (machinepool_controller.go:1396) You're right, and it's actually worse than the mutation risk you flagged — The root cause of the design mistake: we modeled The correct pattern is the GCP actuator ( Will refactor to match: 2. VMGroup computation (vsphereactuator.go:115, 140) Waiting for @jcpowermac's review on this. 3. Attribution Apologies for the misattribution — that was my error in summarizing the review thread. Will correct in the next push description. 4. Webhook nit (clusterdeployment_validating_admission_hook.go:715) Moot — the webhook change will be dropped entirely with the architecture fix above. |
|
I don't think we should drop the webhook relaxation change. The backfill can still be invalid if the existing remote MachineSet we happen to choose is already invalid. |
d7d1911 to
c70960f
Compare
Clusters installed before the HIVE-2391 template naming change have empty
Topology.Template fields in their ClusterDeployment. When the machinepool
controller generates new MachineSets, Infrastructure.DeepCopy() preserves
the empty Template, causing the installer to generate a new-format name
that does not exist on the vCenter.
Fix: follow the GCP actuator pattern — NewVSphereActuator preprocesses
remoteMachineSets into a fd-name→template map at construction time, then
GenerateMachineSets applies it to the DeepCopy'd install-config. The
ClusterDeployment is never mutated.
Matching uses a 5-field workspace conjunction (Datacenter, Datastore,
Server, VMGroup, ResourcePool) mirroring MCO PR #5745. VMGroup is derived
as {infraID}-{fdName} only for host-group zonal failure domains.
Additionally relaxes the ClusterDeployment webhook to allow setting
Topology.Template when previously blank, using the established spoof
pattern (6 existing instances). This permits customers to set the template
explicitly and avoids rejecting updates when the backfill-sourced template
from a remote MachineSet is itself invalid.
Removes the machine-api-operator vendor dependency (~7KLOC) by inlining
the VSphereMachineProviderSpec unmarshalling with encoding/json.
Assisted-by: Claude Code
Co-authored-by: Cursor <cursoragent@cursor.com>
c70960f to
29791fd
Compare
|
v3: Addressing review feedback (force-push 1. Architecture: backfill moved into the actuator (machinepool_controller.go:1396) You're right — the prior version mutated
2. Webhook: folded into existing vSphere block (line 715 nit) The 3. Webhook relaxation kept (per your July 6 comment) Confirmed — the spoof pattern stays. If the backfill reads an invalid template from a corrupted remote MachineSet, customers can still manually set the correct value via CD edit. 4. ResourcePool override bug fix Found during code review: Still pending: @jcpowermac vetting of the vmGroup computation + 5-field match approach (from your July 1 request). |
|
@chdeshpa-hue: 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. |
2uasimojo
left a comment
2uasimojo
left a comment
There was a problem hiding this comment.
This is starting to look sane. I'm still concerned about the hardcoded vmgroup format string. Is there a more generic/future-proof way to handle this?
| // Derive expected VMGroup: only host-group zonal FDs have a VMGroup set on their MachineSets. | ||
| vmGroup := "" | ||
| if fd.ZoneType == installervsphere.HostGroupFailureDomain { | ||
| vmGroup = fmt.Sprintf("%s-%s", infraID, fd.Name) |
|
Thanks @2uasimojo — on the future-proofing question specifically: You're right that That said, I agree reconstructing the formula in Hive isn't ideal if the contract moves again. A more generic option would be to stop building the string and instead read the expected VMGroup from the remote Happy to take that follow-up if you prefer it over the reconstruct. Leaving the "is this pattern durable?" question for @jcpowermac as tagged. |
4 participants
Problem
After upgrading to MCE 2.9+ (ACM 2.14+ / OCP 4.19+), new Hive MachinePool objects for existing vSphere clusters generate MachineSets that reference an RHCOS template that does not exist in vCenter.
Root cause chain:
Installer commit (Dec 2024): Changed vSphere
MachineSets()to callGenerateVSphereTemplateName(clusterID, failureDomain.Name)— template name format changed from{infraID}-rhcos-{region}-{zone}to{infraID}-rhcos-{fdName}.HIVE-2391 (PR HIVE-2391: vsphere zonal #2851, Feb 2026): Rewrote
VSphereActuatorto useInfrastructure.DeepCopy(), removing the explicitosImage/Topology.Templatebackfill.FailureDomain.Topology.Templateis now empty by default.Combined effect: On clusters installed before Dec 2024, the template
{infraID}-rhcos-{region}-{zone}exists in vCenter but Hive now requests{infraID}-rhcos-{fdName}— which doesn't exist → machine provisioning fails.Workaround (customer): Avoid creating new MachinePools; only scale existing MachinePools. Or manually set
Topology.Templatein the ClusterDeployment.Fix
In
GenerateMachineSets, after building theInstallConfigfromInfrastructure.DeepCopy(), check each failure domain with an emptyTopology.Template. Construct the old-style template name{infraID}-rhcos-{region}-{zone}and scan existing remote MachineSets for a reference to it. If found, setTopology.Templateto the old-style name, letting the installer use it directly via the existingif failureDomain.Topology.Template == ""escape hatch.The fix is strictly additive and conservative:
encoding/jsonlocally instead of re-adding themachine-api-operatordependencyBefore/After
Before (MCE 2.9 on a pre-2024 cluster):
After (with fix):
Generated MachineSet references
{infraID}-rhcos-generated-region-generated-zone— the existing template.Test Evidence
Files Changed
pkg/controller/machinepool/vsphereactuator.go— core fix (backfill logic + helpers)pkg/controller/machinepool/machinepool_controller.go— passremoteMachineSetsto constructorpkg/controller/machinepool/vsphereactuator_test.go— 2 new regression tests + template assertionNote on PR #2884 (HIVE-3134)
This PR is open and removes the
machine-api-operatordependency fromvsphereactuator.go. Our fix does NOT re-add that dependency — it inlines the sameproviderSpecFromRawExtension()function body usingencoding/json. When HIVE-3134 merges, there will be no conflict.Made with Cursor
Summary by CodeRabbit
Release Notes
Bug Fixes
Topology.Templatevalues from compatible remote machine sets, while preserving any user-supplied templates.Tests
Chores