[OpenStack][UPI]: tweaks #2823
[OpenStack][UPI]: tweaks #2823
Conversation
iamemilio
commented
Dec 16, 2019
•
iamemilio
commented
- get cluster-name from infraID generated by the installer from the metadata.json now
- Make the naming scheme uniform with how we name resources in IPI
openshift-ci-robot
added
the
size/XS
|
/assign @tomassedovic |
upi/openstack/inventory.yaml
Outdated
| @@ -6,7 +6,7 @@ all: | |||
|
|
|||
| # User-provided values | |||
| os_subnet_range: '10.0.0.0/16' | |||
| os_cluster_name: 'openshift' | |||
| os_cluster_name: "{{ lookup('env', 'INFRA_ID') }}" | |||
There was a problem hiding this comment.
The idea here is that the user provides values in this section. What is the use case for having some variables in env?
There was a problem hiding this comment.
I just figured it would be easy, since we already tell the user to source this value. But I'll tell you what, I can change it to {{ lookup('env', 'INFRA_ID') | default('openshift') }}
There was a problem hiding this comment.
The difference is, that the installer will always define this value as <cluster-name>-<random hash>, so this at least makes sure that the name on your configs matches the name on your infra. It also depends on the workflow you want here. If we are going install-config --> inventory, as the docs currently lay it out, it makes sense this way. However, if we go
inventory --> install-config then it makes sense to have a user defined cluster name here, and then read in the INFRA_ID elsewhere. Ultimately though, somewhere in the inventory, we will have to read this value back from the installer metadata.
There was a problem hiding this comment.
FWIW I'd prefer if the inventory/playbooks read it from install-config. Less error-prone that way.
Also I believe, there's a difference between cluster name and infra ID. Cluster name is the name you specify in install-config.yaml / installer prompt (e.g. lolcloud) but infra-id is cluster name plus the random suffix (e.g. lolcloud-23n9).
So I think the simplest workflow we can do would be to have os_cluster_name as a user-defined variable in the inventory and read os_infra_id from install-config (or rather, metadata.json).
upi/openstack/inventory.yaml
Outdated
| @@ -26,7 +26,7 @@ all: | |||
| # OpenShift cluster resources. It is built in the form of: | |||
| # | |||
| # "{{ os_cluster_name }}{{ installation-specific string }}" | |||
| os_infra_id: "{{ os_cluster_name }}-upi" | |||
| os_infra_id: "{{ os_cluster_name }}" | |||
There was a problem hiding this comment.
oh yes, thank you for this. That upi was meant to be a placeholder for the random string generated by the installer; however, it makes little sense to change a value the user provided explicitly. 👍
Do you mind amending the comment above accordingly?
There was a problem hiding this comment.
I think os_infra_id must be unique and we shouldn't allow users to specify it at all. If there are two clusters with the same infra ID, then there may be resource collisions. Also as you remember the installer destroys clusters based on their os_infra_id values, so we can accidentally destroy another cluster with the same infra ID.
openshift-ci-robot
added
size/M
iamemilio
force-pushed
the
upi-improvements
branch
from
41c5ac1
to
25b92c2
Compare
openshift-ci-robot
added
size/L
iamemilio
force-pushed
the
upi-improvements
branch
from
25b92c2
to
6faf47c
Compare
|
/label platform/openstack |
| @@ -22,7 +25,6 @@ | |||
| allowed_address_pairs: | |||
| - ip_address: "{{ os_subnet_range | next_nth_usable(5) }}" | |||
| - ip_address: "{{ os_subnet_range | next_nth_usable(6) }}" | |||
| - ip_address: "{{ os_subnet_range | next_nth_usable(7) }}" | |||
There was a problem hiding this comment.
Thanks for the heads up, this has been extracted to #2848
|
/approve Good work, and absolutely needed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pierreprinetti 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-robot
added
the
approved
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@iamemilio: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
