KEP-3751: add error handling

[huww98]

• One-line PR description: add error handling, and some cleanups

• Issue link: Kubernetes VolumeAttributesClass ModifyVolume #3751

• Other comments:

I'm aware of another PR #5462 also trying to address this.
As per our discussion on the meeting, the behavior described in this PR should be the default. So I'm proposing this for early review.
We may also add a strict-mode like #5462 to ensure quota restriction in any situation, which I will add in the following days based on this PR.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: huww98
Once this PR has been reviewed and has the lgtm label, please assign msau42 for approval. 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

[huww98]

/cc @msau42 @jsafrane @xing-yang @gnufied @sunnylovestiramisu

[huww98]

At init, it should be enough to only mark InProgress volumes as uncertain. Before we mark a volume Infeasible, we will unset uncertain. Before we retry an Infeasible modify, we will mark it as InProgress again.

[gnufied]

Can you explain why is target=spec after infeasible error?

[gnufied]

It is not clear tbh, what those action mean. What does Action-6 mean?

[huww98]

Added an example and some more descriptions to this table. Hopes this will help.

[gnufied]

Is this a indentation error or did you really meant infeasible to be indented?

Apart from flow-chart, IMO the recovery options from infeasible should be clearly specified here. Like what happens if:

1. nil --> A(infeasible) --> nil
2. A --> B (infeasible) --> nil

The flow chart is helpful, but having a summary here helps a casual reader of the spec.

[huww98]

Yes, I want it to be indented. Because all infeasible errors are final errors, and inherits the policy of final errors.

On the two lines after this comment:

If the Spec.VolumeAttributesClassName is set to nil, we will not retry the request.

Is this clear enough for you?
BTW, I think case 2 is not allowed by APIServer, but if this really happens, we can handle it. I will add a sentence to further clarify the quota.

[gnufied]

BTW, I think case 2 is not allowed by APIServer, but if this really happens, we can handle it. I will add a sentence to further clarify the quota.

Oops, I meant:

A --> B(Infeasible) --> A

[gnufied]

If I am reading your flowchart right, it looks like - for nil --> A (infeasible) --> nil, the target will be set back to nil and hence no quota will be counted towards A. I thought you were arguing that, quota for A should still be counted?

[huww98]

Yes, quota for A should still be counted? I will remove the target=spec after infeasible error. And for A (infeasible) --> nil, it should go to the "stop reconcile" at the bottom of the chart.

In the implementation, I also have a branch

if spec == nil && (status == nil || status == Infeasible) {
    return
}

I think I'd better also add this to the flowchart.

[huww98]

Can you explain why is target=spec after infeasible error?

Thanks for pointing this out. I think I overlooked this case when I write kubernetes-csi/external-resizer#510 . I will fix the flowchart and the code. We should just keep target at its original value here, because in the uncertain case, target is not the same as spec.

[AndrewSirenko]

So to be clear, even if driver reports infeasible INVALID_ARGUMENTS, you cannot go back to nil because driver may have secretly modified volume?

[sunnylovestiramisu]

I think it means the PVC's VAC class field can still be nil, but there is no guarantee that the underlying volume has been reverted.

[huww98]

Yes, even we get INVALID_ARGUMENTS at the last attempt, their is no guarantee about what happened in the previous attempts, e.g.:

• nil -> A (INTERNAL) -> A (INVALID_ARGUMENTS) -> nil
• nil -> B (INTERNAL) -> A (INVALID_ARGUMENTS) -> nil

So, to keep our promise to admin (mentioned later in the next section): every PVC with Status.ModifyVolumeStatus == nil is properly covered by quota, we do not revert PVC.Status.ModifyVolumeStatus from Infeasible to nil automatically.

This also reminds user that the volume may have some changes applied, and may not be the same as their other volumes.

[huww98]

I realized that we cannot clear the pending status when reverting to nil, because Pending status may overwrite the previous InProgress status. I also refined the language of this paragraph. Hopes this will make it more clear.

[huww98]

Can you explain why is target=spec after infeasible error?

Thanks for pointing this out. I think I overlooked this case when I write kubernetes-csi/external-resizer#510 . I will fix the flowchart and the code. We should just keep target at its original value here, because in the uncertain case, target is not the same as spec.

Pushed the fix to kubernetes-csi/external-resizer#512 . @sunnylovestiramisu @gnufied PTAL

[gnufied]

Thanks. I see two distinct flows for going to Done state in the flow chart one via pending state and another via infeasible state and what happens to status field is different for those. I would think that for infeasible "done", we won't be resetting status but whereas for Pending - we will. Is that accurate?

For example:

1.nil --> A(infeasible) --> nil, then .status.modifyStatus == whatever old value was
2. nil --> A (pending) ---> nil, then .status.modifyStatus == nil

Can you update the flowchart to reflect this?