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: Explicitly set default StorageClass to support upgrades #9337

Merged

Conversation

joshbranham
Copy link
Contributor

Dropping the annotation versus setting it to false breaks cluster upgrades.

Set the old gp2 StorageClass explicitly not the default

Fixes #9336

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 11, 2020
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 11, 2020
@joshbranham
Copy link
Contributor Author

/test pull-kops-bazel-test

@johngmyers
Copy link
Member

/kind bug
/lgtm

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 11, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 11, 2020
@hakman
Copy link
Member

hakman commented Jun 12, 2020

Any thoughts on using storageclass.kubernetes.io/is-default-class instead of the beta version?

@joshbranham
Copy link
Contributor Author

Does 1.17 honor that annotation as well?

@hakman
Copy link
Member

hakman commented Jun 12, 2020

The beta one seems deprecated since 1.6:
kubernetes/kubernetes#40088

@joshbranham
Copy link
Contributor Author

The beta one seems deprecated since 1.6:
kubernetes/kubernetes#40088

Yeah we can change the kops-ssd-1-17 to use the non-beta label but we will need to use the beta label (and maybe if we want the non-beta) on the gp2 in order to have the desired behavior.

Otherwise, we go back to the original point of how to make upgrades work on immutable fields, where if we just made gp2 have these updated settings alongside a breaking change of you have to run this with --force to upgrade 🤷

@johngmyers
Copy link
Member

Since we've released 1.17.0, I don't think we can change the name of the annotation. Otherwise clusters updated with 1.17.0 will encounter this bug when we need to make kops-ssd-1-17 non-default.

@joshbranham
Copy link
Contributor Author

So I think the only other thing would be to maybe just add some wording to release notes in case someone runs into a conflict on upgrade, its easy enough and zero risk to fix themselves. We can even provide a few one-liners. Thoughts?

@olemarkus
Copy link
Member

+1 from me!

@hakman
Copy link
Member

hakman commented Jun 13, 2020

+1 from me also

@k8s-ci-robot k8s-ci-robot added area/documentation and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 13, 2020
@johngmyers
Copy link
Member

Ah, that release note commit was added; not a replacement for the first one.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2020
@rifelpet
Copy link
Member

I agree that this sounds good to me. We can get this merged now to get it under e2e test coverage, and open a cherry-pick to release-1.17.
I presume we wont have a 1.17.1 release before office hours on friday, so we can take any final feedback there.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joshbranham, rifelpet

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2020
@k8s-ci-robot k8s-ci-robot merged commit d6f03b3 into kubernetes:master Jun 14, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 14, 2020
@joshbranham joshbranham deleted the bug/fix-default-storageclass branch June 15, 2020 01:04
k8s-ci-robot added a commit that referenced this pull request Jun 15, 2020
…37-upstream-release-1.18

Automated cherry pick of #9337: Explicitly set default storageclass to support upgrades
k8s-ci-robot added a commit that referenced this pull request Jun 17, 2020
…37-upstream-release-1.17

Automated cherry pick of #9337: Explicitly set default storageclass to support upgrades
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. area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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.

New Kops 1.17 AWS StorageClass Wreaks Havoc with Existing Defaults and Provides Uncertain Upgrade Path
6 participants