correct the diskID for device path resolution #309
Conversation
rkoster
requested review from
a team,
klakin-pivotal and
nouseforaname
and removed request for
a team
| @@ -57,7 +57,7 @@ var _ = Describe("IDDevicePathResolver", func() { | |||
| err = fs.Symlink("/dev/intermediate/fake-device-path", "/dev/disk/by-id/virtio-fake-disk-id-include-longname") | |||
| Expect(err).ToNot(HaveOccurred()) | |||
|
|
|||
| fs.SetGlob("/dev/disk/by-id/*fake-disk-id-include-longname", []string{"/dev/disk/by-id/virtio-fake-disk-id-include-longname"}) | |||
There was a problem hiding this comment.
I might just misunderstand the PR... but why did you remove fake- from here and elsewhere? Does that have anything to do with the change?
There was a problem hiding this comment.
Maybe also this can be changed to be clearer. The different cloud providers usually have a disk ID starting with some characters, then a hyphen character and then continuing with a random string. (e.g. AWS: vol-123456789ABCD). However the symlink in by-id does not have the hyphen character (for AWS (e.g. nvme-Amazon_Elastic_Block_Store_vol123456789ABCD). For Ali, everything before and including the hyphen character is missing in by-id (e.g. Ali DiskID: d-123456789ABCD; by-id: virtio-123456789ABCD). This means the device path resolution always fails atm because the Glob looks for "*vol-123456789ABCD" (for AWS) or "*d-123456789ABCD" (for Ali), but cannot find it in by-id. Therefore, we remove everything before the hyphen to make the Glob work again. In this case this means removing the "fake-" at the beginning of the mocked fake disk id. Do you think the mocked Disk ID should be changed to be clearer? Something like "fake_disk-some_random_identifier"?
klakin-pivotal
requested review from
cunnie and
klakin-pivotal
and removed request for
klakin-pivotal
|
@klakin-pivotal @nouseforaname @cunnie, |
|
Hi, can you please provide examples of the errors on ali and aws that you've seen? In general, the updates look okay but right now I cannot tell what error you've seen that creates the need for this PR. Additionally, since this only happens with specific disks I'm wondering if this should be enough reason to actually add an integration test for this.. |
|
Hi, in the linked issue the errors are described. Shall I provide some more examples? |
|
Ah nice. I think I'd be okay if the tests you already added got updated with a few real live ali / aws paths so we leave a track of how these look RN.. The reason is that I don't know if this |
fmoehler
force-pushed
the
resolution-of-device-path-by-id-fails
branch
from
1b74f65 to
4296bbb
Compare
Some IaaS provider have disk ID with prefix which is separated by a hyphen character. However the symlink doesn't have the prefix and the hyphen character that is why the resolution doesn't work in this case. This pr removes the prefix with the hyphen from the disk ID before the resolution to provide a solution for this case
fmoehler
force-pushed
the
resolution-of-device-path-by-id-fails
branch
from
4296bbb to
6636681
Compare
There was a problem hiding this comment.
Had some internal discussion and posted a follow up question here (the issue context, so lets discuss the problem a bit more, and figure out where to fix it): #310 (comment)
|
Proposed architecture, during Working group meeting is to create a configuration property for the bosh agent to configure a strip-volume-prefix-regex (or something like that), which would allow specifying a regex prefix per IaaS in the bosh stemcell builder here (in the case of Ali Cloud). |
|
Closing in favor of #311 |
For ali and aws the device path is not correctly resolved, due to some differences in the ID/Name of the disk between /dev/disk/by-id/ and the ID/Name of the disk in the IAAS provider