Bug 1826100: vSphere allow users to specify existing folder #3498

patrickdillon · 2020-04-23T00:00:29Z

Before this PR, we were always creating a folder with the cluster ID (incorrectly ignoring the folder specified in the install config). Now, if users specify a folder in the install config, terraform will expect there to be an existing folder and will use it for installation. If none is specified, a folder will be created with install config.

TODO:

set folder value from ic or default to cluster id in tfvars and cloud config
terraform: conditionally create folder resource or use existing folder data source
terraform: support nested folder data source
update destroy
validation to ensure specified folder exists
update readme/docs for folder paramater

Folder Paths
vSphere libraries seem to use a mixture of relative and absolute paths. Some of these issues only seem to arise when using nested folders; that is, a relative path is ok for a top-level folder but an absolute path is required for a nested folder. This is the case for the Machine API provider and terraform import_ova resource. The cloud config and terraform virtual machine resource need relative paths.

Validation
Adds static validation to ensure absolute path is provided in the install config:

FATAL failed to fetch Master Machines: failed to load asset "Install Config": invalid "install-config.yaml" file: platform.vsphere.folder: Invalid value: "/wrong/vm/padillon-test/subfolder": folder must be absolute path: expected prefix /dc1/vm/

Adds API validation to ensure that a specified folder exists when installing to the cluster:

FATAL failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Provisioning Check": platform.vsphere.folder: Invalid value: "/dc1/vm/wrong/subfolder": folder '/dc1/vm/wrong/subfolder' not found

openshift-ci-robot · 2020-04-23T00:00:33Z

@patrickdillon: This pull request references Bugzilla bug 1826100, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826100: vSphere allow users to specify existing folder [WIP]

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.

patrickdillon · 2020-04-23T00:00:53Z

/hold
WIP see TODO

patrickdillon · 2020-04-23T00:24:09Z

/test e2e-vsphere

abhinavdahiya · 2020-04-23T05:15:01Z

one thing we should make sure is that the cloudproviderconfig is also configured to match the changes here.

patrickdillon · 2020-04-23T13:03:49Z

/retest

patrickdillon · 2020-04-23T13:46:37Z

/retest

patrickdillon · 2020-04-23T13:47:11Z

cc @dav1x

patrickdillon · 2020-04-23T15:54:58Z

/retest
upi image is failing to d/l matchbox but url is working for me...
update: looks like github is having outages.

patrickdillon · 2020-04-23T17:43:19Z

/test e2e-vsphere

one thing we should make sure is that the cloudproviderconfig is also configured to match the changes here.

I updated 91151a4 with this change. vSphere cloud provider config was using the clustername--not cluster id.

API server seemed to be failing in the last run...

E0423 15:11:42.182268       1 reflector.go:178] github.com/openshift/client-go/user/informers/externalversions/factory.go:101: Failed to list *v1.Group: the server is currently unable to handle the request (get groups.user.openshift.io)
W0423 15:11:42.696634       1 clientconn.go:1208] grpc: addrConn.createTransport failed to connect to {https://localhost:2379  <nil> 0 <nil>}. Err :connection error: desc = "transport: Error while dialing dial tcp [::1]:2379: connect: connection refused". Reconnecting...

patrickdillon · 2020-04-24T23:15:03Z

/test e2e-vsphere

openshift-ci-robot · 2020-04-27T12:06:40Z

@patrickdillon: This pull request references Bugzilla bug 1826100, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826100: vSphere allow users to specify existing folder [WIP]

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.

patrickdillon · 2020-04-27T12:20:21Z

@jcpowermac This requires an update to destroy. Can you take a look at the implementation in the latest commit?

When destroying VMs we check whether VMs are tagged and have a tagged parent folder. If we don't create the folder, the folder isn't tagged. I took the easiest option and just removed the folder check, so now we just we delete any tagged VM regardless of parent folder.

From what I can see this seems perfectly acceptable. Do you know if the extra check is just a safeguard or does it account for any (corner) cases?

The other option would be to add the folder path to the cluster metadata and find a reference to untagged BYO folders by path. Not too much work but probably better to avoid unnecessary complexity if we don't need it.

jcpowermac · 2020-04-27T15:08:07Z

@jcpowermac This requires an update to destroy. Can you take a look at the implementation in the latest commit?

When destroying VMs we check whether VMs are tagged and have a tagged parent folder. If we don't create the folder, the folder isn't tagged. I took the easiest option and just removed the folder check, so now we just we delete any tagged VM regardless of parent folder.

From what I can see this seems perfectly acceptable. Do you know if the extra check is just a safeguard or does it account for any (corner) cases?

That is the only reason I wrote it that way...as a safeguard.

The other option would be to add the folder path to the cluster metadata and find a reference to untagged BYO folders by path. Not too much work but probably better to avoid unnecessary complexity if we don't need it.

@patrickdillon That seems perfectly fine to me.
I doubt that a customer will tag one of their virtual machines with the openshift created tag.

patrickdillon · 2020-04-28T14:19:31Z

/test e2e-vsphere

openshift-ci-robot · 2020-04-28T14:34:40Z

@patrickdillon: This pull request references Bugzilla bug 1826100, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826100: vSphere allow users to specify existing folder [WIP]

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.

abhinavdahiya · 2020-04-28T17:53:49Z

pkg/asset/installconfig/vsphere/validation.go

+
+	finder := find.NewFinder(vim25Client)
+
+	path := fmt.Sprintf("/%s/vm/%s", cfg.Datacenter, cfg.Folder)


what if the user already gave us the folder path prefixed with /<datacenter>/vm/ ?

we should try to error out saying you have prefixed it, we would like un-prefixed.

we should try to error out saying you have prefixed it, we would like un-prefixed.

I can easily add some static validation.

what if the user already gave us the folder path prefixed with //vm/

When create cluster is run validation would run to look for a folder called /<datacenter>/vm//<datacenter>/vm/. Assuming it doesn't exist, the install would abort.

I have probably been thinking about this too much, but my approach has been to rely on the API validation because it is difficult to determine what is actually a valid name and what isn't.

/<datacenter>/vm/<datacenter>/vm is technically a valid folder name. I haven't tested much with the leading /... vsphere seems to convert / to %20 or something like that.

I think we could probably safely statically validate a leading / and I can confirm that with a little more testing.

I also think it is a safe assumption that although it might be allowed no one will really name their folder /<datacenter>/vm/ and it is much more likely they are making a mistake.

I will work on adding these.

@rgolangh suggested requiring the absolute path, which could actually make things simpler and be easier to validate.

abhinavdahiya · 2020-04-28T17:54:34Z

docs/user/vsphere/customization.md

@@ -9,6 +9,7 @@ Beyond the [platform-agnostic `install-config.yaml` properties](../customization
 * `password` (required string): The password to use to connect to the vCenter.
 * `datacenter` (required string): The name of the datacenter to use in the vCenter.
 * `defaultDatastore` (required string): The default datastore to use for provisioning volumes.
+* `folder` (optional string): The name of the existing folder where the installer should create VMs. The value should be relative to the datacenter's VM folder. To install VMs in `/example_datacenter/vm/example_folder/example_subfolder` specify the value `example_folder/example_subfolder`. If a value is specified, the folder must exist. If no value is specified, a folder named with the cluster ID will be created in the `datacenter` VM folder.


can we update the godoc for the field inaddition to this customization document.

patrickdillon · 2020-04-28T19:23:36Z

Hm. Machine API cannot find nested folder when I specify relative path.

This PR is 2 steps forward 1 step back.

/this-is-fine

openshift-ci-robot · 2020-04-28T19:23:37Z

@patrickdillon:

In response to this:

Hm. Machine API cannot find nested folder when I specify relative path.

This PR is 2 steps forward 1 step back.

/this-is-fine

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.

abhinavdahiya · 2020-05-05T14:15:43Z

pkg/asset/installconfig/vsphere/validation.go

@@ -33,3 +39,30 @@ func ValidateForProvisioning(ic *types.InstallConfig) error {

 	return allErrs.ToAggregate()
 }
+


There missing validation test for this, but we can add those later on...

openshift-ci-robot · 2020-05-05T14:16:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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~~ [abhinavdahiya]

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

openshift-bot · 2020-05-05T14:23:30Z

/retest

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

openshift-bot · 2020-05-05T14:36:28Z

/retest

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

openshift-bot · 2020-05-05T15:02:27Z

/retest

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

openshift-bot · 2020-05-05T15:15:28Z

/retest

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

openshift-bot · 2020-05-05T15:28:37Z

/retest

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

openshift-bot · 2020-05-05T16:46:24Z

/retest

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

openshift-bot · 2020-05-05T16:59:27Z

/retest

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

openshift-bot · 2020-05-05T17:12:26Z

/retest

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

openshift-bot · 2020-05-05T18:17:25Z

/retest

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

openshift-bot · 2020-05-05T18:43:47Z

/retest

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

openshift-bot · 2020-05-05T19:48:37Z

/retest

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

openshift-bot · 2020-05-05T20:14:31Z

/retest

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

openshift-ci-robot · 2020-05-05T20:21:27Z

@patrickdillon: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovirt	`2d49e7f`	link	`/test e2e-ovirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 · 2020-05-05T20:27:55Z

/retest

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

openshift-bot · 2020-05-05T20:41:12Z

/retest

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

openshift-ci-robot · 2020-05-05T21:24:18Z

@patrickdillon: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:

openshift/installer#3485 is open
Bugzilla bug 1826100 has been moved to the MODIFIED state.

In response to this:

Bug 1826100: vSphere allow users to specify existing folder

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.

Some users may want to use IPI on a shared vSphere cluster, where they do not have full admin privileges. This allows them to use IPI with privileges mostly confined to an existing resource pool. Compliments PR openshift#3498.

openshift-ci-robot added the bugzilla/unspecified label Apr 23, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 23, 2020

openshift-ci-robot requested review from jcpowermac and jhixson74 April 23, 2020 00:00

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2020

patrickdillon force-pushed the vsphere-folder-bz-1826100 branch from 5a5fd7d to c735917 Compare April 23, 2020 17:37

patrickdillon force-pushed the vsphere-folder-bz-1826100 branch from c735917 to 01bba68 Compare April 27, 2020 12:00

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 27, 2020

patrickdillon force-pushed the vsphere-folder-bz-1826100 branch from 01bba68 to e7f6cc5 Compare April 28, 2020 13:07

openshift-ci-robot removed the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 28, 2020

openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Apr 28, 2020

abhinavdahiya reviewed Apr 28, 2020

View reviewed changes

abhinavdahiya approved these changes May 5, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2020

openshift-merge-robot merged commit 134cd61 into openshift:master May 5, 2020

abhinavdahiya mentioned this pull request May 8, 2020

Bug 1826100: - vSphere IPI - Platform and cloud provider read Folder but machine creation still using clusterID #3485

Closed

patrickdillon mentioned this pull request May 13, 2020

Bug 1835438: move vSphere folder validation to provisioning #3597

Merged

willhaines mentioned this pull request Aug 5, 2021

IPI for vSphere w/ existing resource pool #5136

Merged

1 task


		finder := find.NewFinder(vim25Client)

		path := fmt.Sprintf("/%s/vm/%s", cfg.Datacenter, cfg.Folder)

		@@ -33,3 +39,30 @@ func ValidateForProvisioning(ic *types.InstallConfig) error {

		return allErrs.ToAggregate()
		}

Bug 1826100: vSphere allow users to specify existing folder #3498

Bug 1826100: vSphere allow users to specify existing folder #3498

Conversation

patrickdillon commented Apr 23, 2020 • edited

openshift-ci-robot commented Apr 23, 2020

patrickdillon commented Apr 23, 2020

patrickdillon commented Apr 23, 2020

abhinavdahiya commented Apr 23, 2020

patrickdillon commented Apr 23, 2020

patrickdillon commented Apr 23, 2020

patrickdillon commented Apr 23, 2020

patrickdillon commented Apr 23, 2020 • edited

patrickdillon commented Apr 23, 2020

patrickdillon commented Apr 24, 2020

openshift-ci-robot commented Apr 27, 2020

patrickdillon commented Apr 27, 2020

jcpowermac commented Apr 27, 2020

patrickdillon commented Apr 28, 2020

openshift-ci-robot commented Apr 28, 2020

abhinavdahiya Apr 28, 2020

Choose a reason for hiding this comment

abhinavdahiya Apr 28, 2020

Choose a reason for hiding this comment

patrickdillon Apr 28, 2020

Choose a reason for hiding this comment

patrickdillon Apr 28, 2020 • edited

Choose a reason for hiding this comment

abhinavdahiya Apr 28, 2020

Choose a reason for hiding this comment

patrickdillon commented Apr 28, 2020

openshift-ci-robot commented Apr 28, 2020

abhinavdahiya May 5, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-bot commented May 5, 2020

openshift-ci-robot commented May 5, 2020

patrickdillon commented Apr 23, 2020 •

edited

patrickdillon commented Apr 23, 2020 •

edited

patrickdillon Apr 28, 2020 •

edited