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

Default Storage Classes can't easily be disabled #39561

Closed
saad-ali opened this issue Jan 7, 2017 · 9 comments · Fixed by #40108
Closed

Default Storage Classes can't easily be disabled #39561

saad-ali opened this issue Jan 7, 2017 · 9 comments · Fixed by #40108
Assignees
Labels
sig/storage Categorizes an issue or PR as relevant to SIG Storage.
Milestone

Comments

@saad-ali
Copy link
Member

saad-ali commented Jan 7, 2017

PR https://github.com/kubernetes/kubernetes/pull/31617/files introduced default storage classes for 1.6. They are created with cluster addon which can not easily be modified by end users (requires access to the master).

We should improve experience before 1.6 goes out.

CC @kubernetes/sig-storage-misc

@saad-ali saad-ali added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 7, 2017
@saad-ali saad-ali added this to the v1.6 milestone Jan 7, 2017
@thockin
Copy link
Member

thockin commented Jan 7, 2017 via email

@saad-ali
Copy link
Member Author

@msau42 can help out with this

@thockin
Copy link
Member

thockin commented Jan 12, 2017 via email

@MrHohn
Copy link
Member

MrHohn commented Jan 12, 2017

As @mikedanese mentioned before, it wouldn't be hard to implement an "oneshot" functionality. I also had a short proposal for this.

If we agree this is the proper way to go with, I can also help out. The required work I expect would be putting couple more startup logics into kube-addon.sh that carefully handle upgrade/restart cases.

@thockin
Copy link
Member

thockin commented Jan 13, 2017 via email

@MrHohn
Copy link
Member

MrHohn commented Jan 13, 2017

Ensure exist sounds good. What about two classes: one that reconcile, the other ensure exist?

@msau42
Copy link
Member

msau42 commented Jan 13, 2017

That sounds acceptable. We mainly need:

  1. A way to be able to change the default-class annotation
  2. That change should persist across upgrades

I think it is ok if the deletion case is not supported. Changing the default-class is sufficient to get the behavior we want.

@jsafrane
Copy link
Member

@msau42, can I help you in any way? I know very little about addon manager and its internals.

@msau42
Copy link
Member

msau42 commented Jan 24, 2017

Thanks, I plan to coordinate with addon folks to make the change. It may end up being that all the changes are only in the addon component.

k8s-github-robot pushed a commit that referenced this issue Feb 25, 2017
Automatic merge from submit-queue

Supports 'ensure exist' class addon in Addon-manager

Fixes #39561, fixes #37047 and fixes #36411. Depends on #40057.

This PR splits cluster addons into two categories:
- Reconcile: Addons that need to be reconciled (`kube-dns` for instance).
- EnsureExists: Addons that need to be exist but changeable (`default-storage-class`).

The behavior for the 'EnsureExists' class addon would be:
- Create it if not exist.
- Users could do any modification they want, addon-manager will not reconcile it.
- If it is deleted, addon-manager will recreate it with the given template.
- It will not be updated/clobbered during upgrade.

As Brian pointed out in [#37048/comment](#37048 (comment)), this may not be the best solution for addon-manager. Though #39561 needs to be fixed in 1.6 and we might not have enough bandwidth to do a big surgery.

@mikedanese @thockin 

cc @kubernetes/sig-cluster-lifecycle-misc 

---

Tasks for this PR:
- [x] Supports 'ensure exist' class addon and switch to use new labels in addon-manager.
- [x] Updates READMEs regarding the new behavior of addon-manager.
- [x] Updated `test/e2e/addon_update.go` to match the new behavior.
- [x] Go through all current addons and apply the new labels on them regarding what they need.
- [x] Bump addon-manager and update its template files.
dims pushed a commit to dims/kubernetes that referenced this issue Feb 8, 2018
Automatic merge from submit-queue

Supports 'ensure exist' class addon in Addon-manager

Fixes kubernetes#39561, fixes kubernetes#37047 and fixes kubernetes#36411. Depends on kubernetes#40057.

This PR splits cluster addons into two categories:
- Reconcile: Addons that need to be reconciled (`kube-dns` for instance).
- EnsureExists: Addons that need to be exist but changeable (`default-storage-class`).

The behavior for the 'EnsureExists' class addon would be:
- Create it if not exist.
- Users could do any modification they want, addon-manager will not reconcile it.
- If it is deleted, addon-manager will recreate it with the given template.
- It will not be updated/clobbered during upgrade.

As Brian pointed out in [kubernetes#37048/comment](kubernetes#37048 (comment)), this may not be the best solution for addon-manager. Though kubernetes#39561 needs to be fixed in 1.6 and we might not have enough bandwidth to do a big surgery.

@mikedanese @thockin 

cc @kubernetes/sig-cluster-lifecycle-misc 

---

Tasks for this PR:
- [x] Supports 'ensure exist' class addon and switch to use new labels in addon-manager.
- [x] Updates READMEs regarding the new behavior of addon-manager.
- [x] Updated `test/e2e/addon_update.go` to match the new behavior.
- [x] Go through all current addons and apply the new labels on them regarding what they need.
- [x] Bump addon-manager and update its template files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/storage Categorizes an issue or PR as relevant to SIG Storage.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants