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

Move recover from volume expansion feature to beta #4849

Merged

Conversation

gnufied
Copy link
Member

@gnufied gnufied commented Sep 12, 2024

xref #1790

We are planning to move this feature to beta. The KEP has PRR review already filled in, but please do review closely, if I missed something.

/assign @jsafrane @deads2k

@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Sep 12, 2024
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 12, 2024
@jsafrane
Copy link
Member

jsafrane commented Sep 13, 2024

There is still this drawback added 3 years ago and still saying "We plan to revisit and address this in next release":

## Drawbacks
- One drawback is while this KEP allows an user to reduce requested PVC size, it does not allow reduction in size all the way to same value as original size recorded in `pvc.Status.Cap`. The reason is currently resize controller and kubelet work on a volume one after the other and we need special logic in both kubelet and control-plane to process reduction in size all the way upto original value. This is somewhat racy and we need a signaling mechanism between control-plane and kubelet when `pvc.Status.AllocatedResources` needs to be adjusted. We plan to revisit and address this in next release.

When do we plan to address it?

@gnufied
Copy link
Member Author

gnufied commented Sep 13, 2024

There is still this drawback added 3 years ago and still saying "We plan to revisit and address this in next release":

I do not mind leaving that issue unresolved. I do not think any of folks who are using this feature has asked this to be fixed. @msau42 what do you think?

@gnufied gnufied force-pushed the change-milestone-recovery-feature branch from 90d712a to 1fd9585 Compare September 30, 2024 20:05
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 30, 2024
@jsafrane
Copy link
Member

jsafrane commented Oct 1, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2024
@jsafrane
Copy link
Member

jsafrane commented Oct 1, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2024
@deads2k
Copy link
Contributor

deads2k commented Oct 3, 2024

approving the PRR

/approve

@jsafrane
Copy link
Member

jsafrane commented Oct 7, 2024

/approve
/hold
for @msau42 if she has an opinion

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, gnufied, jsafrane

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 Oct 7, 2024
@msau42
Copy link
Member

msau42 commented Oct 7, 2024

Regarding being able to resize back to 0, I think it would be nice to have. There are some storage systems that have very large expansion increments of 100s of GBs or even TBs, and if a user doesn't realize that, they may want to find alternative ways to manage their space instead of expanding the size. But since we require the size has to be size+1, they will have to go with a painful alternative of having to recreate the PVC/PV, fighting finalizers, recreating Pods (ie downtime) to get rid of the error.

That being said, being able to support this does add signficant complexity to the design, so if we don't want to support this, then I would at least like to see:

  1. User-facing documentation/playbook on how they can recover back to the original size
  2. Strawman design on how we could support this in the future (ie we won't close the door with the current proposal)

@gnufied
Copy link
Member Author

gnufied commented Oct 8, 2024

@msau42 we have always documented how to recover from any expansion failure including resetting the PVC size completely - https://kubernetes.io/docs/concepts/storage/persistent-volumes/#recovering-from-failure-when-expanding-volumes

Strawman design on how we could support this in the future (ie we won't close the door with the current proposal)

I can make a strawman design, but the devil is in the details and making sure proposed flow - https://github.com/kubernetes/enhancements/blob/master/keps/sig-storage/1790-recover-resize-failure/expansion_flow.pdf has no corner cases. I believe currently we have a proven design which works, has much less chances of race conditions and it does help with valid usability problems, including better observability in case of errors (because errors don't flip-flop anymore).

We won't know about all the corner cases until we do a full design and start implementing it. Volume expansion is complicated because of state reconciliation between kubelet and control-plane and has all kind of edge cases. I am not opposed to letting users reset the PVC size via a new feature may be called ResetResizeRequest or something similar. I believe - that may be the best way forward. FWIW - I am yet to come across a driver which really needs to reset size completely to recover from expansion failure and at this point, I am not sure if the effort is worth the outcome.

@msau42
Copy link
Member

msau42 commented Oct 8, 2024

Thanks, I think the only thing missing from those steps is having to deal with pvc finalizers if a Pod is still referencing it. It would be nice if users can fix this without having to restart all their Pods. We can follow up further documentation enhancements.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3d91e6c into kubernetes:master Oct 8, 2024
4 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 8, 2024
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants