-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
KEP 284: Add PRR for volume expansion feature #3195
Changes from 2 commits
e1231c1
cace6ea
a7bd0e5
e8fdef1
e74df66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
kep-number: 284 | ||
alpha: | ||
approver: "@deads2k" | ||
beta: | ||
approver: "@deads2k" | ||
stable: | ||
approver: "@deads2k" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,17 @@ | |
- [PVC API Change](#pvc-api-change) | ||
- [StorageClass API change](#storageclass-api-change) | ||
- [Other API changes](#other-api-changes) | ||
- [Production Readiness Review Questionnaire](#production-readiness-review-questionnaire) | ||
- [Feature Enablement and Rollback](#feature-enablement-and-rollback) | ||
- [Rollout, Upgrade and Rollback Planning](#rollout-upgrade-and-rollback-planning) | ||
- [Monitoring Requirements](#monitoring-requirements) | ||
- [Dependencies](#dependencies) | ||
- [Scalability](#scalability) | ||
- [Troubleshooting](#troubleshooting) | ||
- [Implementation History](#implementation-history) | ||
- [Drawbacks](#drawbacks) | ||
- [Alternatives](#alternatives) | ||
- [Infrastructure Needed (Optional)](#infrastructure-needed-optional) | ||
<!-- /toc --> | ||
|
||
## Release Signoff Checklist | ||
|
@@ -344,3 +355,229 @@ type StorageClass struct { | |
|
||
This proposal relies on ability to update PVC status from kubelet. While updating PVC's status | ||
a PATCH request must be made from kubelet to update the status. | ||
|
||
## Production Readiness Review Questionnaire | ||
|
||
### Feature Enablement and Rollback | ||
|
||
###### How can this feature be enabled / disabled in a live cluster? | ||
|
||
Volume expansion as a feature has been in beta for too long and as a result has gathered | ||
different feature gates that control various aspects of expansion. | ||
|
||
- [x] Feature gate (also fill in values in `kep.yaml`) | ||
- Feature gate name: ExpandPersistentVolumes | ||
- description: Parent feature gate that is required for expansion in general to work. | ||
- Components depending on the feature gate: | ||
- kube-apiserver | ||
- kubelet | ||
- kube-controller-manager | ||
- Feature gate name: ExpandInUsePersistentVolumes | ||
- description: Enables online expansion. Requires ExpandPersistentVolumes feature gate. | ||
- Components depending on the feature gate: | ||
- kube-apiserver | ||
- kubelet | ||
- kube-controller-manager | ||
- Feature gate name: ExpandCSIVolumes | ||
- description: Enables CSI expansion. | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Components depending on the feature gate: | ||
- kube-apiserver | ||
- kubelet | ||
- kube-controller-manager | ||
- [ ] Other | ||
- Describe the mechanism: | ||
- Will enabling / disabling the feature require downtime of the control | ||
plane? | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Will enabling / disabling the feature require downtime or reprovisioning | ||
of a node? (Do not assume `Dynamic Kubelet Config` feature is enabled). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you answer this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
|
||
###### Does enabling the feature change any default behavior? | ||
|
||
Enabling the feature gate allows users to increase size of pvc by editing `pvc.Spec.Resources` which results | ||
in Kubernetes trying to full-fill the request by actually expanding the volume in controller and then performing | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
file system or any other kind of expansion needed on the node. | ||
|
||
###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)? | ||
|
||
Yes - it can be disabled. It just means users can no longer expand their PVCs. | ||
|
||
###### What happens if we reenable the feature if it was previously rolled back? | ||
|
||
It should be safe to do that. It will just re-enable the feature. | ||
|
||
###### Are there any tests for feature enablement/disablement? | ||
|
||
There aren't any e2e but there are unit tests that cover this behaviour. | ||
|
||
### Rollout, Upgrade and Rollback Planning | ||
|
||
###### How can a rollout or rollback fail? Can it impact already running workloads? | ||
|
||
The feature gate should not impact existing workloads but since we try to expand the | ||
file system(or perform node-expansion) during volume mount and if expansion fails with | ||
some kind of terminal error then it may prevent mount operation from succeeding. | ||
|
||
###### What specific metrics should inform a rollback? | ||
|
||
The `volume_mount` operation failure metric - `storage_operation_errors_total{operation_name=volume_mount}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about controller and CSI failures? Do we have failure metrics that's more fine-grained to just the expand operation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. storage_operation_errors_total{operation_name=expand_volume} and storage_operation_errors_total{operation_name=volume_fs_resize}? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently we do not any specific metrics from external-resizer that are more fine grained. We will have to add those I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some text about adding new metric from external-resizer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come kubernetes-csi/external-resizer#67 does not suffice? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, I did originally document There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you also document those metrics in this section? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
should tell us if expansion operation during volume mount is causing mount failures. | ||
|
||
###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? | ||
|
||
There are no e2e for upgrade-downgrade-upgrade tests for this specific feature but since volume expansion has been | ||
in beta since 1.11, we have tested the feature manually. | ||
|
||
###### Is the rollout accompanied by any deprecations and/or removals of features, APIs, fields of API types, flags, etc.? | ||
|
||
This feature does not deprecate any existing features. | ||
|
||
### Monitoring Requirements | ||
|
||
###### How can an operator determine if the feature is in use by workloads? | ||
|
||
A PVC that is being expanded should have `pvc.Status.Conditions` set. | ||
|
||
###### How can someone using this feature know that it is working for their instance? | ||
|
||
- [x] Events | ||
- Event Reason: External resizer is resizing volume pvc-a71483ed-a5bc-48fa-9151-ca41e7e6634e | ||
- [x API .status | ||
- Condition name: "Resizing" or "FileSystemResizePending" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a condition that specifies if a resize finished successfully? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All resizing related conditions are removed when resizing is finished, but there is no specific condition, to indicate successful completion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The conditions here seem to indicate a resizing operation has started but not completed yet. Is there another event that indicates resize has completed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes there is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably remove this condition because it doesn't indicate if it was successful or not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which specific condition or did you mean event? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant remove the "Pending" conditions, because they cannot determine if resize was successful or not. Or are you saying they can at least determine that controlle expand was successful? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove the Pending conditions, but since it is part of beta API - I thought we have to go through some sort of deprecation phases. Or can we remove it right away in this release? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other thing is - documentation. We have documented There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed for successful completion condition. |
||
- Other field: | ||
- [x] Other (treat as last resort) | ||
- Details: `pvc.Status.Capacity` should reflect user requested size after expansion is complete. | ||
|
||
###### What are the reasonable SLOs (Service Level Objectives) for the enhancement? | ||
|
||
Enabling this feature should not negatively impact volume mount timings in general cases and hence percentile determined by `storage_operation_duration_seconds{operation_name=volume_mount}` metric should not change. | ||
|
||
Having said that if file system requires expansion during mount then it is obviously going to take longer for mount operation to finish. | ||
|
||
###### What are the SLIs (Service Level Indicators) an operator can use to determine the health of the service? | ||
|
||
- [ ] Metrics | ||
msau42 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- controller expansion operation duration: | ||
- Metric name: storage_operation_duration_seconds{operation_name=expand_volume} | ||
- [Optional] Aggregation method: percentile | ||
- Components exposing the metric: kube-controller-manager | ||
- controller expansion operation errors: | ||
- Metric name: storage_operation_errors_total{operation_name=expand_volume} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. storage_operation_duration_seconds now also includes return status: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/util/metrics.go#L49 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh thanks. updated this description. |
||
- [Optional] Aggregation method: cumulative counter | ||
- Components exposing the metric: kube-controller-manager | ||
- node expansion operation duration: | ||
- Metric name: storage_operation_duration_seconds{operation_name=volume_fs_resize} | ||
- [Optional] Aggregation method: percentile | ||
- Components exposing the metric: kubelet | ||
- node expansion operation errors: | ||
- Metric name: storage_operation_errors_total{operation_name=volume_fs_resize} | ||
- [Optional] Aggregation method: cumulative counter | ||
- Components exposing the metric: kubelet | ||
- CSI operation metrics: | ||
- Metric name: csi_sidecar_operations_seconds | ||
- [Optional] Aggregation method: percentile | ||
- Components exposing the metric: external-resizer | ||
|
||
- [ ] Other (treat as last resort) | ||
- Details: | ||
|
||
###### Are there any missing metrics that would be useful to have to improve observability of this feature? | ||
|
||
### Dependencies | ||
|
||
<!-- | ||
This section must be completed when targeting beta to a release. | ||
--> | ||
|
||
###### Does this feature depend on any specific services running in the cluster? | ||
|
||
This feature requires external-resizer running in the cluster for CSI volume expansion. | ||
|
||
### Scalability | ||
|
||
###### Will enabling / using this feature result in any new API calls? | ||
|
||
Yes enabling this feature requires new API calls. | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- Updates to PVs | ||
- API operations | ||
- PATCH PV | ||
- GET PV | ||
- List PVs | ||
- originating components: kubelet, kube-controller-manager, external-resizer | ||
- resync duration: 10mins | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Update to PVCs: | ||
- API operations | ||
- PATCH PVC | ||
- GET PVC | ||
- List PVC | ||
- originating components: kubelet, kube-controller-manager, external-resizer | ||
- resync duration: 10mins | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
###### Will enabling / using this feature result in introducing new API types? | ||
|
||
No | ||
|
||
###### Will enabling / using this feature result in any new calls to the cloud provider? | ||
|
||
Yes, we expect new calls to modify existing volume objects. | ||
|
||
###### Will enabling / using this feature result in increasing size or count of the existing API objects? | ||
|
||
Describe them, providing: | ||
- API type(s): PVC | ||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- Estimated increase in size: A PVC with conditions could have its size increased by anywhere between 100 to 250B. | ||
- Estimated amount of new objects: (e.g., new Object X for every existing Pod) | ||
|
||
|
||
###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs? | ||
|
||
If expansion happens because of pending file system during mount, then it would increase mount time of volume. | ||
|
||
###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components? | ||
|
||
Enabling this feature should not result in resource usage by significant margin, but since we are talking about new controller and an external resize controller for CSI, the resource usage is not negligible either. Having said that - this feature has been in beta since 1.11 and enabled by default(and used in production) - we do not expect resource usage to be a problem. | ||
|
||
### Troubleshooting | ||
|
||
###### How does this feature react if the API server and/or etcd is unavailable? | ||
|
||
gnufied marked this conversation as resolved.
Show resolved
Hide resolved
|
||
###### What are other known failure modes? | ||
|
||
- Expansion can be permanently stuck: | ||
- Detection: Check conditions on `pvc.status` | ||
- Mitigations: If expansion is stuck permanently because of issues in backend and can not be recovered then, it requires manual intervention. Steps to recover from expansion failures are documented in - https://kubernetes.io/docs/concepts/storage/persistent-volumes/#recovering-from-failure-when-expanding-volumes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the recover from resize feature eliminate these manual steps? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that in some cases, if no recovery is possible - say when volume was expanded in controller but failing on the node, then recover from resize failure feature will not help. So admins may still have to take some action if that happens. |
||
- Diagnostics: Conditions on `pvc.Status` and events on PVC should clearly indicate that expansion is failing. | ||
- Testing: There are some unit tests for failure mode but no e2e. | ||
|
||
|
||
###### What steps should be taken if SLOs are not being met to determine the problem? | ||
|
||
If expansion is affecting pod startup time or causing other issues. It can be disabled by editing storageclass and setting `allowVolumeExpansion` to `false`. | ||
|
||
## Implementation History | ||
|
||
- 1.8: Alpha | ||
- 1.11: Beta | ||
- 1.24 GA | ||
|
||
## Drawbacks | ||
|
||
<!-- | ||
Why should this KEP _not_ be implemented? | ||
--> | ||
|
||
## Alternatives | ||
|
||
<!-- | ||
What other approaches did you consider, and why did you rule them out? These do | ||
not need to be as detailed as the proposal, but should include enough | ||
information to express the idea and why it was not acceptable. | ||
--> | ||
|
||
## Infrastructure Needed (Optional) | ||
|
||
<!-- | ||
Use this section if you need things from the project/SIG. Examples include a | ||
new subproject, repos requested, or GitHub details. Listing these here allows a | ||
SIG to get the process for these resources started right away. | ||
--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we have PRR for alpha and beta? If not, I think we can remove those entries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed.