-
Notifications
You must be signed in to change notification settings - Fork 1.8k
OSDOCS#9001: Adding vSphere credentials to Agent #70551
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
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. In 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. In 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manoj is correct. folder is not optional and is a required parameter. I think line 2729 should be commented out.
e323ea3
to
4e860cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Configures the connection details so that services can communicate with vCenter"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 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. |
/label merge-review-needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One quick question just to make sure things are good.
|
||
ifdef::vsphere[] | ||
|
||
ifdef::agent,vsphere[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 In 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: