Skip to content

OSDOCS-3435: Nutanix IPI installation#44537

Merged
kalexand-rh merged 1 commit intoopenshift:mainfrom
mjpytlak:osdocs-3435
Jul 22, 2022
Merged

OSDOCS-3435: Nutanix IPI installation#44537
kalexand-rh merged 1 commit intoopenshift:mainfrom
mjpytlak:osdocs-3435

Conversation

@mjpytlak
Copy link
Copy Markdown
Contributor

@mjpytlak mjpytlak commented Apr 12, 2022

@openshift-ci openshift-ci Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 12, 2022
@netlify
Copy link
Copy Markdown

netlify Bot commented Apr 12, 2022

Deploy Preview for osdocs ready!

Name Link
🔨 Latest commit ea60e2ddc64bf8df8c043c63230da3e6b0fa15c9
🔍 Latest deploy log https://app.netlify.com/sites/osdocs/deploys/627966f54bceba0009097d89
😎 Deploy Preview https://deploy-preview-44537--osdocs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@mjpytlak mjpytlak force-pushed the osdocs-3435 branch 3 times, most recently from d9719b2 to f116092 Compare April 13, 2022 20:14
@openshift-ci 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 Apr 13, 2022
Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak Apr 14, 2022

Choose a reason for hiding this comment

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

We need to cover installing the default storage container after installing the cluster. Are these steps documented somewhere or should I schedule some time to step through them?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The current plan is to use the existing documentation here. We also need to include documentation on configuring registry storage, which should be published here. I also think we should include the CLI install doc before these two (like in current vSphere doc).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vnephologist we can't add links to GitHub repositories as a documentation reference. Could you please provide the links to the Nutanix official documentation page as you do have for general Kubernetes --> https://portal.nutanix.com/page/documents/details?targetId=CSI-Volume-Driver-v2_5:CSI-Volume-Driver-v2_5

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@makentenza We don't plan to publish this doc on our traditional portal. If you're against GitHub doc, then we have another option we're working on. We can discuss offline.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added to doc. Thank you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are there any other components we need to account for? Are AOS, AHV, and Prism Central versioned separately? If they are upgraded separately, we might have to explicitly call out what combinations we are supporting. Or, when I upgrade AOS, do other components automatically get upgraded?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We just need Nutanix AOS, and Prism Central. They are upgraded separately but the plan is to limit scope by supporting the latest Nutanix release unless it is initial release in train (eg x.y.0) at OCP release time.

Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak May 19, 2022

Choose a reason for hiding this comment

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

supporting the latest Nutanix release unless it is initial release in train (eg x.y.0) at OCP release time.

Do you know which AOS and Prism Central releases that will be or is this something we will need to revisit towards the end of July?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@mjpytlak Nutanix AOS 5.20.4 and 6.1.1 with Prism Central 2022.4

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added to doc.Thank you.

Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak Apr 14, 2022

Choose a reason for hiding this comment

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

Is "global administrative privileges" the correct term to describe a Prism Central admin account that has the necessary level of privileges to install a cluster. If restrictions within an organization prevent a user account from having global privileges, what subset of privileges are required.

