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

Prefix etcd cluster names with letters #10361

Merged
merged 2 commits into from
Jan 5, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented Dec 3, 2020

Refs: #9982

/cc @rifelpet

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 3, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 3, 2020
@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

this looks good to me though it doesn't fix existing clusters being upgraded to Terraform 0.12.

@hakman
Copy link
Member Author

hakman commented Dec 4, 2020

Not sure if we can have more than a release note for those running Terraform 0.12. At least with this new clusters could be created.

@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

what would the release note say? It seems like we should either use a different terraform resource name if the original name starts with a number and have the release notes instruct users to terraform state mv, or walk users through changing the etcd cluster member names which isn't clearly understood or documented, and likely involves control plane downtime.

Users will experience this issue when upgrading existing clusters, so we should be prepared to handle that.

@hakman
Copy link
Member Author

hakman commented Dec 4, 2020

I think this is a start. If someone bumps into this issue with an existing cluster, will be a pain, but I don't see any easy solution. It's not just about renaming resources. If the community will find some way to fix things manually and documents the steps, I would be happy to look at that.

@rifelpet
Copy link
Member

rifelpet commented Dec 4, 2020

i think the easiest solution is to have kops change the terraform resource name that it generates when it would otherwise start with a number in this situation. Then we know that anyone that had their resource name change would have otherwise ran into this issue and they'll need to terraform state mv the resource to its new name. The fix only impacts users that are directly affected by the issue, and the fix is not disruptive to the cluster itself.

@hakman
Copy link
Member Author

hakman commented Dec 4, 2020

That would require more work and testing.

@justinsb
Copy link
Member

justinsb commented Dec 4, 2020

I find it hard to reason about whether this is safe. Can we first create a test case that hits the numbers problem (in the create_cluster case), and then rebase this on that so we can see the change?

@hakman
Copy link
Member Author

hakman commented Dec 4, 2020

@justinsb I added the test as part of #10365.

@rifelpet
Copy link
Member

rifelpet commented Jan 5, 2021

/lgtm

we will merge this plus #10424 and an env var check as discussed in the referenced GH issue

@hakman
Copy link
Member Author

hakman commented Jan 5, 2021

/retest

k8s-ci-robot added a commit that referenced this pull request Jan 5, 2021
…10361-upstream-release-1.19

Automated cherry pick of #10365: Add integration test for creating an HA cluster in shared #10361: Prefix etcd cluster names with letters
@k8s-ci-robot k8s-ci-robot merged commit 2e202ba into kubernetes:master Jan 5, 2021
@k8s-ci-robot k8s-ci-robot removed this from the v1.19 milestone Jan 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Jan 5, 2021
k8s-ci-robot added a commit that referenced this pull request Jan 5, 2021
Manual cherry pick of #10361: Prefix etcd cluster names with letters
@hakman hakman deleted the single-az-multi-master branch January 10, 2021 05:31
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. blocks-next cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants