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

Use BusPath to resolve dm device paths #130

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Sep 11, 2024

Problem:
If multipathd is running and has taken over a device, ResolvePersistentDevPath() will end up returning a /dev/dm-* device path, which we don't want. We want the real underyling device (e.g. /dev/sda). If we take a /dev/dm-* path and later update the blockdevice CR with it, we lose all the interesting DeviceStatus information, like the WWN.

Happily, in this case, we can figure out the right path to use by checking /dev/disk/by-path/ + the device's bus path. This has the added advantage of also working for block devices that have no WWN.

Solution:
This PR.

Note: we're not trying to make NDM support multipath devices here, we're just trying to avoid having it corrupt the blockdevice CRs if multipath happens to be active.

Related Issue:
harvester/harvester#6531

Test plan:

  • Add an extra disk that has a WWN (e.g. a scsi disk) to a harvester node
  • Start multipathd (this may require editing the grub command line to remove multipath=off)
  • Verify that the disk has been taken by multipathd:
# multipath -l
35000c50015ac3bd9 dm-0 QEMU,QEMU HARDDISK
size=20G features='0' hwhandler='0' wp=rw
`-+- policy='service-time 0' prio=0 status=active
  `- 2:0:0:0 sda 8:0 active undef running
  • Use the Harvester GUI to add the disk to the host, as described in [BUG] Unable to add new disk harvester#6531.
  • Note that it should now correctly show up as /dev/sda, not as /dev/dm-0 as in that bug report.
  • At this point, force formatting will fail, because the device has been taken by multipath. You will see an error like the following. That's fine.
    image
  • Run multipath -F to flush the multipath config. This will remove the device from multipath. It should be automatically formatted shortly afterwards.

Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

LGTM, please rebase to use the latest CI

Prior to this, if `mkfs.ext4` failed, all we saw was "failed to
format /dev/sda. err: exit status 1".  With this change, we'll
see the command output, which is a bit more useful, for exmaple:
"failed to format /dev/sda. exit status 1: mke2fs 1.46.4
(18-Aug-2021) /dev/sda is apparently in use by the system;
will not make a filesystem here!"

Signed-off-by: Tim Serong <tserong@suse.com>
If multipathd is running and has taken over a device,
ResolvePersistentDevPath() will end up returning a "/dev/dm-*"
device path, which we don't want.  We want the real underyling
device (e.g. "/dev/sda").  If we take a "/dev/dm-*" path and
later update the blockdevice CR with it, we lose all the
interesting DeviceStatus information, like the WWN.

Happily, in this case, we can figure out the right path to
use by checking "/dev/disk/by-path/" + the device's bus path.
This has the added advantage of also working for block devices
that have no WWN.

Related issue: harvester/harvester#6531

Signed-off-by: Tim Serong <tserong@suse.com>
@tserong
Copy link
Contributor Author

tserong commented Sep 24, 2024

Rebased

@Vicente-Cheng
Copy link
Collaborator

@Mergifyio backport v0.7.x

Copy link

mergify bot commented Sep 24, 2024

backport v0.7.x

✅ Backports have been created

Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

thanks!

@bk201 bk201 merged commit cefcc3d into harvester:master Sep 25, 2024
6 checks passed
@tserong tserong deleted the wip-fix-dm-paths branch September 25, 2024 04:10
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