Skip to content

Conversation

skopacz1
Copy link
Contributor

@skopacz1 skopacz1 commented Jan 18, 2024

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:

  • QE has approved this change.

Preview:

Existing docs:

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 18, 2024

@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:

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:

  • QE has approved this change.

Preview:

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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 18, 2024
@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 18, 2024
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Jan 18, 2024

🤖 Thu Feb 15 17:16:45 - Prow CI generated the docs preview: https://70551--ocpdocs-pr.netlify.app

@skopacz1 skopacz1 changed the title OSDOCS-9001: Adding vSphere credentials to Agent OSDOCS#9001: Adding vSphere credentials to Agent Jan 19, 2024
@openshift-ci-robot
Copy link

@skopacz1: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

In response to this:

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:

  • QE has approved this change.

Preview:

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.

@openshift-ci-robot openshift-ci-robot removed the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 19, 2024
@openshift-ci openshift-ci bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 19, 2024
Copy link
Contributor Author

@skopacz1 skopacz1 Jan 19, 2024

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).

@rwsu
Copy link

rwsu commented Feb 6, 2024

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.

Screenshot 2024-02-06 at 2 27 17 PM
Screenshot from this page: https://docs.openshift.com/container-platform/4.14/installing/installing_vsphere/installing-vsphere.html#installation-vsphere-config-yaml_installing-vsphere

@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 7, 2024
|platform:
vsphere:
vcenters:
password:
Copy link
Contributor

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).

Suggested change
password:
- password: <pw>
port: 443
server: <vcenter fqdn>

Copy link
Contributor Author

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"

Comment on lines 2661 to 2734
|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

Copy link
Contributor

Choose a reason for hiding this comment

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

rwsu

This comment was marked as duplicate.

Copy link

@rwsu rwsu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024
@skopacz1
Copy link
Contributor Author

skopacz1 commented Feb 9, 2024

@mhanss could you PTAL when you have a chance?

vsphere:
failureDomains:
name:
|The name of the vCenter server.
Copy link

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'.

Comment on lines 2716 to 2717
|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`.
Copy link

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.

Copy link
Contributor Author

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.

Comment on lines 2725 to 2729
|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.
Copy link

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.

Copy link
Contributor Author

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.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2024
Copy link

openshift-ci bot commented Feb 12, 2024

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>`.
Copy link

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.

Suggested change
|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>`.

Comment on lines 2726 to 2729
|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.
Copy link

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor

@michaelryanpeter michaelryanpeter 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 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.
Copy link
Contributor

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:

Suggested change
| 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>`.
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
|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.
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
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.
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
|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.

Copy link
Contributor

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"?

Copy link
Contributor Author

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.

@openshift-ci openshift-ci bot 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 peer-review-in-progress Signifies that the peer review team is reviewing this PR labels Feb 14, 2024
Copy link

openshift-ci bot commented Feb 15, 2024

@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.

@skopacz1
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Feb 15, 2024
@JoeAldinger JoeAldinger added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Feb 15, 2024
@JoeAldinger JoeAldinger self-assigned this Feb 15, 2024
Copy link
Contributor

@JoeAldinger JoeAldinger left a 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[]
Copy link
Contributor

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?

Copy link
Contributor Author

@skopacz1 skopacz1 Feb 15, 2024

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

Copy link
Contributor

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

@JoeAldinger JoeAldinger removed their assignment Feb 15, 2024
@JoeAldinger JoeAldinger merged commit 48c1798 into openshift:main Feb 15, 2024
@JoeAldinger
Copy link
Contributor

/cherrypick enterprise-4.15

@openshift-cherrypick-robot

@JoeAldinger: new pull request created: #71709

In response to this:

/cherrypick enterprise-4.15

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.15 merge-review-in-progress Signifies that the merge review team is reviewing this PR peer-review-done Signifies that the peer review team has reviewed this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants