enhancements
enhancements copied to clipboard
kubernetes
KEP-0661: StatefulSet volume resize
- One-line PR description: This enhancement proposes to add the ability to resize volumes associated with StatefulSet via modifications to its specification.
- Issue link: https://github.com/kubernetes/enhancements/issues/661
- Other comments: This is a fork of https://github.com/kubernetes/enhancements/pull/1848 and https://github.com/kubernetes/enhancements/pull/2842 the former has been long abandoned, and with the latter, after talking with the author, we've decided to continue the work in this PR.
Welcome @areller!
It looks like this is your first PR to kubernetes/enhancements š. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.
You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.
You can also check if kubernetes/enhancements has its own contribution guidelines.
You may want to refer to our testing guide if you run into trouble with your tests not passing.
If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!
Thank you, and welcome to Kubernetes. :smiley:
Hi @areller. Thanks for your PR.
I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
I understand the commands that are listed here.
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.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: areller
To complete the pull request process, please assign johnbelamaric, msau42 after the PR has been reviewed.
You can assign the PR to them by writing /assign @johnbelamaric @msau42 in a comment when ready.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@gnufied reading through https://github.com/kubernetes/enhancements/pull/660 and https://github.com/kubernetes/enhancements/pull/2842 I've gathered some of the open questions we'll need to answer (could be more)
- What level of validation should happen on API server vs. during controller reconciliation. Should we try to validate that the storage class supports expansion for example?
Ideally we should probably strive to validate as much as possible during admission of the statefulset update.
To give an example why it's a good idea, a lot of people that use kubernetes, might have some kind of a CI/CD system that deploys their changes. If an error happens, but it only happens on a controller (and reported as an event on the statefulset object), and not during admission, the CI/CD might not report those errors back to the user, making them think that every change had been applied successfully, not realizing that the resize had actually failed.
To allow statefulset validation to access other objects (i.e. a storageclass), I think we'll need to create an admission plugin, or use the one that already exists for validating that the storage class of a PVC supports expansion when resizing it https://github.com/kubernetes/kubernetes/blob/master/plugin/pkg/admission/storage/persistentvolume/resize/admission.go
I'm not sure if it's going to cover all failure scenarios. volume expansion could fail for reasons other than storage class doesn't support expansion. Maybe the user tries to resize to a size that isn't supported by underlying driver for example.
- related to (1) but has a different purpose behind it, should we prevent the user from decreasing the storage size during admission?
According to https://github.com/kubernetes/enhancements/tree/master/keps/sig-storage/1790-recover-resize-failure we allow users to decrease the request size of a PVC under certain circumstances, so I don't think we should prevent users to decrease request size in the PVC template. but maybe we can allow that only after checking the EnableRecoverFromExpansionFailure feature flag.
- Should we have support for dealing with offline expansions?
I personally don't think we should support that in the scope of this proposal. i.e. we should either reject resize during admission if offline expansion is required (assuming it's possible to check if a storage class requires offline expansion during admission. not sure about that), or fail in the statefulset controller.
If we end up supporting offline expansions, either in this KEP or in a follow up, we should do everything to avoid restarting pods, unless online expansion is supported.
- should we have modifications to PVC template be part of revision control?
As mentions in previous PRs, I think it's a good idea to not include PVC template updates in revision control, for reasons
a. If user issues an update to the pod template of the statefulset and a resize to the PVC template at the same time, we want to allow them to rollback the pod template section. including PVC template updates in revision control will make it impossible since the rollback would try to rollback the PVC resize, which we don't want to do. b. including PVC template in revision will make the statefulset pods restart every time PVC template is changed. we don't want to do that, see (3)
- should we perform PVC resizes one by one and wait for the resize to be finished?
I don't think that the statefulset controller should wait for the resize to be finished on the PVC itself The only reason reason we'd want to do that, is if resizing a PVC that's already undergoing a resize, could cause issues. I don't think that's the case though.
Though, if we decide to implement a solution for offline expansion as pat of this proposal, we'd probably need to wait for resize to finish, to know when to restart the pods https://github.com/kubernetes/enhancements/pull/660/files#r254095445
- If a resize fails during statefulset controller reconciliation, should we block all other operations (e.g. restarting a pod incase of an update to the pod template)
As mentioned before, we should make best effort to catch as many errors during admission. If a non transient resize error happens during controller reconciliation, we should put an event indicating failure on the statefulset and not carry on with other operations. I think something like this https://github.com/kubernetes/kubernetes/pull/110522/files#diff-a1e25eb50e4b2aed8947c2caa5d6d1ddfef1a5f2f95fdf71f2388c657823ee13R333-R337
Let me know what you think and what other questions you had And I can incorporate these points in the doc.
Iām going to help review this, is there any additional context not captured in this kep or the previous PR I need to be aware of?
Hi @smarterclayton I think that the README describes it pretty well On top of it, there's https://github.com/kubernetes/enhancements/pull/3412#issuecomment-1160375231 which I was going to discuss with @gnufied but haven't done so yet That comment also answers some of the points made in https://github.com/kubernetes/enhancements/pull/660 Some of the points made in that comment are not in the README of this PR yet, I was going to do it after reaching some consensus over those points, but we can discuss that.
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
what happens when the resize of one PVC fails but the others succeed? maybe due to the last one putting it over quota/capacity on the backend?
Unlike KEP 1790, you couldn't revert the PVC size in the stateful set spec because some already have allocatedResources of the new size, so the failing PVC is back to the original issue of resize failing over and over.
what happens when the resize of one PVC fails but the others succeed? maybe due to the last one putting it over quota/capacity on the backend?
Unlike KEP 1790, you couldn't revert the PVC size in the stateful set spec because some already have allocatedResources of the new size, so the failing PVC is back to the original issue of resize failing over and over.
I was thinking about producing an error event on the statefulset object and let the user handle it. not sure if there's a way to know ahead of time whether all resize operations going to succeed or some will fail
I personally don't think we should support that in the scope of this proposal. i.e. we should either reject resize during admission if offline expansion is required (assuming it's possible to check if a storage class requires offline expansion during admission. not sure about that), or fail in the statefulset controller.
How are we going to determine that the plugin supports online expansion? Esp. during admission.
I personally don't think we should support that in the scope of this proposal. i.e. we should either reject resize during admission if offline expansion is required (assuming it's possible to check if a storage class requires offline expansion during admission. not sure about that), or fail in the statefulset controller.
How are we going to determine that the plugin supports online expansion? Esp. during admission.
The plugin should expose PluginCapability_VolumeExpansion_ONLINE via its rpc API in such case. I'll have to see if it's possible to access that info from within an admission hook
The plugin should expose PluginCapability_VolumeExpansion_ONLINE via its rpc API in such case. I'll have to see if it's possible to access that info from within an admission hook
Unless admission plugin and CSI driver are deployed together, that goal is almost impossible. We could consider adding something to CSIDriver object but that is another set of problems.
Also btw - I did not see anywhere but what happens when a SS is scaled to 0? and user updates the template?
Unless admission plugin and CSI driver are deployed together, that goal is almost impossible.
Got it. Wouldn't we have the same issue with accessing it from the StatefulSet controller then?
We could consider adding something to CSIDriver object but that is another set of problems.
This sounds like a sensible approach to me. what kind of issues do you think it could introduce?
Also btw - I did not see anywhere but what happens when a SS is scaled to 0? and user updates the template?
right, I'll need to add a section on that. In my current approach, if StatefulSet is scaled to 0, changing the volume template would pass but it won't update the underlying PVCs. Same if you have 2 PVCs and StatefulSet is scaled to 1 (it will only update the 1 PVC that's part of the scaled up pod). When you scale the StatefulSet back up, it'll compare the PVCs to the template and attempt to reconcile.
This sounds like a sensible approach to me. what kind of issues do you think it could introduce?
Generally because it is an API change in core k8s objects and hence we have to be strongly justified in making the change, such as exhausting other options etc. I am not opposed to it in theory, but if we are going to do this - we have to update the KEP and document how we intend to use the field.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: areller Once this PR has been reviewed and has the lgtm label, please assign johnbelamaric for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@areller: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-enhancements-verify | 95a7fe96a22b2c3ba467df2be9879439d23c5d3f | link | true | /test pull-enhancements-verify |
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.
@soltysh
I'm a bit concerned about two major issues in this proposal:
1. The perma beta on resize functionality. 2. The proposed changes to statefulset controller, which will make the controller more complicated, and do not provide sufficient guarantees about reliability of the operations. I'd like to see a more strict coordination mechanism which ensures proper user-feedback from the resize operation.
- this is a mistake š
- can you elaborate on this point please? do you think the mechanism described in https://github.com/kubernetes/enhancements/blob/95a7fe96a22b2c3ba467df2be9879439d23c5d3f/keps/sig-storage/661-statefulset-volume-resize/README.md#feedback won't be sufficient as a feedback mechanism?
- this is a mistake sweat_smile
I've asked to link that GA blog post in other comment, so we're on the same page ;)
- can you elaborate on this point please? do you think the mechanism described in https://github.com/kubernetes/enhancements/blob/95a7fe96a22b2c3ba467df2be9879439d23c5d3f/keps/sig-storage/661-statefulset-volume-resize/README.md#feedback won't be sufficient as a feedback mechanism?
I'm asking for a more detailed description of the interactions, such that I can better understand the flow of information. As it currently stands I'm having hard time to properly grasp the scope and guarantees of the flow.
@areller Is this still being worked on? I would like to contribute or take over this KEP if it's stale
We really need this ability and we would like to contribute to this Kep. Can someone please push it forward as soon as possible?
