OSDOCS#9001: Adding vSphere credentials to Agent#70551
OSDOCS#9001: Adding vSphere credentials to Agent#70551JoeAldinger merged 1 commit intoopenshift:mainfrom
Conversation
|
@skopacz1: This pull request references OSDOCS-9001 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
🤖 Thu Feb 15 17:16:45 - Prow CI generated the docs preview: https://70551--ocpdocs-pr.netlify.app |
|
@skopacz1: No Jira issue is referenced in the title of this pull request. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
I used the following logic to selectively include existing vSphere config parameters in the Agent docs:
- The entire vSphere section, which previously was included only in the vSphere configuration page, has been modified to be included in both the vSphere and Agent configuration pages
- Within that section of vSphere content, I used vSphere-only conditionals to exclude parameters that only belong in the vSphere page and not the Agent page.
- Within that section of vSphere content, I used Agent-only conditionals to exclude parameters that only belong in the Agent page and cannot yet be put in the vSphere page (due to time constraints for reviewing before the 4.15 GA deadline).
|
Hi @skopacz1. I think the existing vsphere config parameters are missing details. Under platform.vsphere.vcenters, there are additional parameters like server, user, password, port, and datacenters. Under platform.vsphere.failureDomains.topology, there are additional parameters like computeCluster, datacenter, datastore, resourcePool, and folder that should be described in greater detail.
|
| |platform: | ||
| vsphere: | ||
| vcenters: | ||
| password: |
There was a problem hiding this comment.
each vCenter is a separate entry in the vcenters slice. this allows multiple vCenters to be defined potentially(though its not supported at the moment).
| password: | |
| - password: <pw> | |
| port: 443 | |
| server: <vcenter fqdn> |
There was a problem hiding this comment.
That makes total sense. Each param is depicted this way because the install writers changed the old config format in the reference tables (e.g. platform.vsphere.vcenters.password) to this newer one that saves horizontal space. We're relying on the description/values column to convey things like what's optional and what needs to go together as a set.
For example, for the compute section of the install-config, instead of documenting architecture, name, platform, etc like this, we rely on the value field of compute.architecture to describe the structure: "Array of MachinePool objects"
| |platform: | ||
| vsphere: | ||
| failureDomains: | ||
| topology: | ||
| computeCluster: | ||
| |The path to the vSphere compute cluster. | ||
| |String | ||
|
|
||
| |platform: | ||
| vsphere: | ||
| failureDomains: | ||
| topology: | ||
| datacenter: | ||
| |Lists and defines the datacenters where {product-title} virtual machines (VMs) operate. | ||
| The list of datacenters must match the list of datacenters specified in the `vcenters` field. | ||
| |String | ||
|
|
||
| |platform: | ||
| vsphere: | ||
| failureDomains: | ||
| topology: | ||
| datastore: | ||
| |The path to the vSphere datastore that holds virtual machine files, templates, and ISO images. | ||
| [IMPORTANT] | ||
| ==== | ||
| You can specify the path of any datastore that exists in a datastore cluster. | ||
| By default, Storage vMotion is automatically enabled for a datastore cluster. | ||
| Red{nbsp}Hat does not support Storage vMotion, so you must disable Storage vMotion to avoid data loss issues for your {product-title} cluster. | ||
|
|
||
| If you must specify VMs across multiple datastores, use a `datastore` object to specify a failure domain in your cluster's `install-config.yaml` configuration file. | ||
| For more information, see "VMware vSphere region and zone enablement". | ||
| ==== | ||
| |String | ||
|
|
||
| |platform: | ||
| vsphere: | ||
| failureDomains: | ||
| topology: | ||
| resourcePool: | ||
| |Optional. The absolute path of an existing resource pool where the installation program creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. | ||
| If you do not specify a value, resources are installed in the root of the cluster, for example `/example_datacenter/host/example_cluster/Resources`. | ||
| |String | ||
|
|
||
| |platform: | ||
| vsphere: | ||
| failureDomains: | ||
| topology: | ||
| folder: | ||
| |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 top-level folder in the datacenter virtual machine folder that is named with the infrastructure ID. | ||
| If you are providing the infrastructure for the cluster and you do not want to use the default `StorageClass` object, named `thin`, you can omit the `folder` parameter from the `install-config.yaml` file. | ||
| |String | ||
|
|
There was a problem hiding this comment.
|
@mhanss could you PTAL when you have a chance? |
| vsphere: | ||
| failureDomains: | ||
| name: | ||
| |The name of the vCenter server. |
There was a problem hiding this comment.
I think it should be 'failure domain name' instead of 'vCenter server'.
| |Optional. The absolute path of an existing resource pool where the installation program creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. | ||
| If you do not specify a value, resources are installed in the root of the cluster, for example `/example_datacenter/host/example_cluster/Resources`. |
There was a problem hiding this comment.
For agent-based installations, users must specify the resource pool where the VMs are created because the installer does not create the VMs in this scenario.
There was a problem hiding this comment.
If I'm interpreting this correctly, I'll get rid of the last line about "not specifying a value". I'm assuming it's still technically an Optional parameter for ABI, but let me know if that's not the case.
| |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 top-level folder in the datacenter virtual machine folder that is named with the infrastructure ID. | ||
| If you are providing the infrastructure for the cluster and you do not want to use the default `StorageClass` object, named `thin`, you can omit the `folder` parameter from the `install-config.yaml` file. |
There was a problem hiding this comment.
For agent-based installations, users must specify the folder where the VMs are created because the installer does not create the VMs in this scenario.
There was a problem hiding this comment.
If I'm interpreting this right, I'll get rid of line 2726 about "not providing the value". I'm assuming the line below it can stay and that this parameter is still Optional for ABI, but let me know if that's not the case.
321d5ea to
e323ea3
Compare
|
New changes are detected. LGTM label has been removed. |
| failureDomains: | ||
| topology: | ||
| resourcePool: | ||
| |Optional. The absolute path of an existing resource pool where the installation program creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. |
There was a problem hiding this comment.
In case of ABI, the user creates the machines, not the installer program.
| |Optional. The absolute path of an existing resource pool where the installation program creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. | |
| |Optional. The absolute path of an existing resource pool where the user has created the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. |
| |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>`. | ||
| // Commenting out the line below because it doesn't apply to Agent, but it may be needed when this content is brought into the regular vSphere docs. | ||
| // If you do not provide this value, the installation program creates a top-level folder in the datacenter virtual machine folder that is named with the infrastructure ID. | ||
| If you are providing the infrastructure for the cluster and you do not want to use the default `StorageClass` object, named `thin`, you can omit the `folder` parameter from the `install-config.yaml` file. |
There was a problem hiding this comment.
In case of ABI, the user creates the machines, not the installer program. Also, I am unable to create an ISO without providing the folder, so it is a required field. Since it is a required field for ABI, we also need to modify this line "you can omit the folder parameter from the install-config.yaml file."
cc: @rwsu
There was a problem hiding this comment.
Sounds good, I modified the first line to say "folder where the user has created the virtual machines".
Since the last sentence is based on omitting a required parameter, which we can't do, should I get rid of the sentence entirely, or is there a different way to perform the functionality described there?
There was a problem hiding this comment.
Per conversation on Slack, I'll remove that last sentence entirely and double check with Richard once it's removed.
There was a problem hiding this comment.
Manoj is correct. folder is not optional and is a required parameter. I think line 2729 should be commented out.
e323ea3 to
4e860cc
Compare
michaelryanpeter
left a comment
There was a problem hiding this comment.
I left a couple of things to consider. Please reach out if you want to discuss anything. Great work!
/label peer-review-done
/remove-label peer-review-needed
/remove-label peer-review-in-progress
|
|
||
| |platform: | ||
| vsphere: | ||
| | Describes your account on the cloud platform that hosts your cluster. You can use the parameter to customize the platform. When providing additional configuration settings for compute and control plane machines in the machine pool, the parameter is optional. You can only specify one vCenter server for your {product-title} cluster. |
There was a problem hiding this comment.
See the ISG re: optional vs. conditional.
So, I wonder if the following would be more accurate:
| | Describes your account on the cloud platform that hosts your cluster. You can use the parameter to customize the platform. When providing additional configuration settings for compute and control plane machines in the machine pool, the parameter is optional. You can only specify one vCenter server for your {product-title} cluster. | |
| | Describes your account on the cloud platform that hosts your cluster. You can use the parameter to customize the platform. If you provide additional configuration settings for compute and control plane machines in the machine pool, the parameter is not required. You can only specify one vCenter server for your {product-title} cluster. |
| topology: | ||
| resourcePool: | ||
| // When this content is added to vSphere docs, the line below should like say "where the installation program creates the virtual machines". | ||
| |Optional. The absolute path of an existing resource pool where the user creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. |
There was a problem hiding this comment.
| |Optional. The absolute path of an existing resource pool where the user creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. | |
| |Optional: The absolute path of an existing resource pool where the user creates the virtual machines, for example, `/<datacenter_name>/host/<cluster_name>/Resources/<resource_pool_name>/<optional_nested_resource_pool_name>`. |
| vsphere: | ||
| vcenters: | ||
| |Configures the connection details for services to communicate with vCenter. | ||
| Currently only a single vCenter is supported. |
There was a problem hiding this comment.
| Currently only a single vCenter is supported. | |
| Currently, only a single vCenter is supported. |
| |platform: | ||
| vsphere: | ||
| vcenters: | ||
| |Configures the connection details for services to communicate with vCenter. |
There was a problem hiding this comment.
| |Configures the connection details for services to communicate with vCenter. | |
| |Configures the connection details for services that communicate with vCenter. |
Just a nit, and I am not sure I am right. to just feels off to my ear, but maybe that changes the meaning too much. Feel free to ignore if this isn't helpful.
There was a problem hiding this comment.
Maybe "Configures the connection details so that services can communicate with vCenter"?
There was a problem hiding this comment.
I think either of your suggestions work, although I'm not familiar enough with the content to know if the first one is still totally accurate. I think I prefer your second suggestion and will go with that one.
4420fa0 to
71c0210
Compare
|
@skopacz1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/label merge-review-needed |
JoeAldinger
left a comment
There was a problem hiding this comment.
One quick question just to make sure things are good.
|
|
||
| ifdef::vsphere[] | ||
|
|
||
| ifdef::agent,vsphere[] |
There was a problem hiding this comment.
I'm not sure I understand why you opened this ifdef here and then also on line 2616. I think my question is do you need line 2616? Just checking/learning: is it because you close this opening one down on line 2958?
There was a problem hiding this comment.
So technically this ifdef and the one on line 2616 are different because they check for slightly different conditions. This ifdef, which is closed on line 2958, basically says "Rule # 1: all of the content within these lines can be included if agent OR vsphere is true, unless there's an exception".
The ifdef on line 2616 (which closes on line 2622) is basically treated like an exception to that agent OR vsphere condition, because it ONLY includes content for agent. It's basically saying "here's an exception to rule # 1, the content from lines 2616 to 2622 can be included ONLY if agent is true, but can't be included in vsphere". Let me know if that makes sense, or if you'd like me to explain further.
TLDR the ifdef on line 2606 makes a general rule and the ifdefs like the ones on lines 2616 and 2623 make exceptions to that rule in order to be more specific
There was a problem hiding this comment.
I'm picking it up now. There just may be hope for me yet 😄 . Merging
|
/cherrypick enterprise-4.15 |
|
@JoeAldinger: new pull request created: #71709 DetailsIn response to this:
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. |

OSDOCS-9001
Versions: 4.15+
This PR adds existing vSphere credential parameters for the install-config that are now available for the Agent-based Installer
QE review:
Preview:
Existing docs: