Skip to content

Comments

[vsphere] support for multi-zone/region installation#5911

Merged
openshift-merge-robot merged 9 commits intoopenshift:masterfrom
rvanderp3:mcmd-platform
Aug 26, 2022
Merged

[vsphere] support for multi-zone/region installation#5911
openshift-merge-robot merged 9 commits intoopenshift:masterfrom
rvanderp3:mcmd-platform

Conversation

@rvanderp3
Copy link
Contributor

The intention of this PR is to implement the changes to the platform specification for vsphere as defined in openshift/enhancements#918

@openshift-ci 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 13, 2022
@openshift-ci openshift-ci bot requested review from jhixson74 and patrickdillon May 13, 2022 16:03
@rvanderp3
Copy link
Contributor Author

cc @jcpowermac

@rvanderp3 rvanderp3 force-pushed the mcmd-platform branch 8 times, most recently from 07c2ef2 to de29a70 Compare May 23, 2022 17:29
@rvanderp3 rvanderp3 force-pushed the mcmd-platform branch 17 times, most recently from ca39a4f to 3a1dbd0 Compare May 26, 2022 20:11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I'm not sure about this. Two things:

  1. Don't both roles (compute & master) go through this code path? So I suspect this would incorrectly fail if a zone with ControlPlane == NotAllowed were used for a compute machine pool.
  2. I suspect this code should live in the machinepool validation code--not here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Machines is only used for rendering control plane nodes. Compute nodes are rendered from machinesets. I can look at pulling this in to the machinepool validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its coming back to me now. at the point of machinepool validation, there isn't a readily available context indicating that a given pool is controlPlane or compute.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ... wait, we know the pool name, and that is validated to ensure it matches controlPlane and compute pools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patrickdillon changes have been committed which apply default zones based upon the machine pool role and what deploymentZones are defined.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2022
@rvanderp3
Copy link
Contributor Author

/retest
/test e2e-vsphere-zones

@rvanderp3
Copy link
Contributor Author

/retest

implements the platform specification and associated validation to
support vSphere multi-zone/region

Implements openshift/enhancements#918
implements zone awareness and validation for associating machinepools
with vSphere zone/region failure domains

implements openshift/enhancements#918
assigns machines and machinesets to zones based upon machinepool
definition

implements openshift/enhancements#918
includes cloud-provider-vsphere to aid with (de)serialization
of vSphere cloud-provider config resources

implements openshift/enhancements#918
required to (un)marshall vSphere cloud provider structs properly

implements openshift/enhancements#918
generates the out of tree cloud provider configuration
when regions/zones are defined

implements openshift/enhancements#918
enables the generation of vsphere-creds credential entries for
multiple vCenters

implements openshift/enhancements#918
verifies that the vCenter resources defined for multi-zone/region
failure domains exist

implements openshift/enhancements#918
labels generated machine specs to enable terraform to properly
target a machine to the desired zone/region

implements openshift/enhancements#918
@rvanderp3
Copy link
Contributor Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

@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-upi 0bf5a1569ffb4a0dc7792ed1cfae761a2e5f5aea link false /test e2e-vsphere-upi
ci/prow/e2e-crc 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link false /test e2e-crc
ci/prow/e2e-azure-upi 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link true /test e2e-azure-upi
ci/prow/e2e-gcp-upi 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link true /test e2e-gcp-upi
ci/prow/e2e-aws-upi 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link true /test e2e-aws-upi
ci/prow/e2e-alibaba 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link false /test e2e-alibaba
ci/prow/e2e-nutanix 66dd5b3f60c8830a66a4faa2828e6cd62da1e83b link false /test e2e-nutanix
ci/prow/e2e-vsphere-zones 1ed9b260823d9933ae91af6475e599b320387e9d link false /test e2e-vsphere-zones
ci/prow/e2e-libvirt e94d5aa link false /test e2e-libvirt
ci/prow/e2e-openstack e94d5aa link false /test e2e-openstack
ci/prow/e2e-ibmcloud e94d5aa link false /test e2e-ibmcloud

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.

@patrickdillon
Copy link
Contributor

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2022
@patrickdillon
Copy link
Contributor

/skip

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2022
@rvanderp3
Copy link
Contributor Author

/retest

1 similar comment
@rvanderp3
Copy link
Contributor Author

/retest

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 2 against base HEAD 1c8e79b and 8 for PR HEAD e94d5aa in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 1 against base HEAD 1c8e79b and 7 for PR HEAD e94d5aa in total

@openshift-merge-robot openshift-merge-robot merged commit 070b006 into openshift:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants