[SPLAT-594] vSphere: add zonal spec to infrastructure object by jcpowermac · Pull Request #1278 · openshift/api

jcpowermac · 2022-08-31T19:31:14Z

This PR implements the required fields necessary to add zonal topology for vSphere-based OpenShift clusters.
It defines the vCenter based objects:

datacenter
cluster
network
datastore
that is required to provision and manage the arbitrary nature of vSphere and region and zone topology.

openshift-ci · 2022-08-31T19:31:22Z

Hello @jcpowermac! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

jcpowermac · 2022-08-31T19:38:17Z

https://gist.github.com/jcpowermac/49eaaadfacd2afcc4e93418a7ffc6fa4

jcpowermac · 2022-08-31T19:39:33Z

@JoelSpeed @gnufied @rvanderp3 @bostrt
Changes that were discussed earlier today

JoelSpeed · 2022-09-05T12:54:00Z

I think this is easier to comprehend that the previous iteration with the deployment zones, are you happier with this iteration?

jcpowermac · 2022-09-06T12:31:16Z

I think this is easier to comprehend that the previous iteration with the deployment zones, are you happier with this iteration?

It will certainly be easier for our users to interact with. Shouldn't be bad for us to work with either so I am good with it.

Should I go ahead and fix this PR up and close the other one? wdyt?

gnufied · 2022-09-06T19:35:05Z

mostly lgtm from storage POV.

gnufied · 2022-09-07T12:29:58Z

/lgtm

JoelSpeed

This is looking a lot better than before and I'm finding it much easier to follow! Mostly nits from this point on

JoelSpeed · 2022-09-07T12:47:44Z

+type VSpherePlatformFailureDomainSpec struct {
+	// name defines the name of the VSpherePlatformFailureDomainSpec
+	// This name is arbitrary but will be used
+	// in VSpherePlatformDeploymentZone for association.


Will it?

Probably worth noting that the names should be globally unique.

Do we want to limit this to a certain set of characters? Eg [a-z._-]*?

Nope I missed that with the changes will fix.

JoelSpeed · 2022-09-07T12:51:08Z

+	Topology VSpherePlatformTopology `json:"topology"`
+
+	// ControlPlane determines if this failure domain is suitable for use by control plane machines.
+	// There is three valid options: Allowed and Disallowed.


Suggested change

// There is three valid options: Allowed and Disallowed.

// Valid options are Allowed and Disallowed.

// When Allowed, control plane machines may be scheduled to this failure domain.

// When Disallowed, control plane machines may not be scheduled to this failure domain.

Why do we have this option? It seems odd not to have a partner here that determines if workers should be scheduled here

After chatting with @rvanderp3 we are going to remove it. In the case of the installer the user can define in the machinepool which zones are used for node type.

JoelSpeed

Structure looks good, some additional detail in the godoc would be helpful, added comments. Please think about what happens when optional fields aren't specified. And note that the validations are not generated into docs, so we should tell users the formats we expect in the godocs

JoelSpeed · 2022-09-12T11:10:23Z

+	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=80
+	// +kubebuilder:validation:Pattern=`^.*?`


What is this pattern doing?

Not sure what validation to put here. This can be any string up to 80 characters. vCenter encodes the name simlar to a URI path string. tbh I think the only validation we can do is string length.

Ok in that case lets drop this and make sure the comment on here explains the name is up to 80 chars in length

JoelSpeed · 2022-09-12T11:10:53Z

+	Datacenter string `json:"datacenter"`
+
+	// computeCluster is the vCenter cluster in which virtual machine will be located.
+	// This value is required to be a path.


Can you include an example of the format in the godoc please

JoelSpeed · 2022-09-12T11:11:32Z

+	// +kubebuilder:validation:MinItems=1
+	Networks []string `json:"networks"`
+
+	// datastore is the name or inventory path of the datastore in which the


Please include an example of the correct format in the godoc

JoelSpeed · 2022-09-12T11:12:20Z

+	// +optional
+	ResourcePool string `json:"resourcePool,omitempty"`
+
+	// folder is the name or inventory path of the folder in which the


Please include an example of the correct format in the godoc

JoelSpeed · 2022-09-12T11:12:36Z

+
+	// folder is the name or inventory path of the folder in which the
+	// virtual machine is created/located.
+	// +kubebuilder:validation:MinLength=1


Does minLength work with optional? Have you tested this?

Should we just remove MinLength since it doesn't make much sense in this context? I think we would be more concerned if the string was too long or didn't validate.

Yep happy to remove minlength

JoelSpeed · 2022-09-12T11:13:37Z

+// for the external and internal node networking.
+type VSpherePlatformNodeNetworkingSpec struct {
+	// networkSubnetCidr IP address on VirtualMachine's network interfaces included in the fields' CIDRs
+	// that will be used in respective status.addresses fields.


What happens when this field is omitted

I will add text regarding this.
These fields are for the CCM, if unused it reverts to previous method of setting the external and internal interfaces.

JoelSpeed · 2022-09-12T11:14:20Z

+	// external represents the network configuration of the node that is externally routable.
+	// +optional
+	External VSpherePlatformNodeNetworkingSpec `json:"external"`
+	// internal represents the network configuration of the node that is routable only within the cluster.
+	// +optional
+	Internal VSpherePlatformNodeNetworkingSpec `json:"internal"`


Please include in the godoc what happens when these aren't configured

JoelSpeed · 2022-09-12T11:14:41Z

+	// Currently, only a single vCenter is supported.
+	// ---
+	// + If VCenters is not defined use the existing cloud-config configmap defined
+	// in openshift-config.


This line needs a plus too

Suggested change

// in openshift-config.

// + in openshift-config.

deads2k · 2022-09-12T18:46:15Z

+- op: add
+  path: /spec/versions/name=v1/schema/openAPIV3Schema/properties/spec/properties/platformSpec/properties/vsphere/properties/vcenters/items/properties/server/anyOf
+  value:
+    - format: ipv4


@Miciah you've spent a lot of time in regexes and kubebuilder "validation". Do these work?

deads2k · 2022-09-12T18:49:36Z

+	Internal VSpherePlatformNodeNetworkingSpec `json:"internal"`
+}
+
 // VSpherePlatformSpec holds the desired state of the vSphere infrastructure provider.


Will any operator be making this state real or is this information used by another component to decide how to act, but we needed a way for the cluster-admin to evolve the config over time?

The installer will initially create it. The goal would be for the 3CMO and CSI operator to read and make changes to their configurations based on the cluster-admin changing this config.

deads2k · 2022-09-12T22:00:01Z

@JoelSpeed @jcpowermac Given this is techpreview in 4.12, it should include the techpreview tags from #1294 (the proof is running) and then we can merge, though you may need openshift/installer#6336 in order to make use of it.

jcpowermac · 2022-09-14T19:06:41Z

@deads2k @JoelSpeed is there anything I need to fixup with this pr?

JoelSpeed

Changes LGTM, thanks for working with me on this one
/lgtm

This PR implements the required fields necessary to add zonal topology for vSphere-based OpenShift clusters as a Tech Preview feature. It defines the vCenter based objects: datacenter, cluster, network, datastore, resource pool, folder and tag names that are required to provision and manage the arbitrary nature of vSphere and region and zone topology.

JoelSpeed · 2022-09-30T15:25:12Z

/lgtm

openshift-ci · 2022-09-30T15:29:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jcpowermac, JoelSpeed

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

jcpowermac · 2022-09-30T15:32:56Z

/label qe-approved
/label docs-approved
/label px-approved

SPLAT is following FF since the majority of the changes involved was within the installer repo.

openshift-ci · 2022-09-30T15:57:10Z

@jcpowermac: all tests passed!

Full PR test history. Your PR dashboard.

Details

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

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 Aug 31, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

openshift-ci Bot requested review from soltysh and sttts August 31, 2022 19:32

jcpowermac force-pushed the vsphere-zoning-alternative branch from 9dad12a to c991dea Compare August 31, 2022 19:37

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022

jcpowermac force-pushed the vsphere-zoning-alternative branch from c991dea to a1ec5ad Compare September 6, 2022 17:53

jcpowermac changed the title ~~[wip] Vsphere zoning alternative~~ [SPLAT-594] vSphere: add zonal spec to infrastructure object Sep 6, 2022

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 6, 2022

jcpowermac mentioned this pull request Sep 6, 2022

[SPLAT-594] vSphere: add zonal spec to infrastructure object #1199

Closed

jcpowermac force-pushed the vsphere-zoning-alternative branch from a1ec5ad to 650359d Compare September 6, 2022 18:20

gnufied reviewed Sep 6, 2022

View reviewed changes

Comment thread config/v1/types_infrastructure.go

jcpowermac force-pushed the vsphere-zoning-alternative branch from 650359d to 55d3460 Compare September 7, 2022 12:03

openshift-ci Bot assigned gnufied Sep 7, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022

JoelSpeed reviewed Sep 7, 2022

View reviewed changes

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022

jcpowermac force-pushed the vsphere-zoning-alternative branch from 0fbc9b8 to 711a040 Compare September 7, 2022 20:29

rvanderp3 mentioned this pull request Sep 8, 2022

[vsphere] refactor platform spec openshift/installer#6320

Closed

JoelSpeed reviewed Sep 12, 2022

View reviewed changes

jcpowermac mentioned this pull request Sep 12, 2022

vsphere zonal: platform spec and terraform updates openshift/installer#6329

Merged

deads2k reviewed Sep 12, 2022

View reviewed changes

deads2k mentioned this pull request Sep 12, 2022

Vpshere techpreview #1294

Closed

jcpowermac force-pushed the vsphere-zoning-alternative branch 2 times, most recently from abe6b2a to ef9a6f9 Compare September 13, 2022 19:52

JoelSpeed reviewed Sep 16, 2022

View reviewed changes

openshift-ci Bot assigned JoelSpeed Sep 16, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 16, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2022

jcpowermac force-pushed the vsphere-zoning-alternative branch from ef9a6f9 to 1c01e1e Compare September 27, 2022 18:08

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2022

openshift-ci Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 27, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2022

openshift-ci Bot added qe-approved Signifies that QE has signed off on this PR docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Sep 30, 2022

openshift-merge-robot merged commit 5648e61 into openshift:master Sep 30, 2022

rvanderp3 mentioned this pull request Oct 4, 2022

[vsphere] populate vSphere infrastructure spec openshift/installer#6457

Merged

-	// There is three valid options: Allowed and Disallowed.
+	// Valid options are Allowed and Disallowed.
+	// When Allowed, control plane machines may be scheduled to this failure domain.
+	// When Disallowed, control plane machines may not be scheduled to this failure domain.

Conversation

jcpowermac commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Aug 31, 2022

Uh oh!

jcpowermac commented Aug 31, 2022

Uh oh!

jcpowermac commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Sep 5, 2022

Uh oh!

jcpowermac commented Sep 6, 2022

Uh oh!

Uh oh!

gnufied commented Sep 6, 2022

Uh oh!

gnufied commented Sep 7, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Sep 12, 2022

Uh oh!

jcpowermac commented Sep 14, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

jcpowermac commented Aug 31, 2022 •

edited

Loading

jcpowermac commented Aug 31, 2022 •

edited

Loading