Skip to content

HIVE-2391: vSphere zonal support #2731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dlom wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HIVE-2391: vSphere zonal support #2731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions apis/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ toolchain go1.24.6

require (
github.com/openshift/api v0.0.0-20251120220512-cb382c9eaf42
github.com/openshift/installer v1.4.21-pre2.0.20260112230456-1c2444827f23
k8s.io/api v0.34.2
k8s.io/apimachinery v0.34.2
)
Expand All @@ -18,10 +19,6 @@ require (
github.com/kr/text v0.2.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
github.com/rogpeppe/go-internal v1.14.1 // indirect
github.com/spf13/pflag v1.0.10 // indirect
github.com/stretchr/testify v1.11.1 // indirect
github.com/x448/float16 v0.8.4 // indirect
go.yaml.in/yaml/v2 v2.4.3 // indirect
golang.org/x/net v0.47.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions apis/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee h1:W5t00kpgFd
github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk=
github.com/openshift/api v0.0.0-20251120220512-cb382c9eaf42 h1:Mo2FlDdoCZ+BE2W4C0lNcxEDeIIhfsYFP6vj4Sggp8w=
github.com/openshift/api v0.0.0-20251120220512-cb382c9eaf42/go.mod h1:d5uzF0YN2nQQFA0jIEWzzOZ+edmo6wzlGLvx5Fhz4uY=
github.com/openshift/installer v1.4.21-pre2.0.20260112230456-1c2444827f23 h1:yoi8j9RmeCOKPolpYfNIRiO6NE7xN5Q1+UpS6GfOhtE=
github.com/openshift/installer v1.4.21-pre2.0.20260112230456-1c2444827f23/go.mod h1:3ATaLOa+jMDvA30gkrTskppvkggXmtKHtMWQ1Sog6kA=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U=
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
Expand Down
7 changes: 5 additions & 2 deletions apis/hive/v1/clusterdeprovision_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,11 @@ type VSphereClusterDeprovision struct {
// CertificatesSecretRef refers to a secret that contains the vSphere CA certificates
// necessary for communicating with the VCenter.
CertificatesSecretRef corev1.LocalObjectReference `json:"certificatesSecretRef"`
// VCenter is the vSphere vCenter hostname.
VCenter string `json:"vCenter"`
// DeprecatedVCenter is the vSphere vCenter hostname.
// Deprecated: use VCenters instead
DeprecatedVCenter string `json:"vCenter"`
Copy link
Member

@2uasimojo 2uasimojo Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it happens to be functionally irrelevant since this isn't a pointer, but adding +optional would at least make this field disappear from the required list in the schema, as well as being visually pro forma for a field we're deprecating.

Since this CRD is only generated by hive, I would expect any external consumers would be read-only. I wonder if we should describe how new code will behave to give consumers a hint as to how to change their code when they see this. Namely, it looks like we're going to only use the new thing for new instances, but not try to convert legacy instances in the field. Thus consumers would need to check both, like we do.

Copy link
Contributor

@jianping-shu jianping-shu Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it means a test of hive upgrade is needed? I didn't do it yet.

Copy link
Member

@2uasimojo 2uasimojo Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, we should definitely have a test case where we

  • start with an old hive build (prior to this change)
  • start a deprovision, but don't let it finish (this could be tricky, but we could probably do it by borking the creds prior to deleting the CD)
  • upgrade hive past this change
  • (restore proper creds and) validate that the deprov completes successfully

Other more "normal" upgrade-y scenarios would also be useful. Install CD with old hive, upgrade, and validate that MachinePool (auto)scaling and deprov flows all work as expected. This would have some functional overlap with your existing scenarios that use the legacy install-config and MP shapes, but is probably worth a separate test suite.

Copy link
Contributor

@jianping-shu jianping-shu Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add these 2 test cases.

// VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter.
VCenters []string `json:"vCenters"`
}

// IBMClusterDeprovision contains IBM Cloud specific configuration for a ClusterDeprovision
Expand Down
32 changes: 12 additions & 20 deletions apis/hive/v1/vsphere/machinepools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,25 @@
package vsphere

import "github.com/openshift/installer/pkg/types/vsphere"

// MachinePool stores the configuration for a machine pool installed
// on vSphere.
type MachinePool struct {
vsphere.MachinePool `json:",inline"`

// ResourcePool is the name of the resource pool that will be used for virtual machines.
// If it is not present, a default value will be used.
// Deprecated: use Topology instead
// +optional
ResourcePool string `json:"resourcePool,omitempty"`

// NumCPUs is the total number of virtual processor cores to assign a vm.
NumCPUs int32 `json:"cpus"`

// NumCoresPerSocket is the number of cores per socket in a vm. The number
// of vCPUs on the vm will be NumCPUs/NumCoresPerSocket.
NumCoresPerSocket int32 `json:"coresPerSocket"`

// Memory is the size of a VM's memory in MB.
MemoryMiB int64 `json:"memoryMB"`

// OSDisk defines the storage for instance.
OSDisk `json:"osDisk"`
DeprecatedResourcePool string `json:"resourcePool,omitempty"`

// TagIDs is a list of up to 10 tags to add to the VMs that this machine set provisions in vSphere.
// Deprecated: use Topology instead
// +kubebuilder:validation:MaxItems:=10
TagIDs []string `json:"tagIDs,omitempty"`
}
DeprecatedTagIDs []string `json:"tagIDs,omitempty"`

// OSDisk defines the disk for a virtual machine.
type OSDisk struct {
// DiskSizeGB defines the size of disk in GB.
DiskSizeGB int32 `json:"diskSizeGB"`
// Topology is the vSphere topology that will be used for virtual machines.
// If it is not present, a default value will be used.
// +optional
Topology *vsphere.Topology `json:"topology,omitempty"`
Copy link
Member

@2uasimojo 2uasimojo Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously we were getting the topology from the CD. I assumed this was:

  • because it's the cluster's topology, the foundation on which you can place Machines, which you stuff into a subset of the available zones via Zones.
  • so you don't have to repeat it for every MachinePool.

IIUC by including this field you're providing a way for a specific MachinePool to override the topology settings for a given FD when producing the mset for that FD. Are there legit, supported use cases for that? Is it only to provide backward compatibility for ResourcePool and TagIDs? If that's the case, we would probably need to make this a map, keyed by zone...

Copy link
Contributor

@jianping-shu jianping-shu Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the topology can be used to filter out all zones matched.
Pls. see scenario "Create infra6 MP" of Case 7 of OCP-84265.
When configures "hostGroup: sgao-host1-zone" in MP, I think it shall match up with FD sgao-host1-zone in theory, but the test result shows still 2 machinesets generated, 1 in sgao-host1-zone and 1 in sgao-host2-zone.
Seems it was used neither for matching nor for overriding.

Copy link
Member

@2uasimojo 2uasimojo Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test result shows still 2 machinesets generated, 1 in sgao-host1-zone and 1 in sgao-host1-zone.

Assume this is a typo; one was sgao-host2-zone?

Looking at the code, I think you would need to include FD.ZoneType: "HostGroup" for the VMGroup field to get a non-default value... but AFAICS HostGroup is only used day 0; it's ignored in the MachineSet generation paths.

I was thinking the topology can be used to filter out all zones matched.

Zones are always/only matched based on fd.name == mp.spec.platform.vsphere.zones[i].

We need to dig deeper here and figure out if there are any fields in topology that can legitimately override their counterparts in the fd. And even if they can, should they?

I think we've already seen that at least a subset of the fields can't be overridden; and the fact that they're required by the schema makes this awkward and annoying.

I'm thinking for the sake of simplicity, we should

  • Remove the topology section from the MachinePool for this epic.
  • Leave ResourcePool and TagIDs in place as overrides for all machines in the pool (assuming overriding them is effective).

If we get customers asking for topology things to be per-mset tunable later on, we can add topology back in... or do something else, if something else makes more sense (which it likely would, given the awkwardness of the schema requirements). Whereas if we commit to this shape now, it's really hard to walk it back if we didn't get it right.

My guess: YAGNI :)

Copy link
Contributor

@jianping-shu jianping-shu Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume this is a typo; one was sgao-host2-zone?
Yes, typo. Just corrected it.

}
34 changes: 28 additions & 6 deletions apis/hive/v1/vsphere/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
package vsphere

import (
"github.com/openshift/installer/pkg/types/vsphere"
corev1 "k8s.io/api/core/v1"
)

// Platform stores any global configuration used for vSphere platforms.
type Platform struct {
// Infrastructure is the desired state of the vSphere infrastructure provider.
Infrastructure *vsphere.Platform `json:"infrastructure,omitempty"`

// VCenter is the domain name or IP address of the vCenter.
VCenter string `json:"vCenter"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedVCenter string `json:"vCenter,omitempty"`

// CredentialsSecretRef refers to a secret that contains the vSphere account access
// credentials: GOVC_USERNAME, GOVC_PASSWORD fields.
Expand All @@ -18,18 +25,33 @@ type Platform struct {
CertificatesSecretRef corev1.LocalObjectReference `json:"certificatesSecretRef"`

// Datacenter is the name of the datacenter to use in the vCenter.
Datacenter string `json:"datacenter"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedDatacenter string `json:"datacenter,omitempty"`

// DefaultDatastore is the default datastore to use for provisioning volumes.
DefaultDatastore string `json:"defaultDatastore"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedDefaultDatastore string `json:"defaultDatastore,omitempty"`

// Folder is the name of the folder that will be used and/or created for
// virtual machines.
Folder string `json:"folder,omitempty"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedFolder string `json:"folder,omitempty"`

// Cluster is the name of the cluster virtual machines will be cloned into.
Cluster string `json:"cluster,omitempty"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedCluster string `json:"cluster,omitempty"`

// Network specifies the name of the network to be used by the cluster.
Network string `json:"network,omitempty"`
// Deprecated: Please use Platform.Infrastructure instead
// See also: Platform.ConvertDeprecatedFields
// +optional
DeprecatedNetwork string `json:"network,omitempty"`
}
36 changes: 17 additions & 19 deletions apis/hive/v1/vsphere/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions apis/hive/v1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading