SPLAT-1733: Update vSphere and include host/vm group #1677

jcpowermac · 2024-09-05T20:16:47Z

Changes

Update the vSphere installer IPI zonal enhancement to include:

current state of zonal
future state of zonal with vm-host group additions

Additional PRs

openshift-ci · 2024-09-05T20:16:53Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-09-05T20:19:10Z

@jcpowermac: This pull request references SPLAT-1733 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 task to target the "4.18.0" version, but no target version was set.

In 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.

openshift-ci-robot · 2024-09-23T17:10:45Z

@jcpowermac: This pull request references SPLAT-1733 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 "4.18.0" version, but no target version was set.

In response to this:

Update the vSphere installer IPI zonal enhancement to include:

current state of zonal

future state of zonal with vm-host group additions

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.

openshift-ci-robot · 2024-09-23T17:11:56Z

@jcpowermac: This pull request references SPLAT-1733 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 "4.18.0" version, but no target version was set.

In response to this:

Changes

Update the vSphere installer IPI zonal enhancement to include:

current state of zonal

future state of zonal with vm-host group additions

Additional PRs

SPLAT-1742: vSphere - enable host group based zonal installer#8873

WIP - SPLAT-1781: api bump for host zonal infra change client-go#294

WIP - SPLAT-1780: api bump for host zonal infra change library-go#1782

SPLAT-1799: Add support for vSphere host and vm group based zonal cluster-control-plane-machine-set-operator#325

SPLAT-1800: Add support for vSphere host and vm group based zonal machine-api-operator#1285

SPLAT-1743: vSphere - add host and vm based zonal api#1999

SPLAT-1733: Update vSphere and include host/vm group #1677

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.

jcpowermac · 2024-09-23T19:50:07Z

/assign @rvanderp3 @vr4manta

jcpowermac · 2024-09-23T19:50:20Z

/cc @gnufied @JoelSpeed

JoelSpeed · 2024-09-24T16:49:23Z

enhancements/installer/vsphere-ipi-zonal.md

+)
+
+// VSpherePlatformFailureDomainSpec holds the region and zone failure domain and the vCenter topology of that failure domain.
+// +kubebuilder:validation:XValidation:rule="has(self.zoneType) && self.zoneType == 'HostGroup' ? self.topology.affinity.vmGroup != '' && self.topology.affinity.hostGroup != '' && self.topology.affinity.vmHostRule != '' : true",message="when zoneType is HostGroup, failuredomain topology affinity vmGroup, hostGroup and vmHostRule fields must be defined"


Topology does not appear in the spec here, I think it may have been meant to be there though?

This validation apppears that you want an optional struct, with required fields in for the zoneType, is this just a discriminated union? It would then have the standard that hostGroup is allowed if and only if type == HostGroup

region: type: ABC zone: type: HostGroup hostGroup: vmGroup: ... hostGroup: ... vmHostRule: ...

I broke out what is current (as of 9/9/24) and what is specifically changing for vm-host zonal. Rather than duplicating these are snipits of what would change. Would you rather have that as a single section?

personally - it makes sense to put new changes into existing structs, rather than breaking them out. This way, we can still see what changed via diff, but we can also see the full datatype if we view complete file.

@JoelSpeed does this look like the direction you are thinking? 677ce07

or something like this: fdb3dde

Since there is a name collision with region and zone, wondering if it should be moved to the topology struct

The latter of the two commits is where I was thinking to go

whew thats what I ended up with

openshift/api@906bb28

The enhancement has been updated for that

ea2162a

JoelSpeed · 2024-09-24T16:50:49Z

enhancements/installer/vsphere-ipi-zonal.md

+// This configuration within vCenter creates the required association between a failure domain, virtual machines
+// and ESXi hosts to create a vm-host based zone.
+type VSphereFailureDomainAffinity struct {
+	// VMGroupName is the name of the vm-host group of type virtual machine within vCenter for this failure domain.


Should be vmGroup at the beginning of this godoc

JoelSpeed · 2024-09-24T16:55:28Z

enhancements/installer/vsphere-ipi-zonal.md

+
+// Host defines host VMs to generate as part of the installation.
+type Host struct {
+	// FailureDomain refers to the name of a FailureDomain as described in https://github.com/openshift/enhancements/blob/master/enhancements/installer/vsphere-ipi-zonal.md


Probably want to explain this in more depth in situ rather than referring out to a doc

@JoelSpeed first time updating a enhancement. This is what is current in the ~~infra~~ installer's platform spec, so are you expecting this to be updated? I suppose that should really be a bug right?

Didn't realise this was the current state, we should probably look to update these where they are non-breaking, can you create a bug?

JoelSpeed · 2024-09-24T16:56:12Z

enhancements/installer/vsphere-ipi-zonal.md

+	// NetworkDeviceSpec to be applied to the host
+	// +kubebuilder:validation:Required
+	NetworkDevice *NetworkDeviceSpec `json:"networkDevice"`
+	// Role defines the role of the node


Please expand the godoc here, what do each of these values mean and what effect does it have when I set them?

Same as above, this is what is current.

JoelSpeed · 2024-09-24T16:56:45Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +kubebuilder:validation:Format=ipv4
+	// +kubebuilder:validation:Format=ipv6


Suggested change

// +kubebuilder:validation:Format=ipv4

// +kubebuilder:validation:Format=ipv6

// +kubebuilder:validation:XValidation:rule="isIP(self)",message="gateway must be a valid IPv4 or IPv6 address"

JoelSpeed · 2024-09-24T16:58:05Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +kubebuilder:example=`192.168.1.100/24`
+	// +kubebuilder:example=`2001:DB8:0000:0000:244:17FF:FEB6:D37D/64`
+	// +kubebuilder:validation:Required
+	IPAddrs []string `json:"ipAddrs"`


You'll have to define a maximal length for this

JoelSpeed · 2024-09-24T16:58:14Z

enhancements/installer/vsphere-ipi-zonal.md

+	// ipAddrs is a list of one or more IPv4 and/or IPv6 addresses and CIDR to assign to
+	// this device, for example, 192.168.1.100/24. IP addresses provided via ipAddrs are


Which device?

JoelSpeed · 2024-09-24T16:58:36Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +kubebuilder:validation:Format=ipv4
+	// +kubebuilder:validation:Format=ipv6


Suggested change

// +kubebuilder:validation:Format=ipv4

// +kubebuilder:validation:Format=ipv6

// +kubebuilder:validation:XValidation:rule="isIP(self)",message="gateway must be a valid IPv4 or IPv6 address"

JoelSpeed · 2024-09-24T16:58:47Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +kubebuilder:validation:Format=ipv4
+	// +kubebuilder:validation:Format=ipv6
+	// +kubebuilder:example=`8.8.8.8`
+	Nameservers []string `json:"nameservers,omitempty"`


Should add a maximal length to this list as well

JoelSpeed · 2024-09-24T16:59:57Z

enhancements/installer/vsphere-ipi-zonal.md

+}
+
+
+type Topology struct {


Where is this struct included?

Similar to the infra spec changes, just showing snipits of what would actually change. The current topology struct is above.

openshift-ci · 2024-09-24T19:24:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from rvanderp3. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gnufied · 2024-09-24T19:30:00Z

enhancements/installer/vsphere-ipi-zonal.md

 type VSpherePlatformSpec struct {
 	// vcenters holds the connection details for services to communicate with vCenter.
-	// Currently, only a single vCenter is supported.
+	// Currently, only a single vCenter is supported, but in tech preview 3 vCenters are supported.


I thought one of the epics in 4.18 is to remove techpreview status from multi-vcenter support?

I just copied and pasted what the current infra spec is as of 9/9 . afaik it is supposed to be ga'ed in 4.18 but maybe those updates have not been added yet.

cc: @vr4manta

makes sense to merge the logic in an overall doc, but while in flux, I do see the complication of multi vcenter not being here as well. Currently 4.17 it is feature gated. Once its all finalized, should be GA. Goal for GA is 4.18.

gnufied · 2024-09-24T19:35:44Z

enhancements/installer/vsphere-ipi-zonal.md

+	// must use URN-notation instead of display names. A maximum of 10 tag IDs may be specified.
+	// +kubebuilder:example=`urn:vmomi:InventoryServiceTag:5736bf56-49f5-4667-b38c-b97e09dc9578:GLOBAL`
+	// +optional
+	TagIDs []string `json:"tagIDs,omitempty"`


How do we intend to use this?

This was added in 4.16 I believe. It allows a compute machine to be tagged. I think it was specifically for backup processes that customers wanted it for.

gnufied · 2024-09-24T19:37:11Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +optional
+	LoadBalancer *configv1.VSpherePlatformLoadBalancer `json:"loadBalancer,omitempty"`
+	// Hosts defines network configurations to be applied by the installer. Hosts is available in TechPreview.
+	Hosts []*Host `json:"hosts,omitempty"`


Is this intended to support hosts as zones? What is the use case I wonder.

Pretty sure the answer is yes, afaik it for static ip.

@vr4manta @rvanderp3 do you want to take that one?

But this is not reflected in Infrastructure type above. So information will be lost after cluster installation?

oh I think I misunderstood, Hosts is for OCP hosts and static ip, that isn't ESXi hosts.

gnufied · 2024-09-24T19:42:47Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +openshift:validation:featureGate=VSphereHostVMGroupZonal
+	// +kubebuilder:validation:MaxLength=80
+	// +optional
+	VMGroup string `json:"vmGroup,omitempty"`


Will this be created if it doesn't exist already?

the installer will create the vmGroup, one per failure domain

gnufied · 2024-09-24T19:42:58Z

enhancements/installer/vsphere-ipi-zonal.md

+	// +openshift:validation:featureGate=VSphereHostVMGroupZonal
+	// +kubebuilder:validation:MaxLength=80
+	// +optional
+	HostGroup string `json:"hostGroup,omitempty"`


same as above.

The installer will not create the hostGroup, that would be kinda hard since we would need to know each esxi host per zone.

gnufied · 2024-09-24T19:48:28Z

enhancements/installer/vsphere-ipi-zonal.md

+// and the vm-host affinity rule that together creates a affinity configuration for vm-host based zonal. 
+// This configuration within vCenter creates the required association between a failure domain, virtual machines
+// and ESXi hosts to create a vm-host based zone.
+type VSphereFailureDomainAffinity struct {


So, the thing - we will have to see is, if a customer specifies computeCluster as region and then specifies two different hostGroup as zones and assuming those hostGroups don't share the datastore - will CSI driver work...

that scenario was just asked in slack the other day so yeah should be a concern.

jcpowermac · 2024-09-26T17:27:59Z

The enhancement has been updated to include current and future states for both the infrastructure and installer's platform spec. Hopefully that will make it more clear.

gnufied · 2024-09-26T17:39:42Z

enhancements/installer/vsphere-ipi-zonal.md

@@ -180,6 +219,57 @@ type VSpherePlatformTopology struct {
 	// +kubebuilder:validation:Pattern=`^/.*?/vm/.*?`
 	// +optional
 	Folder string `json:"folder,omitempty"`
+
+	// template is the full inventory path of the virtual machine or template


If hosts are being used as zones, how will that information reflected in Infrastructure type?

The infrastructure spec will have the name of the vm-host group of type hosts

We will not know the ESXi hosts directly that are in each zone. I don't think we should either since stretched clusters presumably should already have this defined.

But then, it sounds like customer must create a vm-host group for each host? I must be misunderstanding something. The CSI driver doesn't support hostgroup as a failure-domain fwiw.

Nope, and maybe that illustrates something that I am missing.

I expect the customer to create a vm-host group (host type) for each zone prior to installation. This can be 1 to N number of ESXi hosts.

Just like dc, cluster, region and zone topology the openshift-region and openshift-zone based tags are required. I don't expect the CCM or CSI to know anything about a virtual machine or host type vm-host group - and based in testing it doesn't, the region and zone labels are still populated on a control plane or compute node.

the vm-host groups and rules are required to keep rhcos virtual machines on the correct ESXi hosts for the failure domain zone.

updated with 8c3f02c

openshift-ci · 2024-10-03T15:08:26Z

@jcpowermac: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

JoelSpeed · 2024-10-15T14:41:17Z

@jcpowermac At the moment, this PR updates the existing state of the EP to what was actually implemented, and then also updates to add an extension to the feature itself. Is there a way to separate the two changes? I feel like it would probably be beneficial to get the EP updated to the current state of the world first, and then look at the updates for the new features

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 5, 2024

jcpowermac changed the title ~~Update vSphere and include host/vm group~~ SPLAT-1733: Update vSphere and include host/vm group Sep 5, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 5, 2024

jcpowermac mentioned this pull request Sep 23, 2024

SPLAT-1743: vSphere - add host and vm based zonal openshift/api#1999

Open

jcpowermac marked this pull request as ready for review September 23, 2024 17:09

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2024

openshift-ci bot requested review from shawn-hurley and suleymanakbas91 September 23, 2024 17:09

Update vSphere and include host/vm group

89c3343

jcpowermac force-pushed the vsphere-vmhost-zonal branch from d61a30f to 89c3343 Compare September 23, 2024 19:49

openshift-ci bot assigned rvanderp3 and vr4manta Sep 23, 2024

openshift-ci bot requested review from gnufied and JoelSpeed September 23, 2024 19:50

JoelSpeed reviewed Sep 24, 2024

View reviewed changes

fix godoc

2ce86de

gnufied reviewed Sep 24, 2024

View reviewed changes

jcpowermac added 2 commits September 25, 2024 15:26

use the full infra and platform specs with current and future states

6487305

minor updates

5dfbf96

jcpowermac mentioned this pull request Sep 26, 2024

vSphere IPI Zonal - Streched Clusters #1570

Closed

gnufied reviewed Sep 26, 2024

View reviewed changes

jcpowermac added 4 commits September 26, 2024 14:30

specifics about vm-host group creation

8c3f02c

hostgroup zone union

677ce07

another option

fdb3dde

updates to api and cel

ea2162a

		// +kubebuilder:validation:Format=ipv4
		// +kubebuilder:validation:Format=ipv6

	// +kubebuilder:validation:Format=ipv4
	// +kubebuilder:validation:Format=ipv6
	// +kubebuilder:validation:XValidation:rule="isIP(self)",message="gateway must be a valid IPv4 or IPv6 address"

		// ipAddrs is a list of one or more IPv4 and/or IPv6 addresses and CIDR to assign to
		// this device, for example, 192.168.1.100/24. IP addresses provided via ipAddrs are

SPLAT-1733: Update vSphere and include host/vm group #1677

Are you sure you want to change the base?

SPLAT-1733: Update vSphere and include host/vm group #1677

Conversation

jcpowermac commented Sep 5, 2024 • edited Loading

Changes

Additional PRs

openshift-ci bot commented Sep 5, 2024

openshift-ci-robot commented Sep 5, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Sep 23, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Sep 23, 2024 • edited by openshift-ci bot Loading

Changes

Additional PRs

jcpowermac commented Sep 23, 2024

jcpowermac commented Sep 23, 2024

Choose a reason for hiding this comment

jcpowermac Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcpowermac Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2024

JoelSpeed commented Oct 15, 2024

jcpowermac commented Sep 5, 2024 •

edited

Loading

openshift-ci-robot commented Sep 5, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 23, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 23, 2024 •

edited by openshift-ci bot

Loading

jcpowermac Sep 24, 2024 •

edited

Loading

jcpowermac Sep 24, 2024 •

edited

Loading

jcpowermac Sep 24, 2024 •

edited

Loading

jcpowermac Sep 26, 2024 •

edited

Loading