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

rvanderp3 · 2023-07-26T22:38:22Z

Associated PRs:

Depends on PRs:

SPLAT-1211: recognize topology and failure domains represented by the infrastructure resource #250

rvanderp3 · 2023-08-10T13:42:15Z

converting to draft to prevent unnecessary CI runs.

rvanderp3 · 2023-08-10T14:32:27Z

/test lint

rvanderp3 · 2023-08-10T18:00:37Z

/test lint
/test unit

rvanderp3 · 2023-08-11T17:22:35Z

/test fmt
/test generate
/test lint
/test unit
/test vendor
/test vet

rvanderp3 · 2023-08-11T17:31:24Z

/test images

openshift-ci-robot · 2023-08-14T17:09:14Z

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

Details

In response to this:

Associated PRs:

SPLAT-1129: create fixtures for VSphereProviderSpec zones cluster-api-actuator-pkg#288

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

SPLAT-1127: bring in vSphere CPMSO API updates client-go#251

Pre-merge testing:

Build this branch and push to a repository that the eventual cluster can access

Create a new release image for the cluster which configures the cluster-control-plane-machine-set-operator image to point to the built image
oc adm release new  --from-release "<source release image>" --name "<new version>" --to-image "<new release image>" cluster-control-plane-machine-set-operator="<built image>"
Perform an installation of that release image

Change the generated control plane machineset active from inactive

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.

jcpowermac · 2023-11-20T14:28:15Z

/payload-job periodic-ci-openshift-release-master-nightly-4.15-e2e-vsphere-ovn-serial

openshift-ci · 2023-11-20T14:28:54Z

@jcpowermac: trigger 1 job(s) for the /payload-(job|aggregate) command

periodic-ci-openshift-release-master-nightly-4.15-e2e-vsphere-ovn-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0b745ac0-87b1-11ee-934c-9f87e50880b6-0

jcpowermac · 2023-11-20T14:29:05Z

/test e2e-vsphere-multi-zone-operator e2e-vsphere-operator

JoelSpeed · 2023-11-20T14:12:22Z

Makefile

 .PHONY: lint
 lint: ## Run golangci-lint over the codebase.
-	$(call ensure-home, ${GOLANGCI_LINT} run ./... --timeout 5m)
+	$(call ensure-home, GOGC=50 ${GOLANGCI_LINT} run ./... --timeout 5m --new-from-rev=${PULL_BASE_SHA} -v)


Does the linter still work locally with these changes?

yes, it does.

JoelSpeed · 2023-11-20T14:14:08Z

cmd/control-plane-machine-set-operator/main.go

+		Client:                        mgr.GetClient(),
+		Scheme:                        mgr.GetScheme(),
+		Namespace:                     managedNamespace,
+		VSphereCPMSFeatureGateEnabled: vSphereCPMSFeatureGateEnabled,


I would probably put the feature gate accessor as a member of the reconciler struct so that we can keep this around once this feature gate is gone/so it can be re-used by other features

JoelSpeed · 2023-11-20T14:15:13Z

manifests/0000_30_control-plane-machine-set-operator_00_controlplanemachineset-default.crd.yaml

    api-approved.openshift.io: https://github.com/openshift/api/pull/1112
    exclude.release.openshift.io/internal-openshift-hosted: "true"
    include.release.openshift.io/self-managed-high-availability: "true"
+    release.openshift.io/feature-set: Default


We need to make sure the TechPreview and Custom versions of this are included as well

JoelSpeed · 2023-11-20T14:15:56Z

pkg/controllers/controlplanemachinesetgenerator/controller.go

 			return nil, fmt.Errorf("unable to generate control plane machine set spec: %w", err)
 		}
