Update document about error handling

[gnufied]

This PR documents error handling in external-resizer and how each type of error is handled.

[gnufied]

/assign @msau42 @jsafrane @xing-yang

[msau42]

@sunnylovestiramisu @AndrewSirenko

[AndrewSirenko]

Would add to non-final errors that "CO is unsure if volume was modified"

Because CSI Operation CAN time out despite volume modification occurring in storage provider. E.g. storage providers where modifications may take a while

[gnufied]

Kinda added extra wording. But not exactly what you wrote above. Please do check.

[AndrewSirenko]

If volume modification to a VAC is failing with a final and infeasible error

I thought we ONLY cancel modification on infeasible err.

This is to prevent partial modification on final errs like InternalErr, which could lead to half-modified volumes for drivers.

[sunnylovestiramisu]

I was also thinking we ONLY cancel modification(rollback) on Infeasible err, kubernetes-csi/external-resizer#487 is based on this assumption.

[bswartz]

I think he's just stating that infeasible errors are a subset of final errors. All infeasible errors are final, but not all final errors are infeasible. This should probably say:

"failing with an infeasible error (but not other final errors),"

[gnufied]

Yes I meant and to do some heavy lifting here. Since infeasible are already final, both conditions must be true. I will update the wording.

[gnufied]

I fixed it. PTAL.

[bswartz]

I think he's just stating that infeasible errors are a subset of final errors. All infeasible errors are final, but not all final errors are infeasible. This should probably say:

"failing with an infeasible error (but not other final errors),"

[AndrewSirenko]

/lgtm

[huww98]

As a developer of CSI driver, and a cluster admin of our infra, I still cannot accept this.

they will not be undone

This means, when I specify my volume to have 2000 IOPS, and PVC.status tells me the reconcile finishes, but my volume may actually have only 1000 IOPS. And I can never observe the abnormal from Kubernetes API, until something more serious goes wrong:

• If the performance is higher than expected, it will incur extra cost
• If the performance is lower than expected, it can result in unexpected latency to workload, even catastrophic system failure
• If we add topology integration to VAC, it also means then PV nodeAffinity can be out-of-sync, which will cause Pod pending or stuck due to scheduled to wrong node.
• It is also subject to potential quota abuse

This mechanism exists only to prevent perpetual call

This does not make sense. After VAC is rolled back, if the volume is already at the desired state, SP should just return OK and do nothing. There is no reason the call will be perpetual. If the volume is actually partially modified, and cannot be rolled back by SP, it is better to let user notice this, rather than just hide it.

We never end the reconcile process with an failed gRPC call. e.g.

• We only delete VolumeAttachment if ControllerUnpublishVolume returns OK.
• We only clear PVC.Status.AllocatedResourceStatuses if ControllerExpandVolume returns OK.

So we should do the same, only clear PVC.Status.ModifyVolumeStatus if ControllerModifyVolume returns OK, and never cancel modification.

Storage providers and users are recommended to roll forward to different VAC, if desired behaviour is resetting the VAC to some pre-specified value for all mutable_parameters.

In Kubernetes, spec specifies the desired state, not action. Which ever state the user specifies, we should try to bring the underlying system to the specified state. It would be ridiculous if two VAC specifies the same state, but only one of them will work.

[bswartz]

This change is being made in compliance with the CSI spec change container-storage-interface/spec#597. "Infeasible" in this KEP refers to the errors in the CSI spec such as NOT_FOUND and INVALID_ARGUMENT for which the SP must not have made any changes. The proposed sidecar behavior depends on the SP's spec compliance for safety in this case. If the SP returns a final non-infeasible error (such as INTERNAL) then the wording for final errors applies, and it's up to cluster operators to ensure that any new VAC will overwrite any partially applied settings from an older VAC.

[huww98]

See my comment at container-storage-interface/spec#597 (comment)
Basically, if some other errors happened before INVALID_ARGUMENT, we still not sure about the volume state.

[bswartz]

We're taking the stance that, if that happens then the SP has violated the spec, either intentionally or through a bug. In either case, undefined behavior could occur. It is incumbent on the SP to prevent the case you describe (returning INVALID_ARGUMENT after making some changes) in order to remain spec complaint. We realize this is a tightening of the spec, which is why we want to do it while this RPC is still tagged alpha.

[huww98]

No, we are discussing the same case as you replied at container-storage-interface/spec#597 (comment)

I'd state the issue here again. Say initially the PVC has VAC A, and user modified it to B:

retry case

1. CO calls ControllerModifyVolume(B)
2. the request failed, maybe due to process restart, or network error, etc
3. CO retries ControllerModifyVolume(B)
4. SP return INVALID_ARGUMENT

With the tightened spec, SP cannot have made any change between 3 and 4. But SP can still make change between 1 and 2, and not violating the tightened spec. So assuming the volume is not changed at step 4 is not safe for CO.

A -> B -> C case

1. CO calls ControllerModifyVolume(B)
2. the request failed with final error
3. User changed his mind and set VAC name to C
4. CO calls ControllerModifyVolume(C)
5. SP return INVALID_ARGUMENT

Similar to the previous case, SP can still make change between 1 and 2. Now if user try to revert to A, as proposed in this PR, "(partial volume modifications) will not be undone". So the volume will get into the state I described previously: the parameters of A is not applied, but user cannot detect this from Kubernetes API.

[bswartz]

I'd state the issue here again. Say initially the PVC has VAC A, and user modified it to B:

retry case

1. CO calls ControllerModifyVolume(B)
2. the request failed, maybe due to process restart, or network error, etc
3. CO retries ControllerModifyVolume(B)
4. SP return INVALID_ARGUMENT

With the tightened spec, SP cannot have made any change between 3 and 4. But SP can still make change between 1 and 2, and not violating the tightened spec. So assuming the volume is not changed at step 4 is not safe for CO.

If the SP returns INVALID_ARGUMENT in (4), then the there must have been some parameter in B which was invalid. What is invalid should not change over time. That's part of what we mean by infeasible -- it's that the passage of time won't change whether the request succeeds or not. Assuming the SP adheres to this logic, then we know nothing could have changed between 1 and 2 either, regardless of the error code returned.

A -> B -> C case

1. CO calls ControllerModifyVolume(B)
2. the request failed with final error
3. User changed his mind and set VAC name to C
4. CO calls ControllerModifyVolume(C)
5. SP return INVALID_ARGUMENT

Similar to the previous case, SP can still make change between 1 and 2. Now if user try to revert to A, as proposed in this PR, "(partial volume modifications) will not be undone". So the volume will get into the state I described previously: the parameters of A is not applied, but user cannot detect this from Kubernetes API.

In this case, C contained something the SP couldn't understand, and so the SP made no changes between (2) and (5). The actual state of the volume is somewhere between A and B, and the status reflects A. I'm acutally unsure which quota is consumed. I assume it's at least quota from A, and maybe also B and or C (although it can't be all 3).

In any case, this is an admin error, and undefined results are expected if an admin puts bad parameters into a VAC and then does this kind of manipulation. I'm not arguing that the results are perfect or desirable in this case, I'm arguing that it's safe from intentional exploitation (assuming the admin creates sane VACs and quotas), and that it's as good as we can do considering that nothing in the CO remembers the old state, so only forward progress is possible.

[huww98]

undefined results are expected if an admin puts bad parameters into a VAC

Whether the parameters are valid may depends on many aspects, such as the region/zone that the volume locate at, the current status of the volume, etc. It is not something admin can completely avoided in advance.

it's as good as we can do considering that nothing in the CO remembers the old state, so only forward progress is possible.

Please take a look at my proposal at https://docs.google.com/document/d/1VebyLSRcngn3_9wqaF5OUmxu60gwX7dN8euXRUqJSXA/edit?usp=sharing

Basically, I'm proposing to call ControllerModifyVolume(A) again in this case, to at least ensure the parameters explicitly specified for A are applied.

[huww98]

Assuming the SP adheres to this logic, then we know nothing could have changed between 1 and 2 either, regardless of the error code returned.

Yes, I agree that SP should do this. But assuming is not something we can rely on. Lets image that SP can do this:

changed, err := modify_part_1()
if err != nil { ... }

err = modify_part_2()
if err != nil {
    if changed { return INTERNAL }
    else { return INVALID_PARAMETER }
}

At first attempt, modify_part_1 succeeded, but modify_part_2 does not. at second attempt, SP would return INVALID_PARAMETER. I agree this is not recommended. But it is compliant to CSI spec.

[sunnylovestiramisu]

"With the tightened spec, SP cannot have made any change between 3 and 4. But SP can still make change between 1 and 2, and not violating the tightened spec. So assuming the volume is not changed at step 4 is not safe for CO."

If 2 is a non final error, we do not let a new modification to C happen.

[bswartz]

/lgtm

[bswartz]

This change is being made in compliance with the CSI spec change container-storage-interface/spec#597. "Infeasible" in this KEP refers to the errors in the CSI spec such as NOT_FOUND and INVALID_ARGUMENT for which the SP must not have made any changes. The proposed sidecar behavior depends on the SP's spec compliance for safety in this case. If the SP returns a final non-infeasible error (such as INTERNAL) then the wording for final errors applies, and it's up to cluster operators to ensure that any new VAC will overwrite any partially applied settings from an older VAC.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bswartz, gnufied
Once this PR has been reviewed and has the lgtm label, please ask for approval from jsafrane. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-storage/OWNERS

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]

New changes are detected. LGTM label has been removed.