Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPI for vSphere w/ existing resource pool #5136

Merged
merged 7 commits into from
Nov 17, 2021

Conversation

willhaines
Copy link
Contributor

@willhaines willhaines commented Aug 5, 2021

Adds support for installing a cluster into an existing resource pool when using IPI on VMware. This allows users to take advantage of IPI on VMware with fewer cluster-wide permissions. Resolves #3921.

Heavily borrows from PR #3498. Tested manually against 4.7.0-0.okd-2021-07-03-190901 on vSphere 6.7 with (new functionality) and without (existing functionality) a resource pool. This was done by cherry-picking these commits on top of vrutkovs/installer:release-4.7-okd.

TODO:

  • Restructure doc/user/vsphere/privileges.md a bit to reflect permissions required when specifying an existing folder and/or resource pool, possibly without enumerating every possible combination of existing or non-existing folder and resource pool.

@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 Aug 5, 2021
@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 5, 2021

Hi @willhaines. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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
Copy link
Contributor

Thanks @willhaines for the PR!
/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2021
@jcpowermac
Copy link
Contributor

/test e2e-vsphere
/test e2e-vsphere-upi

@jcpowermac
Copy link
Contributor

vsphere e2e failing for tests not installation related - this should be ok.

@willhaines willhaines changed the title WIP: IPI for vSphere w/ existing resource pool IPI for vSphere w/ existing resource pool Aug 10, 2021
@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 10, 2021
@willhaines
Copy link
Contributor Author

@jcpowermac @jstuever I've now finished the doc updates that were in progress. Can either of you shine some light on which tests need to pass, and which ones are okay to fail?

@jcpowermac
Copy link
Contributor

@willhaines need to update:
https://github.com/openshift/installer/blob/master/data/data/install.openshift.io_installconfigs.yaml#L2057-L2138
for the additional resourcepool field in the platform struct

We are also in code-freeze so this won't be merged until master is back open.

@willhaines
Copy link
Contributor Author

/remove-work-in-progress

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2021
@jcpowermac
Copy link
Contributor

@ayesha54 if you have time can you please review this PR

@@ -255,12 +270,16 @@ func findImportOvaParams(client *vim25.Client, datacenter, cluster, datastore, n
}

if foundDatastore && foundNetwork {
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

oops ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops ^

Sorry for all the noise. That's what I get for trying to use fancy merge tools in vscode. Should be good now.

Copy link
Contributor

Choose a reason for hiding this comment

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

no worries @willhaines, it happens to all of us ;-)

@willhaines willhaines force-pushed the resource-pool branch 2 times, most recently from 308ed27 to 0fc796a Compare September 29, 2021 20:51
@jcpowermac
Copy link
Contributor

🤞 no ci issues: openshift/release#22354

/test e2e-vsphere

/test e2e-vsphere-upi

@jcpowermac
Copy link
Contributor

/test e2e-vsphere

/test e2e-vsphere-upi

@willhaines
Copy link
Contributor Author

Rebased on master. Had to slightly tweak how the resource pool id is passed around in terraform, to fit with changes made in #5094. Sorry if rebase/force-push makes that hard to see.

@jcpowermac
Copy link
Contributor

/remove-label do-not-merge/work-in-progress

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 15, 2021

@jcpowermac: The label(s) /remove-label do-not-merge/work-in-progress cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, backport-risk-assessed, cherry-pick-approved

In response to this:

/remove-label do-not-merge/work-in-progress

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
Copy link
Contributor

/retitle [wip] IPI for vSphere w/ existing resource pool

@openshift-ci openshift-ci bot changed the title IPI for vSphere w/ existing resource pool [wip] IPI for vSphere w/ existing resource pool Nov 15, 2021
@jcpowermac
Copy link
Contributor

/retitle IPI for vSphere w/ existing resource pool

@openshift-ci openshift-ci bot changed the title [wip] IPI for vSphere w/ existing resource pool IPI for vSphere w/ existing resource pool Nov 15, 2021
@openshift-ci 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 Nov 15, 2021
@jcpowermac
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2021
@jcpowermac
Copy link
Contributor

/assign @jstuever

@jcpowermac
Copy link
Contributor

Both upi and ipi installed correctly. Minor test failures imho are unrelated to this pr. Still 👍 for me.

@ayesha54
Copy link
Contributor

@ayesha54 if you have time can you please review this PR

sure

@jstuever
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 16, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever

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:

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 Nov 16, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

3 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@willhaines
Copy link
Contributor Author

/test e2e-aws-upgrade

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 17, 2021

@willhaines: 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-aws-fips 31f42c7 link false /test e2e-aws-fips
ci/prow/e2e-aws-workers-rhel8 31f42c7 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-aws-single-node 31f42c7 link false /test e2e-aws-single-node
ci/prow/e2e-aws-workers-rhel7 31f42c7 link false /test e2e-aws-workers-rhel7
ci/prow/okd-e2e-aws 31f42c7 link false /test okd-e2e-aws
ci/prow/okd-e2e-aws-upgrade 31f42c7 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-crc 31f42c7 link false /test e2e-crc
ci/prow/e2e-vsphere-upi 31f42c7 link false /test e2e-vsphere-upi
ci/prow/e2e-vsphere 31f42c7 link false /test e2e-vsphere

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-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPI for vSphere: Support deployment to vSphere Resource Pool
6 participants