SPLAT-1127: extend API to enable vSphere control plane machineset #1539

rvanderp3 · 2023-07-26T21:52:20Z

To enable control plane machineset support, the followings changes are required:

add VSphereFailureDomain
add template to infrastructure spec. a unique template is created for each failure domain. CPMS has no way of knowing what the intended template without it being defined in the failure domain topology.

These API extensions are gated by the VSphereControlPlaneMachineSet feature gate.

openshift-ci · 2023-07-26T21:52:29Z

Hello @rvanderp3! 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.

rvanderp3 · 2023-07-26T22:03:12Z

/uncc @eparis @mandre

openshift-ci-robot · 2023-08-10T17:46:48Z

@rvanderp3: This pull request references SPLAT-1127 which is a valid jira issue.

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 kubernetes/test-infra repository.

openshift-ci-robot · 2023-08-14T15:53:17Z

@rvanderp3: This pull request references SPLAT-1127 which is a valid jira issue.

In response to this:

To enable control plane machineset support, the followings changes are required:

add VSphereFailureDomain

add template to infrastructure spec. a unique template is created for each failure domain. CPMS has no way of knowing what the intended template without it being defined in the failure domain topology.

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-robot · 2023-08-23T20:06:10Z

@rvanderp3: This pull request references SPLAT-1127 which is a valid jira issue.

In response to this:

To enable control plane machineset support, the followings changes are required:

add VSphereFailureDomain

add template to infrastructure spec. a unique template is created for each failure domain. CPMS has no way of knowing what the intended template without it being defined in the failure domain topology.

These API extensions are gated by the VSphereControPlaneMachineSet feature gate.

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.

rvanderp3 · 2023-09-20T12:29:11Z

@JoelSpeed this is ready for another look when you get a chance.

rvanderp3 · 2023-09-20T19:41:57Z

test results from today:

e2e presubmit tests[passed]: https://gist.github.com/rvanderp3/fb1cf8a69bb6f5300cf9d36a7476189d
multi-zone, tech preview[cpmso reconciled]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-vsphere-modern/1704494131960614912
multi-zone, non tech-preview[cpmso shows platform as unsupported]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-vsphere-modern/1704529844882116608
AWS, non tech-preview: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/release-openshift-origin-installer-launch-aws-modern/1706288409699422208

JoelSpeed · 2023-09-25T13:00:34Z

machine/v1/types_controlplanemachineset.go

+	// Name is the name of the failure domain in which the vSphere machine provider will create the VM.
+	// Failure domains are defined in a cluster's config.openshift.io/Infrastructure resource.
+	// +kubebuilder:validation:Required
+	Name string `json:"name"`


Ok, I think we should expand the comment here then to explain what the CPMS will do with the information from the failure domains.

// When balancing machines across failure domains, the control plane machine set will inject configuration from the // Infrastructure resource into the machine providerSpec to allocate the machine to a failure domain.

JoelSpeed · 2023-09-25T13:00:56Z

machine/v1/types_controlplanemachineset.go

 	// +optional
 	GCP *[]GCPFailureDomain `json:"gcp,omitempty"`

+	// VSphere configures failure domain information for the VSphere platform.


Nit, should be a lower case vsphere to match json casing, other fields here aren't correct

Suggested change

// VSphere configures failure domain information for the VSphere platform.

// vsphere configures failure domain information for the VSphere platform.

JoelSpeed · 2023-09-25T13:02:35Z

config/v1/types_infrastructure.go

+	// template is the inventory path of the virtual machine or template
+	// that will be used for cloning.


Suggested change

// template is the inventory path of the virtual machine or template

// that will be used for cloning.

// template is the inventory path of the virtual machine or template

// that will be cloned when creating new machines in this failure domain.

JoelSpeed · 2023-09-25T13:03:41Z

config/v1/types_infrastructure.go

+	// +kubebuilder:validation:MinLength=1
+	// +kubebuilder:validation:MaxLength=2048
+	// +kubebuilder:validation:Pattern=`^/.*?/vm/.*?`


These validations should be explained in prose within the godoc.

Do we run the same regex validation against the template field on the providerSpec?

yes, we will perform that validation in the cpmso

validation for this has been added to the cpmso

rvanderp3 · 2023-09-29T20:35:46Z

hi @JoelSpeed when you get a chance can you give this another look? Thanks!

JoelSpeed

One small nit but otherwise LGTM, how are we getting on with the implementation? I'm guessing I need to review that one next?

JoelSpeed · 2023-10-10T09:05:21Z

config/v1/types_infrastructure.go

+	// that will be cloned when creating new machines in this failure domain.
+	// The maximum length of the path is 2048 characters.
+	//
+	// if not defined, the template will be calculated by the control plane


Nit for idiomatic language

Suggested change

// if not defined, the template will be calculated by the control plane

// When omitted, the template will be calculated by the control plane

The implementation is going well. We still have some issues we are working through as we continue to test. Certainly, if you have time to review the implementation PR that would be great. We are working through some remaining issues but the overall function is in the PR.

JoelSpeed · 2023-10-10T13:12:07Z

config/v1/types_infrastructure.go

+	// that will be cloned when creating new machines in this failure domain.
+	// The maximum length of the path is 2048 characters.
+	//
+	// when omitted, the template will be calculated by the control plane


Should be a capital W on when since there's a full stop on the previous para

jcpowermac · 2023-10-10T19:41:57Z

/lgtm

rvanderp3 · 2023-10-18T13:36:54Z

@JoelSpeed this is ready for another look when you get a chance.

JoelSpeed · 2023-10-24T11:17:20Z

/lgtm
/override ci/prow/verify-crd-schema

Failures are pre-existing, the schema check doesn't account for renames

openshift-ci · 2023-10-24T11:18:10Z

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to this:

/lgtm
/override ci/prow/verify-crd-schema

Failures are pre-existing, the schema check doesn't account for renames

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 · 2023-10-24T11:18:13Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

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

openshift-ci · 2023-10-24T11:18:14Z

@rvanderp3: 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/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 Jul 26, 2023

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 26, 2023

openshift-ci bot requested review from eparis and mandre July 26, 2023 21:52

openshift-ci bot removed request for eparis and mandre July 26, 2023 22:03

rvanderp3 mentioned this pull request Jul 26, 2023

SPLAT-1129: adding failure domain awareness for vSphere openshift/cluster-control-plane-machine-set-operator#228

Merged

6 tasks

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 7be297a to b91b14a Compare July 27, 2023 14:48

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 2 times, most recently from 6f9f13f to 299b3fe Compare August 9, 2023 14:13

rvanderp3 changed the title ~~WIP: define VSphereFailureDomain for use in control plane machinesets~~ SPLAT-1127: define VSphereFailureDomain for use in control plane machinesets Aug 10, 2023

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

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 10, 2023

rvanderp3 changed the title ~~SPLAT-1127: define VSphereFailureDomain for use in control plane machinesets~~ SPLAT-1127: extend API to enable vSphere control plane machineset Aug 14, 2023

rvanderp3 mentioned this pull request Aug 14, 2023

SPLAT-1141: implement vSphere control plane machinesets openshift/installer#7418

Merged

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 2 times, most recently from 8337577 to 37d9131 Compare August 15, 2023 17:42

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 37d9131 to 9dae305 Compare August 23, 2023 19:48

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 23, 2023

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 9dae305 to 728d311 Compare August 23, 2023 19:58

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 3 times, most recently from 44da11b to 37d9131 Compare August 25, 2023 16:29

openshift-ci bot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 25, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2023

JoelSpeed reviewed Sep 25, 2023

View reviewed changes

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from fabb5c2 to af9ae35 Compare September 26, 2023 14:40

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from af9ae35 to 5b09f16 Compare October 3, 2023 17:22

JoelSpeed reviewed Oct 10, 2023

View reviewed changes

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 5b09f16 to 3bda49d Compare October 10, 2023 12:53

JoelSpeed reviewed Oct 10, 2023

View reviewed changes

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 3bda49d to 58ee449 Compare October 10, 2023 13:30

openshift-ci bot assigned jcpowermac Oct 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 58ee449 to 2f7bbc5 Compare October 18, 2023 13:39

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023

rvanderp3 added 3 commits October 23, 2023 15:22

define VSphereFailureDomain for use by control plane machinesets

9e435a7

add template to infrastructure vSphere platform topology

c7b7f25

create vSphere control plane machienset feature gate

c2f724a

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 2f7bbc5 to c2f724a Compare October 23, 2023 19:22

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023

openshift-ci bot merged commit 79b9cd5 into openshift:master Oct 24, 2023

	// VSphere configures failure domain information for the VSphere platform.
	// vsphere configures failure domain information for the VSphere platform.

		// template is the inventory path of the virtual machine or template
		// that will be used for cloning.

	// if not defined, the template will be calculated by the control plane
	// When omitted, the template will be calculated by the control plane

SPLAT-1127: extend API to enable vSphere control plane machineset #1539

SPLAT-1127: extend API to enable vSphere control plane machineset #1539

Uh oh!

Conversation

rvanderp3 commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 26, 2023

Uh oh!

rvanderp3 commented Jul 26, 2023

Uh oh!

openshift-ci-robot commented Aug 10, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 14, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Aug 23, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvanderp3 commented Sep 20, 2023

Uh oh!

rvanderp3 commented Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvanderp3 commented Sep 29, 2023

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!

jcpowermac commented Oct 10, 2023

Uh oh!

rvanderp3 commented Oct 18, 2023

Uh oh!

JoelSpeed commented Oct 24, 2023

Uh oh!

openshift-ci bot commented Oct 24, 2023

Uh oh!

openshift-ci bot commented Oct 24, 2023

Uh oh!

openshift-ci bot commented Oct 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rvanderp3 commented Jul 26, 2023 •

edited

Loading

openshift-ci-robot commented Aug 10, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 14, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 23, 2023 •

edited by openshift-ci bot

Loading

rvanderp3 commented Sep 20, 2023 •

edited

Loading