Fix Attachment ID matching

[jsafrane]

When deciding if a string is VolumeAttachment ID or a volume unique name, make sure that volumes of a CSI driver named "csi-foo.bar" are recognized as unique volume names and not attachment IDs.

Use a regexp that allows only characters that are in sha256 (0-9,a-f), which is part of the attachment ID. Unique volume names always have "^" in their name and the regexp won't match them.

/kind bug

Which issue(s) this PR fixes:

Fixes #97230

Fixed detaching CSI volumes from nodes when a CSI driver name has prefix "csi-".

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[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:

• pkg/volume/OWNERS [jsafrane]

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

[jsafrane]

/priority important-soon
/triage accepted

[gnufied]

I have been wondering if we can get rid of this check altogether. Do you remember why was this check added? It sounds like it was added because we record intree volume names in node.Status.VolumeAttached field and when ASOW is rebuild during ADC restart, we try to detach using intree names?

[jsafrane]

A comment says it's related to CSI migration:

kubernetes/pkg/volume/csi/csi_attacher.go

Lines 416 to 421 in d9b08c6

	if isAttachmentName(volumeName) {
	// Detach can also be called with the attach ID as the `volumeName`. This codepath is
	// hit only when we have migrated an in-tree volume to CSI and the A/D Controller is shut
	// down, the pod with the volume is deleted, and the A/D Controller starts back up in that
	// order.
	attachID = volumeName

[gnufied]

yes I did read the comment, but I am trying to understand condition under which we will hit this case.

[jsafrane]

I checked it with in-tree AWS EBS PV and in-line AWS EBS volume in a pod, both with CSI migration enabled and disabled, and I've never experienced attachment ID in volumeName.

Looking at the code, there was a corner case where attachment ID was used in the volume name, 5dde1df#diff-3e8f8a6e5eca57db8e84bcc6c896f0a3b10289fe55d8e999b4bdf1cfdc0de394R447-R452

But the code got removed later on in 129f153#diff-3e8f8a6e5eca57db8e84bcc6c896f0a3b10289fe55d8e999b4bdf1cfdc0de394L542-L546

It seems to me that volumeName is always unique volume ID in format plugin^unique ID. I think we could remove the whole isAttachmentName().

@ddebroy, you wrote the later commit referenced here, what do you think?

[jsafrane]

For the record, node.status.volumesAttached with an in-tree in-line AWS EBS volume migrated to CSI looks like a CSI volume:

  volumesAttached:
  - devicePath: ""
    name: kubernetes.io/csi/ebs.csi.aws.com^vol-07acc366093a6af02
  volumesInUse:
  - kubernetes.io/csi/ebs.csi.aws.com^vol-07acc366093a6af02

(today's master, almost Kubernetes 1.23)

[ddebroy]

Great catch! I have filed #106749 to ensure the corner case situation is handled by reintroducing 129f153#diff-3e8f8a6e5eca57db8e84bcc6c896f0a3b10289fe55d8e999b4bdf1cfdc0de394L510 which was incorrectly removed.

[jsafrane]

I created #107025 to remove the whole AttachID mathing, IMO it's not used anywhere.

[jsafrane]

/hold
#107025 looks like a better solution

[k8s-ci-robot]

@jsafrane: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jsafrane]

/close

[k8s-ci-robot]

@jsafrane: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.