Skip to content
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

Add /dev/disk/by-id link tests #219

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

hamistao
Copy link
Contributor

@hamistao hamistao commented Jun 28, 2024

This adds tests for checking if /dev/disk/by-id links are being properly set up for different kinds of block devices. Also moves non pool specific tests away from storage-vm.
This is needed for #13672 and #13671.

@hamistao hamistao force-pushed the add_disks_by-id_tests branch from 9e0492f to c6de956 Compare June 28, 2024 18:12
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the add_disks_by-id_tests branch 5 times, most recently from 7fcfa85 to e9d054c Compare July 1, 2024 04:49
@hamistao hamistao marked this pull request as ready for review July 1, 2024 05:18
@hamistao hamistao requested a review from tomponline as a code owner July 1, 2024 05:18
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be some commits that are iterating on changes of the same lines - please dont do this. As discussed before each line change should be the final version of a logical change - not an iteration.

Secondly, we agreed on Friday that we would move the "Checking additional disk device support" tests from storage-vm to storage-disks-vm so as to keep the external disks together in one file and so as to avoid repeating them for each pool driver unnecessarily, however I do not see this change.

@tomponline
Copy link
Member

@hamistao additionally please can you also add some tests to the storage-disks-vm file that checks for devices passed with long & escaped names that are longer than what udev uses, so we can check the udev /dev/disk/by-id path is unchanged. Ta

@hamistao
Copy link
Contributor Author

hamistao commented Jul 1, 2024

@tomponline I am planning to include the discussed moving of the tests on a later PR along with other similar changes to make this simpler.
Also, using long names don't serve a lot of purpose here because the name is trimmed to about 16 characters when added to the /dev/disk/by-id path (e.g.: device-with-very-long-name becomes scsi-0QEMU_QEMU_HARDDISK_lxd_device--with--ve), if you think it still makes sense to use them I can make the changes. As for escaped names, I believe I am using one here.

Edit: Sorry I misread your message, the purpose is to go beyond the limit on udev. I am afraid this may break the tests, I am doing some investigation and will open an issue on this shortly

@hamistao hamistao force-pushed the add_disks_by-id_tests branch 12 times, most recently from 3420966 to 64847a6 Compare July 1, 2024 17:57
@hamistao
Copy link
Contributor Author

hamistao commented Jul 1, 2024

@tomponline The new long name tests were added, feel free to take a look at this when you can. The tests seem to be failing due to unrelated problems with latest/edge.

@tomponline
Copy link
Member

Thanks, unfortunately as latest/edge is now broken ill hold off on this until @mihalicyn and I can unblock it.

@tomponline
Copy link
Member

The new long name tests were added

You mentioned some devices didn't work with long names - please can you elaborate on this.

@hamistao
Copy link
Contributor Author

hamistao commented Jul 2, 2024

@tomponline It seemed like using long names could break the devices but in fact, the paths on /dev/ and /dev/disk/by-id behave differently. For example, on ubuntu-minimal VMs adding two devices with names that share a 16 character prefix, the /dev/disk/by-id path of the first would be overwritten by the second, leaving the first without a link on /dev/disk/by-id.
For the purpose of these tests, thay are not affected by this behavior.

@hamistao
Copy link
Contributor Author

hamistao commented Jul 2, 2024

Thanks, unfortunately as latest/edge is now broken ill hold off on this until @mihalicyn and I can unblock it.

No problem!

tests/storage-disks-vm Outdated Show resolved Hide resolved
@hamistao
Copy link
Contributor Author

hamistao commented Jul 2, 2024

@tomponline I will push new changes removing the tests that use readlink to check links to /dev/sd*, as was discussed earlier with @simondeziel

@tomponline
Copy link
Member

Good news, the latest/edge is unblocked and the tests are green.

Just need to understand about the restrictions being relaxed and then we can merge.

tests/storage-disks-vm Outdated Show resolved Hide resolved
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove remaining traces of /dev/sdX everywhere.

@hamistao hamistao force-pushed the add_disks_by-id_tests branch 4 times, most recently from 58a35f5 to ac2f30d Compare July 2, 2024 16:59
tests/storage-vm Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the add_disks_by-id_tests branch 2 times, most recently from 8b149fb to aa28116 Compare July 3, 2024 05:22
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-vm Outdated Show resolved Hide resolved
tests/storage-disks-vm Outdated Show resolved Hide resolved
@hamistao hamistao force-pushed the add_disks_by-id_tests branch from aa28116 to 410dbbb Compare July 3, 2024 12:34
hamistao added 5 commits July 3, 2024 10:34
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
Signed-off-by: hamistao <pedro.ribeiro@canonical.com>
@hamistao hamistao force-pushed the add_disks_by-id_tests branch from 410dbbb to 1e5aa10 Compare July 3, 2024 13:36
@hamistao
Copy link
Contributor Author

hamistao commented Jul 3, 2024

@tomponline This is ready for another look

Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tomponline tomponline merged commit 7228e88 into canonical:main Jul 3, 2024
94 checks passed
@hamistao hamistao deleted the add_disks_by-id_tests branch October 14, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants