Add pdf and svg document for explaining the flow

Author: gnufied

keps/sig-storage/1790-recover-resize-failure/README.md

@@ -15,7 +15,7 @@
       - [Case 1 (controller+node expandable):](#case-1-controllernode-expandable)
       - [Case 2 (node only expandable volume):](#case-2-node-only-expandable-volume)
       - [Case 3 (Malicious user)](#case-3-malicious-user)
-    - [Case 4(Malicious User and rounding to GB/GiB bounaries)](#case-4malicious-user-and-rounding-to-gbgib-bounaries)
+      - [Case 4(Malicious User and rounding to GB/GiB bounaries)](#case-4malicious-user-and-rounding-to-gbgib-bounaries)
   - [Risks and Mitigations](#risks-and-mitigations)
 - [Graduation Criteria](#graduation-criteria)
   - [Test Plan](#test-plan)
@@ -96,7 +96,14 @@ As part of this proposal, we are mainly proposing three changes:
 
 1. Relax API validation on PVC update so as reducing `pvc.Spec.Resoures` is allowed, as long as requested size is `>pvc.Status.Capacity`.
 2. Add a new field in PVC API called - `pvc.Status.AllocatedResources` for the purpose of tracking used storage size. By default only api-server or resize-controller can set this field.
-3. Add a new field in PVC API called - `pvc.Status.ResizeStatus` for purpose of tracking status of volume expansion.
+3. Add a new field in PVC API called - `pvc.Status.ResizeStatus` for purpose of tracking status of volume expansion. Following are possible values of newly introduced `ResizeStatus` field:
+   - nil // General default because type is a pointer.
+   - empty // When expansion is complete, the empty string is set by resize controller or kubelet.
+   - ControllerExpansionInProgress // state set when external resize controller starts expanding the volune.
+   - ControllerExpansionFailed // state set when expansion has failed in external resize controller with a terminal error. Transient errors don't set ControllerExpansionFailed.
+   - NodeExpansionPending // state set when external resize controller has finished expanding the volume but further expansion is needed on the node.
+   - NodeExpansionInProgress // state set when kubelet starts expanding the volume.
+   - NodeExpansionFailed // state set when expansion has failed in kubelet with a terminal error. Transient errors don't set NodeExpansionFailed.
 3. Update quota code to use `max(pvc.Spec.Resources, pvc.Status.AllocatedResources)` when evaluating usage for PVC.
 
 ### Implementation
@@ -106,64 +113,22 @@ reduces the PVC request size, for both CSI and in-tree plugins they are designed
 
 We however do have a problem with quota calculation because if a previously issued expansion is successful but is not recorded(or partially recorded) in api-server and user reduces requested size of the PVC, then quota controller will assume it as actual shrinking of volume and reduce used storage size by the user(incorrectly). Since we know actual size of the volume only after performing expansion(either on node or controller), allowing quota to be reduced on PVC size reduction will allow an user to abuse the quota system.
 
-To solve aforementioned problem - we propose that, a new field will be added to PVC, called `pvc.Status.AllocatedResources`. When user expands the PVC, and when expansion-controller starts volume expansion - it will set `pvc.Status.AllocatedResources` to user requested value in `pvc.Spec.Resources` before performing expansion. The quota calculation will be updated to use `max(pvc.Spec.Resources, pvc.Status.AllocatedResources)` which will ensure that abusing quota will not be possible.
-
-Resizing operation will always work towards full-filling size recorded in `pvc.Status.AllocatedResources` and only when previous operation has finished(i.e `pvc.Status.ResizeStatus` is nil) or when previous operation has failed with a terminal error - it will use new user requested value from `pvc.Spec.Resources`.
-
-When user reduces `pvc.Spec.Resources`, expansion-controller will set `pvc.Status.AllocatedResources` to lower value only if:
-1. If `pvc.Status.ResizeStatus` is `ResizeFailedOnController` (indicating that previous expansion to last known `allocatedResources` failed with a final error).
-2. If `pvc.Status.ResizeStatus` is `ResizeFailedOnNode` and SP supports node-only expansion (indicating that previous expansion to last known `allocatedResources` failed on node with a final error).
-
-One key exception is - `pvc.Status.Allocatedresources` will not be lowered if expansion succeeded on the controller but failed on the node with a final error. The implementation of new `allocatedResources` will look like:
-
-
-```golang
-func (ctrl *resizeController) getNewSize(pvc *v1.PersistentVolumeClaim, pv *v1.PersistentVolume) resource.Quantity {
-    newSize := pvc.Spec.Resources.Requests.Storage()
-    if pvc.Status.ResizeStatus == nil || *pvc.Status.ResizeStatus == "" {
-        return *newSize
-    }
-    resizeStatus := *pvc.Status.ResizeStatus
-    allocatedSize := pvc.Status.AllocatedResources.Storage()
-
-    switch resizeStatus {
-    case v1.PersistentVolumeClaimResizeInProgress:
-        // If resize was inprogress before this means - we should keep trying to finish
-        // previously started expansion to last known `allocatedSize`.
-        if allocatedSize != nil {
-            newSize = allocatedSize
-        }
-    case v1.PersistentVolumeClaimResizeFailedOnController:
-        // if expansion failed on controller with a final error, then we can safely use new user requested size
-        newSize = pvc.Spec.Resources.Requests.Storage()
-    case v1.PersistentVolumeClaimResizeFailedOnNode:
-        if newSize.Cmp(*allocatedSize) > 0 {
-            newSize = pvc.Spec.Resources.Requests.Storage()
-        } else {
-            // newSize is smaller than whatever we tried to expand before
-            // and if expansion failed on the node, but it succeeded in control-plane
-            // then it is not safe to use lower value from pvc.Spec
-            if ctrl.resizer.SupportsControllerExpansion() {
-                newSize = allocatedSize
-            }
-        }
-
-    }
-    return *newSize
-}
-```
-
-One more key design choice is - once expansion on node failed and user requested lower size (by modifying `pvc.Spec.Resources`) - kubelet must not retry expanding the same volume to previous `allocatedResources`, because external-resizer will consider `ResizeFailedOnNode` state as a terminal error and allow lowering of `allocatedResources` - and at the same time kubelet will try to expand the volume to higher `allocatedResources`. This will be solved in kubelet by following code:
-
-```golang
-// read latest version of PVC from api-server
-if pvcStatusCap.Cmp(pvSpecCap) < 0 {
-    // this is the if block where kubelet performs expansion
-    if (pvc.Spec.Resources.Storage < pvc.Status.AllocatedResources ) && pvc.Status.ResizeStatus == ResizeFailedOnNode {
-        return false, nil
-    }
-}
-```
+To solve aforementioned problem - we propose that, a new field will be added to PVC, called `pvc.Status.AllocatedResources`. When user expands the PVC, and when expansion-controller starts volume expansion - it will set `pvc.Status.AllocatedResources` to user requested value in `pvc.Spec.Resources` before performing expansion and it will set `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`. The quota calculation will be updated to use `max(pvc.Spec.Resources, pvc.Status.AllocatedResources)` which will ensure that abusing quota will not be possible.
+
+Resizing operation in external resize controller will always work towards full-filling size recorded in `pvc.Status.AllocatedResources` and only when previous operation has finished(i.e `pvc.Status.ResizeStatus` is nil) or when previous operation has failed with a terminal error - it will use new user requested value from `pvc.Spec.Resources`.
+
+Kubelet on the other hand will only expand volumes for which `pvc.Status.ResizeStatus` is in `NodeExpansionPending` or `NodeExpansionInProgress` state and `pv.Spec.Cap > pvc.Status.Cap`. If a volume expansion fails in kubelet with a terminal error(which will set `NodeExpansionFailed` state) - then it must wait for resize controller in external-resizer to reconcile the state and put it back in `NodeExpansionPending`.
+
+When user reduces `pvc.Spec.Resources`, expansion-controller will set `pvc.Status.AllocatedResources` to lower value only if one of the following is true:
+
+1. If `pvc.Status.ResizeStatus` is `ControllerExpansionFailed` (indicating that previous expansion to last known `allocatedResources` failed with a final error) and previous control-plane has not succeeded.
+2. If `pvc.Status.ResizeStatus` is `NodeExpansionFailed` and SP supports node-only expansion (indicating that previous expansion to last known `allocatedResources` failed on node with a final error).
+3. If `pvc.Status.ResizeStatus` is `nil` or `empty` and previous `ControllerExpandVolume** has not succeeded.
+
+**Note**: Whenever resize controller or kubelet modifies `pvc.Status` (such as when setting both `AllocatedResources` and `ResizeStatus`) - it is expected that all changes to `pvc.Status` are submitted as part of same patch request to avoid race conditions.
+
+The complete expansion and recovery flow of both control-plane and kubelet is documented in attached PDF. The attached pdf - documents complete volume expansion flow via state diagrams and is much more exhaustive than the text above.
+
 
 #### User flow stories
 
@@ -174,11 +139,10 @@ if pvcStatusCap.Cmp(pvSpecCap) < 0 {
 - User increases 10Gi PVC to 100Gi by changing - `pvc.spec.resources.requests["storage"] = "100Gi"`.
 - Quota controller uses `max(pvc.Status.AllocatedResources, pvc.Spec.Resources)` and adds `90Gi` to used quota.
 - Expansion controller starts expanding the volume and sets `pvc.Status.AllocatedResources` to `100Gi`.
-- Expansion controller also sets `pvc.Status.ResizeStatus` to `ResizeInProgress`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`.
 - Expansion to 100Gi fails and hence `pv.Spec.Capacity` and `pvc.Status.Capacity `stays at 10Gi.
-- Expansion controller sets `pvc.Status.ResizeStatus` to `ResizeFailedOnController`.
+- Expansion controller sets `pvc.Status.ResizeStatus` to `ControllerExpansionFailed`.
 - User requests size to 20Gi.
-- Expansion controller tries expanding the PVC/PV to `20Gi` size.
 - Expansion controler notices that previous expansion to last known `allocatedresources` failed, so it sets new `allocatedResources` to `20G`
 - Expansion succeeds and `pvc.Status.Capacity` and `pv.Spec.Capacity` report new size as `20Gi`.
 - Quota controller sees a reduction in used quota because `max(pvc.Spec.Resources, pvc.Status.AllocatedResources)` is 20Gi.
@@ -187,46 +151,51 @@ if pvcStatusCap.Cmp(pvSpecCap) < 0 {
 - User increases 10Gi PVC to 100Gi by changing - `pvc.spec.resources.requests["storage"] = "100Gi"`
 - Quota controller uses `max(pvc.Status.AllocatedResources, pvc.Spec.Resources)` and adds `90Gi` to used quota.
 - Expansion controller starts expanding the volume and sets `pvc.Status.AllocatedResources` to `100Gi`.
-- Expansion controller also sets `pvc.Status.ResizeStatus` to `ResizeInProgress`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`.
 - Since expansion operations in control-plane are NO-OP, expansion in control-plane succeeds and `pv.Spec` is set to `100G`.
-- Expansion starts on the node and it fails on the node.
-- Kubelet sets `pvc.Status.ResizeStatus` to `ResizeFailedOnNode`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `NodeExpansionPending`.
+- Expansion starts on the node and kubelet sets `pvc.Status.ResizeStatus` to `NodeExpansionInProgress`.
+- Expansion fails on the node with a final error.
+- Kubelet sets `pvc.Status.ResizeStatus` to `NodeExpansionFailed`.
+- Since pvc has `pvc.Status.ResizeStatus` set to `NodeExpansionFailed` - kubelet will stop retrying node expansion.
+- At this point Kubelet will wait for `pvc.Status.ResizeStatus` to be `NodeExpansionPending`.
 - User requests size to 20Gi.
-- Kubelet notices that `pvc.Status.AllocatedResources > pvc.Spec.Resources` (that means user shrunk the volume) and last resize on node failed, it must not retry expansion to old `allocatedResources`.
-- At this point Kubelet will wait for `pvc.Status.AllocatedResources` to be updated.
 - Expansion controller starts expanding the volume and sees that last expansion failed on the node and driver does not have control-plane expansion.
 - Expansion controller sets `pvc.Status.AllocatedResources` to `20G`.
-- Expansion controller also sets `pvc.Status.ResizeStatus` to `ResizeInProgress`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`.
 - Since expansion operations in control-plane are NO-OP, expansion in control-plane succeeds and `pv.Spec` is set to `20G`.
 - Expansion succeed on the node with latest `allocatedResources` and `pvc.Status.Size` is set to `20G`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `NodeExpansionPending`.
+- Kubelet can now retry expansion and expansion on node succeeds.
+- Kubelet sets `pvc.Status.ResizeStatus` to empty string and `pvc.Status.Capacity` to new value.
 - Quota controller sees a reduction in used quota because `max(pvc.Spec.Resources, pvc.Status.AllocatedResources)` is 20Gi.
 
 
 ##### Case 3 (Malicious user)
 - User increases 10Gi PVC to 100Gi by changing `pvc.spec.resources.requests["storage"] = "100Gi"`
 - Quota controller uses `max(pvc.Status.AllocatedResources, pvc.Spec.Resources)` and adds `90Gi` to used quota.
 - Expansion controller slowly starts expanding the volume and sets `pvc.Status.AllocatedResources` to `100Gi` (before expanding).
-- Expansion controller also sets `pvc.Status.ResizeStatus` to `ResizeInProgress`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`.
 - At this point -`pv.Spec.Capacity` and `pvc.Status.Capacity` stays at 10Gi until the resize is finished.
 - While the storage backend is re-sizing the volume, user requests size 20Gi by changing `pvc.spec.resources.requests["storage"] = "20Gi"`
 - Expansion controller notices that previous expansion to last known `allocatedresources` is still in-progress.
-- Expansion controller starts expanding the volume but `allocatedResources` stays at `100G`.
+- Expansion controller keeps expanding the volume to `allocatedResources`.
 - Expansion to `100G` succeeds and `pv.Spec.Capacity` and `pvc.Status.Capacity` report new size as `100G`.
 - Although `pvc.Spec.Resources` reports size as `20G`, expansion to `20G` is never attempted.
 - Quota controller sees no change in storage usage by the PVC because `pvc.Status.AllocatedResources` is 100Gi.
 
-#### Case 4(Malicious User and rounding to GB/GiB bounaries)
+##### Case 4(Malicious User and rounding to GB/GiB bounaries)
 - User requests a PVC of 10.1GB but underlying storage provides provisions a volume of 11GB after rounding.
 - Size recorded in `pvc.Status.Capacity` and `pv.Spec.Capacity` is however still 10.1GB.
 - User expands expands the PVC to 100GB.
 - Quota controller uses `max(pvc.Status.AllocatedResources, pvc.Spec.Resources)` and adds `89.9GB` to used quota.
 - Expansion controller starts expanding the volume and sets `pvc.Status.AllocatedResources` to `100GB` (before expanding).
-- Expansion controller also sets `pvc.Status.ResizeStatus` to `ResizeInProgress`.
+- Expansion controller also sets `pvc.Status.ResizeStatus` to `ControllerExpansionInProgress`.
 - At this point -`pv.Spec.Capacity` and `pvc.Status.Capacity` stays at 10.1GB until the resize is finished.
 - while resize was in progress - expansion controler crashes and loses state.
 - User reduces the size of PVC to 10.5GB.
 - Expansion controller notices that previous expansion to last known `allocatedresources` is still in-progress.
-- Expansion controller starts expanding the volume but `allocatedResources` stays at `100G`.
+- Expansion controller starts expanding the volume to last known `allocatedResources` (which is `100GB`).
 - Expansion to `100G` succeeds and `pv.Spec.Capacity` and `pvc.Status.Capacity` report new size as `100G`.
 - Although `pvc.Spec.Resources` reports size as `10.5GB`, expansion to `10.5GB` is never attempted.
 - Quota controller sees no change in storage usage by the PVC because `pvc.Status.AllocatedResources` is 100Gi.
@@ -238,9 +207,9 @@ if pvcStatusCap.Cmp(pvSpecCap) < 0 {
 
 ## Graduation Criteria
 
-* *Alpha* in 1.22 behind `RecoverExpansionFailure` feature gate with set to a default of `false`.
-* *Beta* in 1.23: Since this feature is part of general `ExpandPersistentVolumes` feature which is in beta, we are going to move this to beta with enhanced e2e and more stability improvements.
-* *GA* in 1.25 along with `ExpandPersistentvolumes` feature. The list of issues for volume expansion going GA can be found at - https://github.com/orgs/kubernetes-csi/projects/12.
+* *Alpha* in 1.23 behind `RecoverExpansionFailure` feature gate with set to a default of `false`.
+* *Beta* in 1.24: Since this feature is part of general `ExpandPersistentVolumes` feature which is in beta, we are going to move this to beta with enhanced e2e and more stability improvements.
+* *GA* in 1.26 along with `ExpandPersistentvolumes` feature. The list of issues for volume expansion going GA can be found at - https://github.com/orgs/kubernetes-csi/projects/12.
 
 ### Test Plan
 
@@ -259,7 +228,7 @@ if pvcStatusCap.Cmp(pvSpecCap) < 0 {
 * **How can this feature be enabled / disabled in a live cluster?**
   - [x] Feature gate (also fill in values in `kep.yaml`)
     - Feature gate name: `RecoverExpansionFailure`
-    - Components depending on the feature gate: kube-apiserver, kube-controller-manager, csi-external-resizer
+    - Components depending on the feature gate: kube-apiserver, kube-controller-manager, csi-external-resizer, kubelet
   - [ ] Other
     - Describe the mechanism:
     - Will enabling / disabling the feature require downtime of the control
@@ -272,6 +241,10 @@ if pvcStatusCap.Cmp(pvSpecCap) < 0 {
   so it should not break any of existing automation. This means that if `pvc.Status.AllocatedResources` is available it will be
   used for calculating quota.
 
+  One more thing to keep in mind is - enabling this feature in kubelet while keeping it disabled in external-resizer will cause
+  all volume expansions operations to get stuck(similar thing will happen when feature moves to beta and kubelet is newer but external-resizer sidecar is older).
+  This limitation will be documented in external-resizer.
+
 * **Can the feature be disabled once it has been enabled (i.e. can we rollback
   the enablement)?**
   Yes the feature gate can be disabled once enabled. However quota resources present in the cluster will be based off a field(`pvc.Status.AllocatedResources`) which is no longer updated.