doc: UML diagram for volume creation and deletion

[pohly]

What type of PR is this?
/kind documentation

What this PR does / why we need it:

While this information is available in various design documents, users
of the external-provisioner benefit from getting a summary directly in
the documentation of external-provisioner. This is a good opportunity
to call our expected behavior of the CSI driver.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[pohly]

A first revision of this was in PR #524. We can and should merge it separately because it is documenting the current status, not that enhancement.

/assign @jsafrane

[verult]

/cc

[msau42]

This is really useful for developers of external-provisioner/pv controller, but I like to keep the README focused on users. Can we move this to a docs/ subpage and reference it from the readme?

[pohly]

Okay.

[pohly]

@msau42 given what we discussed in PR #524 about the pv protection finalizer, would it be more correct to show external-provisioner and the PV controller to act on a PV at the same time in parallel? One removes the finalizer, the other handles the DeleteVolume call and deletes the PV?

[msau42]

would it be more correct to show external-provisioner and the PV controller to act on a PV at the same time in parallel? One removes the finalizer, the other handles the DeleteVolume call and deletes the PV?

Sure, that sounds good. There isn't actually a finalizer to protect the PV object from being deleted while the disk still exists.

[pohly]

would it be more correct to show external-provisioner and the PV controller to act on a PV at the same time in parallel? One removes the finalizer, the other handles the DeleteVolume call and deletes the PV?

No, the current sequential flow is correct. First the PV controller sets phase: Released, then the external-provisioner deletes the PV, then the PV controller removes the finalizer and the PVC gets removed.

I checked by deleting the PVC while the CSI driver was down: the PVC continues to have the finalizer until the CSI driver comes back up and deletes the PVC.

What's the rationale for not removing the finalizer as soon as the PVC is gone and the phase is Released?

[msau42]

This is not my understanding of how the pv protection controller works. It only waits for PV to be in released state:
https://github.com/kubernetes/kubernetes/blob/6d43e2b3bb55237fbb64118dc13beac259745983/pkg/controller/volume/pvprotection/pv_protection_controller.go#L143

And PV controller sets PV to released state before doing the delete operation:
https://github.com/kubernetes/kubernetes/blob/6d43e2b3bb55237fbb64118dc13beac259745983/pkg/controller/volume/persistentvolume/pv_controller.go#L639

[pohly]

This is not my understanding of how the pv protection controller works. It only waits for PV to be in released state:
https://github.com/kubernetes/kubernetes/blob/6d43e2b3bb55237fbb64118dc13beac259745983/pkg/controller/volume/pvprotection/pv_protection_controller.go#L143

The code block which removes the finalizer is only called when IsDeletionCandidate returns true. That function only returns true if the PV has a deletion time stamp: https://github.com/kubernetes/kubernetes/blob/62876fc6d93e891aa7fbe19771e6a6c03773b0f7/pkg/controller/volume/protectionutil/utils.go#L24-L28

That's consistent with my observation that the finalizer is kept in place until someone deletes the PV or rather, because of the finalizer, sets the deletion time stamp.

That logic is three years old, from kubernetes/kubernetes#58743 (search for isDeletionCandidate). 🤷

[msau42]

Ah ok thanks for pointing that out, that explains it.

I think it is preferable to wait for the volume to actually be deleted, that prevents the volume from leaking unless the user went in and deleted the PV object themselves.

[pohly]

I think it is preferable to wait for the volume to actually be deleted, that prevents the volume from leaking

The same can be achieved when removing the finalizer after setting phase: Released and before external-provisioner deletes the PV. Having the finalizer still set after just delays the actual deletion because the PV controller then must do another update.

A finalizer that automatically gets removed when the object gets deleted still makes no sense to me.

[msau42]

The same can be achieved when removing the finalizer after setting phase: Released and before external-provisioner deletes the PV.

If the finalizer gets removed before external-provisioner deletes the PV, then a user could delete the PV object before external-provisioner gets to it, causing the volume to leak.

[pohly]

A user can also delete the PV when the finalizer is set and then the PV controller will remove the finalizer without the external-provisioner ever getting involved. The finalizer makes no difference, a user simply must not delete PVs.

Hmm that kind of makes the purpose of the finalizer a little moot then. But that's a separate discussion :-)

[msau42]

/approve

Just one last nit from me to move the design section out of the top level README

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pohly]

@msau42 I've created a separate doc/design.md and also changed the wording of the last step in the diagram:

**PV controller** determines that PV has no volume -> **PV controller** checks the volume one last time
and removes the pv protection finalizer.

That's more accurate because the PV controller doesn't really check whether the volume still exists.

[msau42]

Sorry I accidentally modified your comment instead of replying to it. I tried to restore it back to the original.. Anyway, to be most accurate, should we say it's the storage protection controller that removes the finalizer?

[pohly]

Anyway, to be most accurate, should we say it's the storage protection controller that removes the finalizer?

I'm talking about pkg/controller/volume/pvprotection/pv_protection_controller.go - is that the same thing? That's where the kubernetes.io/pv-protection finalizer gets removed.

[pohly]

I agree, the last step is "PV protection controller checks the volume one last time and removes the pv protection finalizer".

I used the term "PV controller" already earlier, without double-checking in the source code which part that really is. Did I get those steps right? I'm not sure anymore.

Probably it would be good to define each term and link it to the source code.

[pohly]

This doesn't need to go into the next release. We can enhance it after New Years.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[pohly]

/remove-lifecycle stale

[jsafrane]

sorry, I missed this. IMO it seems to be accurate and most @msau42's comments were addressed.
/lgtm