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

BZ1849434 updating vCenter folder #24161

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Conversation

kalexand-rh
Copy link
Contributor

@kalexand-rh kalexand-rh added this to the Next Release milestone Jul 24, 2020
@kalexand-rh kalexand-rh self-assigned this Jul 24, 2020
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 24, 2020
@openshift-docs-preview-bot

The preview will be available shortly at:

Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

I hope these comments clarify. Please let me know if they are unclear. I will open a PR to help clarify the repo docs.

@@ -99,30 +100,31 @@ in vSphere.
<9> The password associated with the vSphere user.
<10> The vSphere datacenter.
<11> The default vSphere datastore to use.
<12> Whether to enable or disable FIPS mode. By default, FIPS mode is not enabled. If FIPS mode is enabled, the {op-system-first} machines that {product-title} runs on bypass the default Kubernetes cryptography suite and use the cryptography modules that are provided with {op-system} instead.
<12> Optional: The absolute path of an existing folder where the installation program creates the virtual machines, for example, `/<datacenter_name>/vm/<folder_name>/<subfolder_name>`. If you do not provide this value, the installation program creates a folder that is named with the infrastructure ID in the datacenter virtual machine folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<12> Optional: The absolute path of an existing folder where the installation program creates the virtual machines, for example, `/<datacenter_name>/vm/<folder_name>/<subfolder_name>`. If you do not provide this value, the installation program creates a folder that is named with the infrastructure ID in the datacenter virtual machine folder.
<12> Optional: The absolute path of an existing folder where the installation program creates the virtual machines, for example, `/<datacenter_name>/vm/<folder_name>/<subfolder_name>`. If you do not provide this value, the installation program points to a top-level folder that is named with the infrastructure ID in the datacenter virtual machine folder.

ifdef::vsphere[]
. Provide the name of the vCenter folder that hosts your cluster's virtual machines to the cloud provider:
.. Open the `<installation_directory>/manifests/cloud-provider-config.yaml` file.
.. Set the value of the `folder` parameter to match the value that you specified for the folder name in the `install-config.yaml` file or, if you did not specify a folder name, the cluster ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is not quite right, and I don't think we actually need it. This line is describing what the folder value in the cloud-provider-config will already be--no users steps are needed to achieve this.

@@ -118,7 +118,7 @@ endif::openshift-origin[]
.. Right-click the name of your datacenter.
.. Click *New Folder* -> *New VM and Template Folder*.
.. In the window that is displayed, enter the folder name. The folder name must
match the cluster name that you specified in the `install-config.yaml` file.
match the folder name that you specified in the `install-config.yaml` file or, if you did not specify a folder name, the infrastructure ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

@kalexand-rh
Copy link
Contributor Author

@patrickdillon, thank you so much! I've pushed some changes to try to address your feedback. Will you let me know if anything else is amiss?

Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

looks much better. I did leave a couple more comments becuase I am really trying to get this right so we can be done with it :)

@@ -482,7 +482,7 @@ in vSphere.
|String.

|`platform.vsphere.folder`
|_Optional_. The absolute path of an existing folder where the installation program creates the virtual machines. create VMs. If you do not provide this value, the installation program creates a folder that is named with the cluster ID is created in the datacenter virtual machine folder.
|_Optional_. The absolute path of an existing folder where the installation program creates the virtual machines. If you do not provide this value, the installation program creates a folder that is named with the infrastructure ID in the datacenter virtual machine folder.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only true for IPI. For UPI I would suggest:

"The absolute path to the virtual machine folder. The default value is a top-level folder named with the infrastructure ID.`

But I think this is shared beween IPI and UPI so not sure how to do that. My suggestion would technically be true for both cases, but is a little less informative for IPI.

Choose a reason for hiding this comment

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

I think most of the platform fields in install config was only valid for IPI because UPI is just pausing before IPI and then doing what the installer would have done ourselves because that allows user to do things differently. So i'm fine if this most accurate for IPI.

@@ -118,7 +118,7 @@ endif::openshift-origin[]
.. Right-click the name of your datacenter.
.. Click *New Folder* -> *New VM and Template Folder*.
.. In the window that is displayed, enter the folder name. The folder name must
match the cluster name that you specified in the `install-config.yaml` file.
match the folder name that you specified in the `install-config.yaml` file or, if you did not specify a folder name, the infrastructure ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is acceptable for UPI but it is probably simpler to say "if you didn't specify an existing folder in the install-config, then create a folder..."

Suggested change
match the folder name that you specified in the `install-config.yaml` file or, if you did not specify a folder name, the infrastructure ID.
the infrastructure ID.

One reason I say this is because the instructions technically describe how to create a top-level folder where users could specify a nested folder

@jinyunma
Copy link

jinyunma commented Aug 4, 2020

Sorry for the late. The content looks good to me.
One comment: folder update is only applicable on 4.5 and later release. @patrickdillon could you help to confirm?

@jinyunma
Copy link

jinyunma commented Aug 6, 2020

Sorry for the late. The content looks good to me.
One comment: folder update is only applicable on 4.5 and later release. @patrickdillon could you help to confirm?

Patrick is on the leave. @abhinavdahiya could you please help to confirm this? thanks.

@abhinavdahiya
Copy link

LGTM

@abhinavdahiya
Copy link

/lgtm

@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 22, 2020
@kalexand-rh
Copy link
Contributor Author

@jinyunma, will you PTAL?

@jinyunma
Copy link

New added part Extracting the infrastructure name seems missed in section Installing a cluster on vSphere with user-provisoned infrastructure and network customizations
"

@kalexand-rh
Copy link
Contributor Author

@jinyunma, thanks for pointing that out. I've added the Extracting the infrastructure name section to Installing a cluster on vSphere with user-provisoned infrastructure and network customizations. Do you see any other changes?

@jinyunma
Copy link

I read again the doc and have one comment:
sample install-config.yaml the description of option12 folder. For upi installation, installer program doesn't create folder named with infrastructure ID if user doesn't define folder in cluster-config.yaml, but need user to create folder named infrastructure ID. other parts are LGTM.

@kalexand-rh
Copy link
Contributor Author

Thank you @jinyunma! I've noted that that parameter is only valid for IPI.

Copy link
Contributor

@adellape adellape left a comment

Choose a reason for hiding this comment

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

👍

@adellape adellape added the peer-review-done Signifies that the peer review team has reviewed this PR label Sep 28, 2020
@jinyunma
Copy link

jinyunma commented Sep 29, 2020

@kalexand-rh, thanks for update, it looks good to me.

@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.5

@kalexand-rh
Copy link
Contributor Author

/cherrypick enterprise-4.6

@kalexand-rh kalexand-rh deleted the BZ1849434 branch September 29, 2020 18:39
@openshift-cherrypick-robot

@kalexand-rh: new pull request created: #25865

In response to this:

/cherrypick enterprise-4.5

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-cherrypick-robot

@kalexand-rh: new pull request created: #25866

In response to this:

/cherrypick enterprise-4.6

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch/enterprise-4.5 branch/enterprise-4.6 peer-review-done Signifies that the peer review team has reviewed this PR size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants