Grab snow device path via last character rather than trimming #1546
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
|
@TerryHowe: changing LGTM is restricted to collaborators In response to this:
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. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -101,7 +101,7 @@ func (d *nodeService) findDevicePath(devicePath, volumeID, partition string) (st | |||
|
|
|||
| if util.IsSBE(d.metadata.GetRegion()) { | |||
| klog.V(5).InfoS("[Debug] Falling back to snow volume lookup", "devicePath", devicePath) | |||
| canonicalDevicePath = "/dev/vd" + strings.TrimPrefix(devicePath, "/dev/xvdb") | |||
| canonicalDevicePath = "/dev/vd" + devicePath[len(devicePath)-1:] | |||
There was a problem hiding this comment.
np: Can you add a comment here explaining why you're doing this?
There was a problem hiding this comment.
For my own understanding: the 26-volume limit doesn't exist for the current implementation, correct?
There was a problem hiding this comment.
Why both? I thought the 26-device limitation was from this change taking only the last character of devicePath while the current implementation just trims the prefix.
Due to kubernetes-sigs#1518, volumes now start with /dev/xvda rather than /dev/xvdb. Because of this, the hardcoded path in the snow code stopped working, breaking mounting on snow devices. This change instead creates the snow device path based on the last character of the generated path, rather than trying to do it via trimming, which is unreliable. Snow devices are currently limited to 10 volumes attached which makes this 100% safe, but this will need to be revisited if snow ever adds suport for >26 volumes. Signed-off-by: Connor Catlett <conncatl@amazon.com>
k8s-ci-robot
removed
the
lgtm
|
/retest Is this the new test breakage :( |
|
/retest Prow was actually broken, hopefully fixed now: kubernetes/test-infra#29167 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TerryHowe, torredil 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 |
k8s-ci-robot
added
the
approved
Is this a bug fix or adding new feature?
Bug fix
What is this PR about? / Why do we need it?
Due to #1518, volumes now start with /dev/xvda rather than /dev/xvdb. Because of this, the hardcoded path in the snow code stopped working, breaking mounting on snow devices. This change instead creates the snow device path based on the last character of the generated path, rather than trying to do it via trimming, which is unreliable.
Snow devices are currently limited to 10 volumes attached which makes this 100% safe, but this will need to be revisited if snow every adds suport for >26 volumes.
What testing is done?
Manual (with snow customer), added additional unit test case