In-progress (or non-final) VAC modifications should not allow next VAC to be applied until current one succeeds (or fails)

[gnufied]

VAC modifications which are currently failing with non-final errors should prevent application of next VAC, until current target succeeds or fails with final error.

This will make it easier to reason about volume state.

[gnufied]

cc @AndrewSirenko @sunnylovestiramisu

[bswartz]

How would this even be possible? The spec is mutable and the reconciler just reconciles them.

[huww98]

I think this should be fixed by #512 ?

[huww98]

How would this even be possible? The spec is mutable and the reconciler just reconciles them.

@bswartz In the original design, when external-resizer starts, it reconciles towards pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName. After it gets a final error, it will start to reconcile torwards pvc.Spec.VolumeAttributesClassName.

But there is a bug in the implementation at https://github.com/kubernetes-csi/external-resizer/pull/512/files#diff-4277b1740fa83ba8c035a7583123dd6c54a40b8eb03cc0e782d7e1a604b75504R135

[bswartz]

@bswartz In the original design, when external-resizer starts, it reconciles towards pvc.Status.ModifyVolumeStatus.TargetVolumeAttributesClassName. After it gets a final error, it will start to reconcile torwards pvc.Spec.VolumeAttributesClassName.

This is unfortunate, because it makes it impossible to abort a change that's failing and change it to a different value instead. What is the reason for the extra target field, rather than always reconciling to the spec and updating the status when it's done?

[gnufied]

This is unfortunate, because it makes it impossible to abort a change that's failing and change it to a different value instead. What is the reason for the extra target field, rather than always reconciling to the spec and updating the status when it's done?

As replied offline without this extra field in status one can easily abuse quota. I have updated KEP with all the corner cases of error handling - https://github.com/kubernetes/enhancements/pull/5462/files

[bswartz]

As replied offline without this extra field in status one can easily abuse quota. I have updated KEP with all the corner cases of error handling - https://github.com/kubernetes/enhancements/pull/5462/files

I retract my comment. I was confused about final and infeasible errors, and I didn't understand that even on non-infeasible errros, it's possible to change the value, as long as the error is final. I understand why non-final errors should prevent changing the value.

[sunnylovestiramisu]

I can see if it is in-progress, we do not let it to change to a new vac.

But if it is non-final like backend temp not available, aborted etc. shouldn't the user still be able to update to a new vac?

[sunnylovestiramisu]

To keep consistency with other operations like CreateVolume, we will treat in-progress and non final error the same in this case. And for both in-progress and non final error, the Status.ModifyVolumeStatus.Status will be PersistentVolumeClaimModifyVolumeInProgress and the non-final error will exist in ctrl.uncertainPVCs cache.

Which is:

if pvc.Status.ModifyVolumeStatus.Status == corev1.PersistentVolumeClaimModifyVolumeInProgress &&
   (isInUncertainCache(pvc) || !hasModifyErrorCondition(pvc)) {
    // This block will execute if modification is in-progress OR has a non-final error.
}

[sunnylovestiramisu]

The fix is in #513