Skip to content
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

Bug 1870082: Use index in MachineSet names for Openstack IPI #4070

Merged

Conversation

yussufsh
Copy link
Contributor

Fixes #4069

Signed-off-by: Yussuf Shaikh yussuf@us.ibm.com

@yussufsh
Copy link
Contributor Author

I have problems accessing my Bugzilla account hence created #4069 with details.

@pierreprinetti
Copy link
Member

pierreprinetti commented Aug 19, 2020

@yussufsh Thank you for the patch!

/retitle Bug 1870082: Always convert AZ to lowercase in MachineSet name
/label platform/openstack
/approve
/hold

@mandre
Do you happen to know if OpenStack availability zones are case-sensitive (and thus could cause collisions in the MachineSet names)?

@openshift-ci-robot openshift-ci-robot added platform/openstack do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 19, 2020
@openshift-ci-robot openshift-ci-robot changed the title Always convert AZ to lowercase for machineset to work Bug 1870082: Always convert AZ to lowercase in MachineSet name Aug 19, 2020
@openshift-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2020
@openshift-ci-robot
Copy link
Contributor

@yussufsh: This pull request references Bugzilla bug 1870082, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1870082: Always convert AZ to lowercase in MachineSet name

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.

@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 19, 2020
@mandre
Copy link
Member

mandre commented Aug 19, 2020

Since the availability zone name ends up in the machine name, we need to ensure that it contains only alphanumeric characters, - or .. We'll need to strip or convert invalid characters in the AZ names to -, or possibly add a validation so that we can fail early when the AZ names include invalid chars.

Also, we'll have to take that into account when validating the cluster name length.

Do you happen to know if OpenStack availability zones are case-sensitive (and thus could cause collisions in the MachineSet names)?

I do not know, but the fact that Yussuf was able to create an AZ with name PowerKVM makes me think it's case sensitive. I'll double check with nova people.

Copy link
Contributor

@Fedosin Fedosin left a comment

Choose a reason for hiding this comment

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

Based on the documentation az names can contain everything except ":". https://docs.openstack.org/nova/latest/admin/availability-zones.html
If so, we need to get rid of special characters too. I created a go playground with an example how I would do it: https://play.golang.org/p/XqlHfz6IsE5

@Fedosin
Copy link
Contributor

Fedosin commented Aug 19, 2020

/retest

@yussufsh
Copy link
Contributor Author

Frankly I am not sure what is the relevance of appending the AZ name to the machineset. If it is just a matter of uniqueness then I think we could use the index (idx) instead of AZ name.

Also, if I am not wrong the filename would be in same format.

Just a suggestion to save us a lot of thinking.

@pierreprinetti
Copy link
Member

If we operate transformations to the AZ name (and if we want to keep composing the MachineSet name out of the AZ name), we might need to add random characters to the MachineSet name to prevent collisions.

@yussufsh
Copy link
Contributor Author

I wonder if this is not a problem with other cloud providers supporting AZ.

@pierreprinetti
Copy link
Member

I wonder if this is not a problem with other cloud providers supporting AZ.

It appears that AWS and GCP have URL-compatible zone names, and they compose the MachineSet name out of those. Sensible variability in AZ names is probably a problem of on-premise clouds.

Frankly I am not sure what is the relevance of appending the AZ name to the machineset. If it is just a matter of uniqueness then I think we could use the index (idx) instead of AZ name.

I like this. @mandre @Fedosin what do you think?

@pierreprinetti
Copy link
Member

@yussufsh
We discussed this; having the index in the name instead of the AZ name sounds like a plan.

Do you want to amend the PR and implement your idea?

@yussufsh
Copy link
Contributor Author

@yussufsh
We discussed this; having the index in the name instead of the AZ name sounds like a plan.

Do you want to amend the PR and implement your idea?

Sure about that.

Fixes openshift#4069

Signed-off-by: Yussuf Shaikh <yussuf@us.ibm.com>
@yussufsh yussufsh changed the title Bug 1870082: Always convert AZ to lowercase in MachineSet name Bug 1870082: Use index in MachineSet names for Openstack IPI Aug 21, 2020
@mandre
Copy link
Member

mandre commented Aug 21, 2020

/test e2e-openstack

@pierreprinetti
Copy link
Member

/retest

@yussufsh
Copy link
Contributor Author

/test e2e-openstack

@pierreprinetti
Copy link
Member

/retest

1 similar comment
@pierreprinetti
Copy link
Member

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@yussufsh
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

16 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 26, 2020

@yussufsh: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-crc ec4dfc7 link /test e2e-crc

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.

@openshift-merge-robot openshift-merge-robot merged commit c4e0dce into openshift:master Aug 26, 2020
@openshift-ci-robot
Copy link
Contributor

@yussufsh: All pull requests linked via external trackers have merged:

Bugzilla bug 1870082 has been moved to the MODIFIED state.

In response to this:

Bug 1870082: Use index in MachineSet names for Openstack IPI

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
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. platform/openstack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenStack IPI install fails during bootstrap due to availability zone name
7 participants