+	case configv1.VSpherePlatformType:
+		if !r.VSphereCPMSFeatureGateEnabled {


Should move the feature gate accessor Enabled call to here IMO

JoelSpeed · 2023-11-20T14:18:13Z

pkg/controllers/controlplanemachinesetgenerator/utils.go

+	if cpmsFailureDomain.Platform == "" {
+		return nil, errNoFailureDomains
+	}


Can we actually get here? Is there ever a generation on AWS/Azure/GCP without failure domains?

it doesn't appear we can, just removed this.

JoelSpeed · 2023-11-20T14:18:27Z

pkg/controllers/controlplanemachinesetgenerator/utils.go

+	var cpmsFailureDomain machinev1.FailureDomains
+
+	var err error


Does the linter complain if you remove this empty line?

yes, it does with 'declarations should never be cuddled'

JoelSpeed · 2023-11-20T14:18:52Z

pkg/controllers/controlplanemachinesetgenerator/utils.go

-		return nil, fmt.Errorf("%w: %sFailureDomain{}", errUnsupportedPlatform, machineFailureDomains[0].Type())
+	cpmsFailureDomain, err := buildPlatformFailureDomains(platformType, failureDomains)
+	if err != nil {
+		return nil, err


Error should be wrapped

JoelSpeed · 2023-11-20T14:19:21Z

pkg/controllers/controlplanemachinesetgenerator/utils.go

-	if cpmsFailureDomain.Platform == "" {
-		return nil, errNoFailureDomains
+	if cpmsFailureDomain == nil && err == nil {
+		return nil, nil //nolint:nilnil


Why is this ok to be nil nil? Can you add a quick comment to explain it? Can we even get a nil nil? Looking at buildPlatformFailureDomains I don't see it as possible to return nil nil?

you are correct, removing that statement

JoelSpeed · 2023-11-20T14:53:47Z

pkg/machineproviders/providers/openshift/machine/v1beta1/providerconfig/vsphere.go

+		}
+	}
+
+	return machinev1.VSphereFailureDomain{}


Unresolved conversation thread

rvanderp3 · 2023-11-20T21:47:27Z

/hold

placing hold for additional work on comments

rvanderp3 · 2023-11-27T13:21:50Z

/hold cancel

rvanderp3 · 2023-11-27T13:50:51Z

/test unit

Co-authored-by: vr4manta <vr4manta@gmail.com>

https://golangci-lint.run/usage/performance/

JoelSpeed · 2023-11-27T17:47:31Z

/approve
/lgtm

openshift-ci · 2023-11-27T17:53:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

openshift-ci · 2023-11-27T19:54:32Z

@rvanderp3: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-vsphere-operator	`3f8ed21`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-vsphere-multi-zone-operator	`3f8ed21`	link	false	`/test e2e-vsphere-multi-zone-operator`
ci/prow/e2e-vsphere-ovn-etcd-scaling	`3f8ed21`	link	false	`/test e2e-vsphere-ovn-etcd-scaling`
ci/prow/e2e-azure-ovn-etcd-scaling	`3f8ed21`	link	false	`/test e2e-azure-ovn-etcd-scaling`

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-bot · 2023-11-27T22:41:27Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-control-plane-machine-set-operator-container-v4.15.0-202311272131.p0.g800e341.assembly.stream for distgit ose-cluster-control-plane-machine-set-operator.
All builds following this will include this PR.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 26, 2023

openshift-ci bot requested review from damdo and elmiko July 26, 2023 22:39

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

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 8, 2023

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 2 times, most recently from 21a03f8 to 9831732 Compare August 9, 2023 14:35

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

rvanderp3 marked this pull request as draft August 10, 2023 13:41

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 9831732 to ae64fda Compare August 10, 2023 14:32

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 042f8fd to b7e9d28 Compare August 10, 2023 18:00

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 2 times, most recently from 728f955 to 1777d77 Compare August 11, 2023 14:19

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 719486c to a6d2e97 Compare August 14, 2023 16:04

rvanderp3 changed the title ~~WIP: adding support for vSphere~~ SPLAT-1129: WIP: adding support for vSphere Aug 14, 2023

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

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 6 times, most recently from 6aedea0 to 2a6bacf Compare August 15, 2023 18:24

JoelSpeed reviewed Nov 20, 2023

View reviewed changes

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from e08d75f to 1add572 Compare November 20, 2023 21:46

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 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 Nov 27, 2023

rvanderp3 and others added 8 commits November 27, 2023 09:04

vendor

1c671f2

Co-authored-by: vr4manta <vr4manta@gmail.com>

introduce feature gate

0cfe722

Co-authored-by: vr4manta <vr4manta@gmail.com>

add vSphere platform to cpms reconciliation

44905d3

e2e tests

fcb2245

update crd sync to sync techpreview/customnoupgrade CRDs

ef9646e

providerSpec validation

c821fce

preserve ippool when failure domains are specified

9c41715

lint changes from main and reduce memory usage

0019b26

https://golangci-lint.run/usage/performance/

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch 2 times, most recently from 663d8b4 to 805d05e Compare November 27, 2023 17:16

extend presubmit timeout for vSphere

3f8ed21

rvanderp3 force-pushed the cpms-vsphere-failure-domain branch from 805d05e to 3f8ed21 Compare November 27, 2023 17:22

openshift-ci bot assigned JoelSpeed Nov 27, 2023

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

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

openshift-merge-bot bot merged commit 800e341 into openshift:main Nov 27, 2023

Conversation

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

Uh oh!

Uh oh!

rvanderp3 commented Aug 10, 2023

Uh oh!

rvanderp3 commented Aug 10, 2023

Uh oh!

rvanderp3 commented Aug 10, 2023

Uh oh!

rvanderp3 commented Aug 11, 2023

Uh oh!

rvanderp3 commented Aug 11, 2023

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!

jcpowermac commented Nov 20, 2023

Uh oh!

openshift-ci bot commented Nov 20, 2023

Uh oh!

jcpowermac commented Nov 20, 2023

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!

rvanderp3 commented Nov 20, 2023

Uh oh!

rvanderp3 commented Nov 27, 2023

Uh oh!

rvanderp3 commented Nov 27, 2023

Uh oh!

JoelSpeed commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-bot commented Nov 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Comments

rvanderp3 commented Jul 26, 2023 •

edited

Loading

openshift-ci-robot commented Aug 14, 2023 •

edited by openshift-ci bot

Loading