Fix canonical nvme device resolution in more cases

[PaulFurtado]

Is this a bug fix or adding new feature?
Bug fix further improving on #1082

What is this PR about? / Why do we need it?

• It allows the /dev/xvdXX symlinks to continue to function, but properly identifies them as nvme devices based on the /dev/nvme prefix
• No matter what kind of symlinking is setup, it will always find the canonical block device because all symlinks are always resolved now. This means that if some crazy non-nitro system had /dev/xvdb symlinked to /dev/sdb, even that would work correctly.
• nvme devices resolved via the /dev/xvdXX symlinks will now correctly handle appending the partition string
• The /dev/disk/by-id fallback continues to work correctly
• It fixes logging for node_linux.go: this file was using klog v2 while the rest of the repo was using v1 so its logging was going nowhere

What testing is done?

• Tested on:
  • a kubelet with just the /dev/xvdXX udev rules for nvme devices
  • a kubelet with just the /dev/disk/by-id udev rules for nvme devices
  • a kubelet with both sets of udev rules present
• On each of those, I confirmed that:
  • pods with PVCs can be created and deleted successfully
  • when kubelet in 1.21 is restarted, the CSI driver correctly recognizes that the volumes are already mounted and the operation is idempotent

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Paul Furtado (ee7162a)

[k8s-ci-robot]

Welcome @PaulFurtado!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-ebs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @PaulFurtado. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[ayberk]

/ok-to-test

/assign @wongma7

[wongma7]

thanks, this makes sense to me at a glance. from what i can tell resolving symlinks is exactly what linux means "canonicalizing" a path is before putting entry in mount table https://github.com/util-linux/util-linux/blob/441f9b9303d015f1777aec7168807d58feacca31/libmount/src/cache.c https://github.com/util-linux/util-linux/blob/441f9b9303d015f1777aec7168807d58feacca31/lib/canonicalize.c#L121 so it follows that if we are trying to canonicalize device path to compare it with what's in mount table

aws-ebs-csi-driver/pkg/driver/node.go

Lines 200 to 203 in 5d78287

	// This operation (NodeStageVolume) MUST be idempotent.
	// If the volume corresponding to the volume_id is already staged to the staging_target_path,
	// and is identical to the specified volume_capability the Plugin MUST reply 0 OK.
	if device == source {

we should follow symlinks all the way, not make any assumptions about the depth of symlinks or such which could break at any time with a EC2 new instance type etc.

[wongma7]

should we still try to fallback to the findNvmeVolume method in case there is an error in this block (if lstat or evalsymlinks somehow fails)? Is there any situation where we receive a devicePath that exists but cannot* be lstatted or evalsymlink'd? If not then it is not necessary to fallback and this PR is good as-is

[PaulFurtado]

@wongma7 I cannot think of a situation where that could be possible.

[PaulFurtado]

Also worth noting: we have been running with this patch in our clusters internally and it has eliminated all of the EBS attach/detach issues we've had since upgrading kubernetes from 1.19 to 1.21.

[wongma7]

/lgtm
/approve

thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PaulFurtado, wongma7

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

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