Reorder API calls after successful detach

[jsafrane]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Remove a finalizer on a VolumeAttachment (VA) first and then mark its state as detached (if the object still exists).

This ensures that ControllerUnpublish is called only once. It won't be called on VA without the finalizer, even if it has attached: true.

The other way around, i.e. marking the VA as detached first and removing the finalizer later, it can happen that the attacher calls ControllerUnpublish twice. A VA with the finalizer, deletionTimestamp and attached: false is considered as "uncertainly attached" - a previous ControllerPublish might have timed out and the attacher needs to confirm the volume is detached with ControllerUnpublish.

From outer perspective:

• Before this PR, a detaching VolumeAttachment gets attached: false first and then the finalizer is removed. (And ControllerUnpublish may be called twice).
• With this PR, there is a high chance that the finalizer is removed and the attachment is deleted in the API server before it's marked as attached: false. A deleted VolumeAttachment should be considered as detached. (And ControllerUnpublish is called once).

Which issue(s) this PR fixes:

Fixes #587

Does this PR introduce a user-facing change?:

Fixed repeated call to ControllerUnpublishVolume by reordering its finalizer removal and marking VolumeAttachment as detached. The VolumeAttachment can now be deleted in the API server before it's marked as `attached: false`.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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 [jsafrane]

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

[gnufied]

/assign

[AndrewSirenko]

markAsDetached changes look good to me and indeed remove all duplicate ControllerUnpublish calls.

Validated pr by running 100 replica sts scale tests comparing attacher built from this commit vs v4.8.0. (On Karpenter auto-scalar managed nodes)

Before PR, 17 extra ControllerPublish calls are made. After PR no extra calls are made. See full metrics and attacher logs in this gist.

Thanks!

PS: In Feb 5 SIG Triage I mentioned Karpenter may rely on VA resource. On further look Karpenter only relies on existence of VA resource to stall node termination, not va.attached, so this PR is safe for Karpenter.

[gnufied]

Non-existence of VA objects and attached=false, both should signal the fact that volume is detached and hence we should be fine I think.

/lgtm