AGENT-718: Add vSphere credentials to install-config overrides #7593
Conversation
Removed the warnings for vSphere credentials. Added validations to check * all required credential fields have been filled in if any are provided * if failureDomains are defined there is a corresponding vcenter with the same server defined Added new warnings for new fields LoadBalancer and Hosts.
Agent-based installation allows credentials to be optional. Platform values are not validated if credentials are not provided. Folder is now a mandatory field for agent-based installations. It is optional for IPI and UPI. It is being made mandatory because if credentials are entered and folder is left unspecified, assisted-service will substitute in placeholder values for folder. The placeholder value is "/datacenterplaceholder/vm/folderplaceholder". When assisted-service generates the install-config, validation will fail because "datacenterplaceholder" will not match the actual datacenter that the user entered. Better to make it mandatory and validate folder is entered when the agent ISO is being created. Otherwise the user will not realize there is an issue until they boot up the agent ISO. The issue will not be transparent and only visible if the user examines the assisted-service container log.
openshift-ci-robot
added
the
jira/valid-reference
|
@rwsu: This pull request references AGENT-718 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 task to target the "4.15.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 kubernetes/test-infra repository. |
pkg/asset/agent/installconfig.go
Outdated
| } | ||
|
|
||
| for _, failureDomain := range vspherePlatform.FailureDomains { | ||
| // Although folder is optional in IPI/UPI, it is must be set for agent-based installs. |
There was a problem hiding this comment.
nit: it must be set
pkg/asset/agent/installconfig.go
Outdated
| // If it is not set, assisted-service will set a placeholder value for folder: | ||
| // "/datacenterplaceholder/vm/folderplaceholder" | ||
| // | ||
| // When assisted-service generates the install-config to for the cluster, it will fail |
There was a problem hiding this comment.
nit: install-config for the cluster
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pawanpinjarkar The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| // agent-based installation allows the credentials to be optional | ||
| if len(p.VCenters) > 0 { | ||
| if p.VCenters[0].Username != "" && p.VCenters[0].Password != "" && | ||
| p.VCenters[0].Server != "" && len(p.VCenters[0].Datacenters) != 0 { |
There was a problem hiding this comment.
Maybe I'm missing something, but shouldn't there be an OR between these instead of AND? Otherwise it won't catch if one of them is missing and there's no point calling validateVCenters() which checks each value. It actually seems like this whole check should be removed.
There was a problem hiding this comment.
Yes, it is a bit confusing here. Let me reword it and change the logic a bit to make it more clear. The goal was to allow credentials to be optional and to not validate if the credentials are not provided. I think this actually belongs in AGENT-717. There is overlapping logic happening here with asset/agent/installconfig.go.
Updated comments.
If vSphere credentials are present in install-config.yaml, they are passed through as a install-config override annotation in AgentClusterInstall. If vSphere credentials are not present, then nothing is added to the install-config override.
|
/lgtm |
|
@rwsu: The following tests failed, say
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. |
If vSphere credentials are present in install-config.yaml, they are passed through as a install-config override annotation in AgentClusterInstall.
If vSphere credentials are not present, then nothing is added to the install-config override.
Requires: #7572