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

jcpowermac · 2022-05-19T18:21:40Z

No description provided.

jcpowermac · 2022-05-19T18:22:12Z

See enhancement: openshift/enhancements#918

openshift-ci · 2022-08-02T18:25:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jcpowermac
Once this PR has been reviewed and has the lgtm label, please assign sjenning for approval by writing /assign @sjenning in a comment. For more information see:The Kubernetes Code Review Process.

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

Details

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

jcpowermac · 2022-08-05T15:40:10Z

/test verify

JoelSpeed · 2022-08-09T12:25:24Z

+	// networks is the list of port group network names within this failure domain.
+	// Currently, we only support a single interface per RHCOS virtual machine.


What is an expected value here?

port group name

@rvanderp3 @bostrt do you think this should be a path?

i don't think a path is required here. the network is looked up relative to the associated datacenter and cluster.

An example of an expected value is useful for godocs

JoelSpeed · 2022-08-09T12:28:04Z

+- op: add
+  path: /spec/versions/name=v1/schema/openAPIV3Schema/properties/spec/properties/platformSpec/properties/vsphere/properties/nodeNetworking/properties/external/properties/excludeNetworkSubnetCidr/items/format
+  value: cidr
+- op: add
+  path: /spec/versions/name=v1/schema/openAPIV3Schema/properties/spec/properties/platformSpec/properties/vsphere/properties/nodeNetworking/properties/external/properties/networkSubnetCidr/items/format
+  value: cidr
+- op: add
+  path: /spec/versions/name=v1/schema/openAPIV3Schema/properties/spec/properties/platformSpec/properties/vsphere/properties/nodeNetworking/properties/internal/properties/excludeNetworkSubnetCidr/items/format
+  value: cidr
+- op: add
+  path: /spec/versions/name=v1/schema/openAPIV3Schema/properties/spec/properties/platformSpec/properties/vsphere/properties/nodeNetworking/properties/internal/properties/networkSubnetCidr/items/format
+  value: cidr


If you only have one format, can this not be kubebuilder:validation:Format:=cidr?

For some reason kubebuilder adds the format not under items.
Unsure its a bug or a me problem.

jcpowermac · 2022-08-25T18:54:50Z

/test verify

This PR adds additional fields for the vSphere spec to provide zonal information for operators such as storage, machine and cloud controller.

JoelSpeed · 2022-08-26T10:11:10Z

+	// +kubebuilder:validation:MaxLength=80
+	Datacenter string `json:"datacenter"`
+
+	// computeCluster as the failure domain


What does this mean?

Is this option only valid when type is ComputeCluster?

Does this need to be a discrminated union https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#discriminated-unions?

Changed text since we removed HostGroup as a topology option.

JoelSpeed · 2022-08-26T10:12:43Z

+
+	// Topology describes a given failure domain using vSphere constructs
+	// +kubebuilder:validation:Required
+	Topology VSpherePlatformTopology `json:"topology"`


I'm getting the impression that the topology should be a discrminated union

topology: type: Datacenter datacenter: name: ...

Why is the type in the region/zone failure domain rather than the topology?

With the removal of HostGroup as a zone not sure this still applies.

Topology defines the vCenter-based objects that a region-zone pair includes.

gnufied · 2022-08-26T15:10:39Z

+	// +kubebuilder:validation:MaxLength=2048
+	// +kubebuilder:validation:Pattern=`^/.*?/host/.*?/Resources.*`
+	// +optional
+	ResourcePool string `json:"resourcePool,omitempty"`


So this was something that is not configurable before and now it is. Can the values here conflict with folder optional below? If unspecified - the system picks a default ?

ResourcePool is configurable
https://github.com/openshift/installer/blob/release-4.11/pkg/types/vsphere/platform.go#L42-L44

gnufied · 2022-08-26T16:09:21Z

+	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=80
+	TagCategory string `json:"tagCategory"`


One of the concerns @jsafrane raised was - since we are not enforcing anything on these tag category names, what happens if customers names them inconsistently? Such as say:

FailureDomain A - tag is "regionA", category is" k8s-region", for zone - tag is "zoneA", category is "k8s-zone"

FailureDomain B - for region (tag is regionB and category is "k8s-region"), for zone (tag is "zoneB" and category is "locationZone")

In this case, categories of zones isn't matching and could create problems in determining topology of storage/nodes. I have not tested the behavior of CSI driver if categories are named inconsistently. In a ideal case, for failureDomain B - a reasonable person would name category of the zone to be k8s-zone (basically same as category name in failureDomainA).

So question is - should we enforce some kind of restriction on these names? Is allowing any random string for a domain is too broad and will cause problems?

Basically I think that - if user is using tag category - "k8s-region" for region, then they should use same tag category for all the regions in the cluster. Values can be different but category names should be the same. Allowing different "ways" of naming tag categories opens us up for various kind of corner cases.

I was also thinking of picking a convention for these categories and sticking with it. So basically, you can't call your region category anything other than k8s-region, so on and so forth. What do you think?

Updated removing the ability to set the tag category.
The Region and Zone are now just strings for the tag name.

Is the category name now openshift-zone and openshift-region hardcoded values? Any other values are not accepted?

Going to create a PR for the installer now to hardset the cloud-config labels to openshift-region, openshift-zone.
Will also update the enhancement

It won't be definable so not sure how other values would be accepted.

openshift/installer#6273

Thank you. much neater. If need be we can relax this later.

jcpowermac · 2022-08-26T17:53:05Z

Here is an example of the yaml:
https://gist.github.com/jcpowermac/49eaaadfacd2afcc4e93418a7ffc6fa4

openshift-ci · 2022-08-29T17:30:55Z

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

jcpowermac · 2022-09-06T18:12:55Z

/close
in favor of #1278

openshift-merge-robot · 2022-09-06T18:13:05Z

@jcpowermac: PR needs rebase.

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.

openshift-ci · 2022-09-06T18:13:07Z

@jcpowermac: Closed this PR.

Details

In response to this:

/close
in favor of #1278

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.

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 May 19, 2022

openshift-ci Bot requested review from derekwaynecarr and sjenning May 19, 2022 18:22

JoelSpeed reviewed Jun 22, 2022

View reviewed changes

rvanderp3 reviewed Jul 28, 2022

View reviewed changes

Comment thread config/v1/types_infrastructure.go

Comment thread config/v1/types_infrastructure.go

jcpowermac force-pushed the vsphere-zoning branch from 8cadeb1 to 48f263b Compare August 2, 2022 18:25

jcpowermac force-pushed the vsphere-zoning branch 2 times, most recently from b1d51ae to cfbf5bd Compare August 5, 2022 13:32

jcpowermac changed the title ~~[WIP] Add additional properties required for vSphere zoning~~ Add additional properties required for vSphere zoning Aug 5, 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 Aug 5, 2022

JoelSpeed reviewed Aug 9, 2022

View reviewed changes

jcpowermac force-pushed the vsphere-zoning branch from cfbf5bd to 4544e36 Compare August 9, 2022 23:02

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

jcpowermac force-pushed the vsphere-zoning branch from 4544e36 to 9113afe Compare August 9, 2022 23:10

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

jcpowermac force-pushed the vsphere-zoning branch from 9113afe to e5ee9d5 Compare August 9, 2022 23:27

patrickdillon mentioned this pull request Aug 11, 2022

vsphere: add multiple datacenter and clusters openshift/enhancements#918

Merged

jcpowermac changed the title ~~Add additional properties required for vSphere zoning~~ vSphere: add zonal spec to infrastructure object Aug 25, 2022

jcpowermac changed the title ~~vSphere: add zonal spec to infrastructure object~~ [SPLAT-594] vSphere: add zonal spec to infrastructure object Aug 25, 2022

jcpowermac force-pushed the vsphere-zoning branch from e5ee9d5 to a9c50d0 Compare August 25, 2022 18:26

jcpowermac force-pushed the vsphere-zoning branch from a9c50d0 to 514d253 Compare August 25, 2022 19:12

vSphere: add zonal spec to infrastructure

2285e5f

This PR adds additional fields for the vSphere spec to provide zonal information for operators such as storage, machine and cloud controller.

jcpowermac force-pushed the vsphere-zoning branch from 514d253 to 2285e5f Compare August 25, 2022 20:11

JoelSpeed reviewed Aug 26, 2022

View reviewed changes

gnufied reviewed Aug 26, 2022

View reviewed changes

Comment thread config/v1/types_infrastructure.go Outdated

gnufied reviewed Aug 26, 2022

View reviewed changes

jcpowermac added 4 commits August 26, 2022 14:59

removal of hostgroup topology option

e033120

complete removal of hostgroup option; removed tag category option

588bfcf

make controlplane required

353667b

comment changes

9ef3ea9

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

openshift-ci Bot closed this Sep 6, 2022

		// networks is the list of port group network names within this failure domain.
		// Currently, we only support a single interface per RHCOS virtual machine.

Conversation

jcpowermac commented May 19, 2022

Uh oh!

jcpowermac commented May 19, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci Bot commented Aug 2, 2022

Uh oh!

jcpowermac commented Aug 5, 2022

Uh oh!

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!

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!

jcpowermac commented Aug 25, 2022

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

jcpowermac commented Aug 26, 2022

Uh oh!

openshift-ci Bot commented Aug 29, 2022

Uh oh!

jcpowermac commented Sep 6, 2022

gnufied Aug 26, 2022 •

edited

Loading