Example of this doc: vCenter requirements > Required vCenter account privileges (https://docs.openshift.com/container-platform/4.10/installing/installing_vsphere/installing-vsphere-installer-provisioned.html#installation-vsphere-installer-infra-requirements_installing-vsphere-installer-provisioned)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finer grained roles are not yet supported. Accounts hold admin privileges.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rvanderp3 To clarify - I am interpreting your comment as that the doc simply state that the account that is used to install the cluster requires administrative privileges and that the subsequent tables that detail Roles and privileges required for installation and Required permissions and propagation settings should be deleted. Is that correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjpytlak yes, that is correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread modules/nutanix-csi-driver-reqs.adoc Outdated
Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak Apr 14, 2022

Choose a reason for hiding this comment

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

Given that storage is day-2 item, is this section even applicable? If it is applicable, I suspect we would need to move the requirements into "Configuring the default storage container"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, I think we can cover this in the "Configuring the default storage container" section.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the section.

Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak Apr 14, 2022

Choose a reason for hiding this comment

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

"Cluster limits", "Cluster resources", and "Networking requirements" are largely boilerplate. Is there anything specific to Nutanix that should be added/removed?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Added some inline suggestions.

Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak May 19, 2022

Choose a reason for hiding this comment

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

Thank you. Updated.

@mjpytlak
Copy link
Copy Markdown
Contributor Author

@jeana-redhat FYI.

@jeana-redhat
Copy link
Copy Markdown
Contributor

Thanks Mike! Per our conversation today:

I will work with the CCO dev and QA folks on the CCO specific content in a separate PR, and get you the finalized results of that to blend into the overall install flow 👍

@mjpytlak
Copy link
Copy Markdown
Contributor Author

mjpytlak commented May 2, 2022

@vnephologist A few questions about the CSI operator and creating object storage for the register.

From an earlier conversation with @makentenza. do these represent the correct doc to be added to the help?

IIRC correctly, there was some discussion if the doc around creating storage would live in the Red Hat help or would live under the Nutanix domain. Has a decision be made?

@lpettyjo (storage writer) for awareness.

Comment on lines 1491 to 1625
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@thunderboltsid i think we need to update this based on schema updates in install-config.yaml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

updated schema for the platform spec

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indeed, this needs to be updated to reflect newer install-config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not believe there are machine types defined for nutanix.

Copy link
Copy Markdown
Contributor Author

@mjpytlak mjpytlak May 4, 2022

Choose a reason for hiding this comment

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

@rvanderp3 While machine types might not be defined for Nutanix, is there anything else we might suggest to help mitigate degraded performance if multithreading is disabled? For example the vShpere doc states that Your machines must use at least 8 CPUs and 32 GB of RAM if you disable simultaneous multithreading.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mjpytlak For VMware, we only include the first sentence. It will be up to the admin to ensure they have enough CPU resource to cover the need if SMT is disabled.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 20 to 34
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should resemble the vSphere platform config. The only deviations are memoryMB and diskSizeGB parameters. For Nutanix, these parameters are memoryMiB and diskSizeMiB respectively.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I could have sworn I based the Nutanix sample on the vSphere sample, but clearly I didn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 128 to 139
Copy link
Copy Markdown
Contributor

@rvanderp3 rvanderp3 May 2, 2022

Choose a reason for hiding this comment

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

This section should not be required as the ccoutl is not creating credentials in the nutanix environment.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Indeed, delete-service-id is not a valid subcommand for ccoctl nutanix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

@rvanderp3 rvanderp3 left a comment

Choose a reason for hiding this comment

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

I left a few comments(as well as some that I added prior to starting the review). I think we need a section called Installing a cluster on Nutanix with customizations as well. Installers may want to tune aspects of the install-config.yaml.

Comment on lines 47 to 56
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
platform:
nutanix:
apiVIP: 10.40.142.7 <1>
ingressVIP: 10.40.142.8 <1>
password: samplepassword <1>
port: "9440" <1>
prismCentral: your.prismcentral.domainname <1>
prismElementUUID: 0005b0f1-8f43-a0f2-02b7-3cecef193712
subnetUUID: c7938dc6-7659-453e-a688-e26020c68e43
username: sampleadmin
platform:
nutanix:
apiVIP: 10.40.142.7
ingressVIP: 10.40.142.8
prismCentral:
endpoint:
address: your.prismcentral.domainname
port: 9440
password: samplepassword
username: sampleadmin
prismElements:
- endpoint:
address: your.prismelement.domainname
port: 9440
uuid: 0005b0f1-8f43-a0f2-02b7-3cecef193712
subnetUUIDs:
- c7938dc6-7659-453e-a688-e26020c68e43

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 1491 to 1625
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

updated schema for the platform spec

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finer grained roles are not yet supported. Accounts hold admin privileges.

Comment on lines 42 to 46
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
----
$ oc adm release extract --credentials-requests --cloud=nutanix \
--to=<path_to_directory_with_list_of_credentials_requests>/credrequests \
quay.io/<path_to>/ocp-release:<version>
----
----
$ RELEASE_IMAGE=$(./openshift-install version | awk '/release image/ {print $3}')
$ oc adm release extract --credentials-requests --cloud=nutanix \
--to=<path_to_directory_with_list_of_credentials_requests>/credrequests \
$RELEASE_IMAGE -a pull-secret
----
. Extract `ccoutl` and make it executable
----
$ CCO_IMAGE=$(oc adm release info --image-for='cloud-credential-operator' $RELEASE_IMAGE -a pull-secret)
$ oc image extract $CCO_IMAGE --file="/usr/bin/ccoctl -a pull-secret"
$ chmod 775 ccoctl
----

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jeana-redhat Some feedback on putting the CCO into manual mode. Worth noting that the draft doc details how to extract the ccoctl binary and make it executable in Configuring the Cloud Credential Operator utility.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we might want to consider replacing:

$ oc adm release extract --credentials-requests --cloud=nutanix \
--to=<path_to_directory_with_list_of_credentials_requests>/credrequests \
quay.io/<path_to>/ocp-release:<version>

with something like:

RELEASE_IMAGE=$(./openshift-install version | awk '/release image/ {print $3}')
$ oc adm release extract --credentials-requests --cloud=nutanix \
--to=<path_to_directory_with_list_of_credentials_requests>/credrequests \
$RELEASE_IMAGE -a pull-secret

this makes getting the release image less ambiguous

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@rvanderp3 I might be confused, but I believe the steps you have added here are already in the docs under "Configuring the Cloud Credential Operator utility". I've been working with Akhil on the CCO-specific stuff in a different PR for Mike to pull into this one, you can take a look at it here to see if you think it makes sense :)
mjpytlak#1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh it is very possible i'm confused :) . i was performing an install yesterday based on the docs rendered from this PR and ran across this. if this is already being handled in a different PR, i think we are good.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

no worries! Hopefully early next week my PR will be validated and Mike can pull it into here so it's all in one place and folks can step through it in its entirety 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rvanderp3 My latest commit pulls in @jeana-redhat work. Still waiting on validation from QE on the CCO-specific material, but Jeana believes the content is in good shape. I will be sure to pick up any updates from the pending review, if required.

@mjpytlak
Copy link
Copy Markdown
Contributor Author

mjpytlak commented May 3, 2022

I think we need a section called Installing a cluster on Nutanix with customizations as well. Installers may want to tune aspects of the install-config.yaml.

@rvanderp3 When @makentenza and I discussed doc for 4.11, he believed that the standard IPI installation would be enough. @makentenza thoughts here?

@rvanderp3 I actually replied too quickly to your comment and failed to remember that I structured the topic in a way that accommodates both a standard IPI install and an IPI install with customizations. From a doc perspective, the difference between each of the latter amounts to updating a few procedures and adding several extra sections – the most notable difference, however, is whether customers must create the install configuration file before deploying the cluster.

In the case of Nutanix, customers must create the installation configuration file before deploying the cluster to set the CCO to manual mode. Given the later, step 2 in Creating the installation configuration file instructs users to modify the install configuration file (albeit I need to wordsmith this to among other things, say it is an optional step). The remainder of the content that would appear in a "customizations" topic is already included, so our customization case is covered.

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2022
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2022
@mjpytlak
Copy link
Copy Markdown
Contributor Author

mjpytlak commented May 9, 2022

@rvanderp3 Thank you for the review. Outside of the steps on configuring the CCO, which is handled by another writer, I have addressed all other feedback. Please confirm that the updates look good.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a bit confusing. The following is the equiv of the way the network is referenced in vSphere doc... "The Prism Element network that contains the virtual IP addresses and DNS records that you configured."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you done.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

N/A

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
** 1 template
** 1 disk image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@mjpytlak
Copy link
Copy Markdown
Contributor Author

mjpytlak commented Jul 1, 2022

@sgaoshang Ready for QE review. PTAL. Please note we are still waiting on a few last items[1], but overall the content is in good shape. Thank you!

[1] #44537 (comment)

Comment thread modules/cco-ccoctl-configuring.adoc Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The sample output of "ccoctl --help" is old and without nutanix option.

It can be updated based on the following new output sample:
[cloud-user@preserve-for-hive-test]$ ./ccoctl --help
OpenShift credentials provisioning tool

Usage:
ccoctl [command]

Available Commands:
alibabacloud Manage credentials objects for alibaba cloud
aws Manage credentials objects for AWS cloud
completion generate the autocompletion script for the specified shell
gcp Manage credentials objects for Google cloud
help Help about any command
ibmcloud Manage credentials objects for IBM Cloud
nutanix Manage credentials objects for Nutanix

Flags:
-h, --help help for ccoctl

Use "ccoctl [command] --help" for more information about a command.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you.

Copy link
Copy Markdown

@jianping-shu jianping-shu Jul 8, 2022

Choose a reason for hiding this comment

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

Note 1 may need update.
Refer to https://bugzilla.redhat.com/show_bug.cgi?id=2097051
In which note 1 is "credrequests is the directory where the list of CredentialsRequest objects is stored. This command creates the directory if it does not exist."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
<1> Specify the path to the directory that contains the files for the component `CredentialsRequests` objects.
<1> Specify the path to the directory that contains the files for the component `CredentialsRequests` objects. If the specified directory does not exist, this command creates it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
#Is there a sample output here?#
total 64
-rw-r----- 1 <user> <user> 2335 Jul 8 12:22 cluster-config.yaml
-rw-r----- 1 <user> <user> 161 Jul 8 12:22 cluster-dns-02-config.yml
-rw-r----- 1 <user> <user> 864 Jul 8 12:22 cluster-infrastructure-02-config.yml
-rw-r----- 1 <user> <user> 191 Jul 8 12:22 cluster-ingress-02-config.yml
-rw-r----- 1 <user> <user> 9607 Jul 8 12:22 cluster-network-01-crd.yml
-rw-r----- 1 <user> <user> 272 Jul 8 12:22 cluster-network-02-config.yml
-rw-r----- 1 <user> <user> 142 Jul 8 12:22 cluster-proxy-01-config.yaml
-rw-r----- 1 <user> <user> 171 Jul 8 12:22 cluster-scheduler-02-config.yml
-rw-r----- 1 <user> <user> 200 Jul 8 12:22 cvo-overrides.yaml
-rw-r----- 1 <user> <user> 118 Jul 8 12:22 kube-cloud-config.yaml
-rw-r----- 1 <user> <user> 1304 Jul 8 12:22 kube-system-configmap-root-ca.yaml
-rw-r----- 1 <user> <user> 4090 Jul 8 12:22 machine-config-server-tls-secret.yaml
-rw-r----- 1 <user> <user> 3961 Jul 8 12:22 openshift-config-secret-pull-secret.yaml
-rw------- 1 <user> <user> 283 Jul 8 12:24 openshift-machine-api-nutanix-credentials-credentials.yaml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you.

@sgaoshang
Copy link
Copy Markdown

lgtm

@kalexand-rh kalexand-rh added peer-review-needed Signifies that the peer review team needs to review this PR branch/enterprise-4.11 labels Jul 20, 2022
@kalexand-rh kalexand-rh added this to the Future Release milestone Jul 20, 2022
Copy link
Copy Markdown
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

I think this is coming together nicely.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

! using noun
To avoid ambiguity, replace this gerund with either "by using" or "that uses".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Thank you.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Before you install a cluster, be sure that your Nutanix environment meets the following requirements.
Before you install an {product-title} cluster, be sure that your Nutanix environment meets the following requirements.

I think you mean the OCP cluster, not the Nutanix one. In general, make sure that it's clear enough which "cluster" you mean.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch.

Comment thread modules/cco-ccoctl-configuring.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please double-check that this is the only intro statement for Alibaba. If it is, your conditionals are good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. IIRC @jeana-redhat had worked with Michael Burke when updating this module for the Alibaba work. I do believe we are OK here, but can make updates if necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
|The configuration for the specific platform upon which to perform the installation: `alibabacloud`, `aws`, `baremetal`, `azure`, `ibmcloud`, `nutanix` `openstack`, `ovirt`, `vsphere`, or `{}`. For additional information about `platform.<platform>` parameters, consult the table for your specific platform that follows.
|The configuration for the specific platform upon which to perform the installation: `alibabacloud`, `aws`, `baremetal`, `azure`, `ibmcloud`, `nutanix`, `openstack`, `ovirt`, `vsphere`, or `{}`. For additional information about `platform.<platform>` parameters, consult the table for your specific platform that follows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Eagle eye. Thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
|The username that is used to log into Prism Central.
|The user name that is used to log into Prism Central.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about this directory, both here and later in the procedure. Are you trying to say that you specify a directory so this command can create a manifests directory in it?

Suggested change
<2> Specify the directory that contains the files of the component credentials secrets, under the `manifests` directory. By default, `ccoctl` creates objects in the directory in which the commands are run. To create the objects in a different directory, use the `--output-dir` flag.
<2> Specify the directory that contains the files of the component credentials secrets, under the `manifests` directory. By default, the `ccoctl` tool creates objects in the directory in which the commands are run. To create the objects in a different directory, use the `--output-dir` flag.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused about this directory, both here and later in the procedure. Are you trying to say that you specify a directory so this command can create a manifests directory in it?

@jeana-redhat Thoughts on Kathryn's comment?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, yes this could be better stated. It would be a change in multiple (already published) versions of this procedure for different platforms, so probably easier to handle in one go as a separate effort.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
$ openshift-install create manifests --dir <installation_directory><1>
$ openshift-install create manifests --dir <installation_directory> <1>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread modules/nutanix-csi-driver-reqs.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Be sure to add this module to an assembly when you complete it or remove it from the PR if you cannot.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the housing keeping reminder. This module is not required. Deleted it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a bit out of sequence - the instructions to install oc are later in the assembly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Moved the the oc ahead of this one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider being more specific about which secrets the user should be looking for. (I'm not sure which ones are appropriate.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jeana-redhat not sure if you can/should be more explicit here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would have to check with dev on that, and I expect it varies from release to release. There might be a way for them to check it against something else so we don't have to maintain a list and change it as the product changes. Good backlog item (I bet this applies to other platforms as well, historically we have not been super explicit on this type of topic).

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jul 20, 2022
@mjpytlak
Copy link
Copy Markdown
Contributor Author

mjpytlak commented Jul 21, 2022

@sgaoshang My latest commit addresses a late breaking bug [1]. PTAL at the small change I made to the values for metadata.name Thanks.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2104374

@sgaoshang
Copy link
Copy Markdown

Lgtm, thanks.

@vnephologist
Copy link
Copy Markdown

lgtm, great work @mjpytlak and team!

@kalexand-rh kalexand-rh merged commit 3d348b1 into openshift:main Jul 22, 2022
@kalexand-rh
Copy link
Copy Markdown
Contributor

/cherrypick enterprise-4.11

@openshift-cherrypick-robot
Copy link
Copy Markdown

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

Details

In response to this:

/cherrypick enterprise-4.11

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.11 peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.