-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor get device by volumeID routines #940
Refactor get device by volumeID routines #940
Conversation
Hi @huww98. 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 Once the patch is verified, the new status will be reflected by the 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. |
b46548b
to
fb583a6
Compare
fb583a6
to
9f98d01
Compare
e0e2fad
to
49c755c
Compare
49c755c
to
33714af
Compare
41a9f4b
to
630230b
Compare
} | ||
|
||
// this is danger in Bdf mode | ||
if !m.DisableSerial { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
log.Infof("AttachDisk: Successful attach bdf disk %s to node %s device %s by DiskID/Device mapping", diskID, nodeID, deviceName) | ||
return deviceName, nil | ||
_, err := DefaultDeviceManager.GetRootBlockByVolumeID(diskID) | ||
if err != nil && bdf != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these nest ifs are very confused, try to clarify them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is out of the scope of this PR. I've reverted some changes here
Try device link first, since it is the most efficient. Report aggregated error message if device is not found. Currently we only report the confusing "virtio-" link not found.
to gain testablilty
If udev is working, we should be able to find the disk by /dev/disk/by-id link. If it is not working, we should directly query the kernel by sysfs.
7b01d3c
to
2ff265a
Compare
Return the disk/by-id path if found, it is safer if disk is detached when we are processing. The path is auto removed with the device, and cannot be reused by another disk. Avoid filepath.EvalSymlinks(). This function is complex. It resolves ALL symlinks in the path, not just the link pointed by the path. We instead verify the link by stat this path to see it is really a block special file. Extract every global dependency into instanced field for testablity. Add unit test for every function.
2ff265a
to
9a71d43
Compare
/ok-to-test |
/lgtm Please add partition disk e2e test. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: huww98, mowangdk 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
See commit message for details
Which issue(s) this PR fixes:
Fixes the confusing error message when AD controller enabled but disk not found:
Old: NodeStageVolume: ADController Enabled, but device can't be found:d-2ze08oskojgoeh7vceblvolumeID link path "/dev/disk/by-id/virtio-2ze08oskojgoeh7vcebl" not found
New: NodeStageVolume: ADController Enabled, but disk d-2ze08oskojgoeh7vceblvolume can't be found: [get by link "/dev/disk/by-id/virtio-2ze08oskojgoeh7vcebl" failed: file does not exist, get by link "/dev/disk/by-id/nvme-Alibaba_Cloud_Elastic_Block_Storage_2ze08oskojgoeh7vcebl" failed: file does not exist, find by serial: device not found]
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: