opensvc / multipath-tools

Other
60 stars 48 forks source link

multipathd does not trigger udev rules for a path that the user has added to the wwids file #103

Open pcahyna opened 1 week ago

pcahyna commented 1 week ago

I would like to configure multipath on a device which has only one path (/dev/sdd). (This is for testing, but I believe there may be uses for this even in production, e.g. when only one path exists for a device, but one knows that more paths will appear.)

I tried to do this via adding the device to the wwids file (multipath -a /dev/sdd) and restarting multipathd (systemctl restart multipathd), which then picks up the device and creates the multipath map (/dev/mapper/mpatha). This mostly works, but I have encountered an irregularity where udevadm info on the path does not show that it is a path:

# udevadm info /dev/sdd
E: DM_MULTIPATH_DEVICE_PATH=0

Also, there are still partition device nodes on /dev/sdd (they should have been deleted by /usr/lib/udev/rules.d/68-del-part-nodes.rules):

# lsblk
NAME                          MAJ:MIN RM   SIZE RO TYPE  MOUNTPOINT
sdd                             8:48   0 558.9G  0 disk  
├─sdd1                          8:49   0 558.9G  0 part  
└─mpatha                      253:3    0 558.9G  0 mpath 
  └─mpatha1                   253:4    0 558.9G  0 part  

This gets corrected after reboot, udevadm info shows the expected results:

# udevadm info /dev/sdd
E: DM_DEL_PART_NODES=1
E: DM_MULTIPATH_DEVICE_PATH=1
E: ID_FS_TYPE=mpath_member

I found that multipathd configured by using mpathconf --find_multipaths greedy picks up the device without using multipath -a and in this case the udev information of the path is immediately correct (without a reboot). I would prefer to have a more fine-grained control over which devices are configured as multipath, though, and this behavior does not look correct.

When examining the difference between the two cases, I found that when using greedy multipathd emits a change event for /dev/sdd which then triggers the update of the udev variables and deletion of the partition device nodes, and this does not happen when using multipath -a and then restarting multipathd. (The rule that sets DM_MULTIPATH_DEVICE_PATH is in /usr/lib/udev/rules.d/62-multipath.rules .) I then found that using udevadm trigger -w -c change /dev/sdd after restarting multipathd corrects everything.

While it is nice to have a workaround, I believe that this is a bug and the event should have been emitted by multipathd itself, because a documented use of multipath and multipathd should not lead to a situation which is apparently inconsistent.

I looked into multipath-tools code and found two locations that are most likely related to the problem: https://github.com/opensvc/multipath-tools/blob/ee3a70175a8a9045e5c309d5392300922e2a0625/libmultipath/configure.c#L940 https://github.com/opensvc/multipath-tools/blob/ee3a70175a8a9045e5c309d5392300922e2a0625/multipathd/main.c#L3087 Not sure which of them is executed, but both seem to trigger udev on the paths only if the wwid gets newly added to the wwids file, which would explain the behavior (when using multipath -a the wwid is already there, with greedy it is not - multipathd adds it). If that's the case, what is the reason for this behavior?

This is on RHEL 8 and RHEL 9 btw (multipath tools version 0.8.4 and 0.8.7).

mwilck commented 1 week ago

The classification of paths between multipath-and non-multipath depends on the find_multipaths setting.

Note that mpathconf is Fedora/RedHat specific and not part of upstream multipath tools. Please report to your distribution.

(But maybe we can short-cut your issue by having @bmarzins comment here).

pcahyna commented 1 week ago

@mwilck note that I have not mentioned any use of mpathconf in the problematic case (only in the working case). How is it relevant that mpathconf is not part of the upstream tools?

mwilck commented 1 week ago

Usage of mpathconf is an indicator for someone not using standard procedures, at least from my personal point of view.

The expected behavior is that with find_multipaths greedy, your single-path device would be picked up by multipathd, like (almost) any other block device. "greedy" is best combined with some sort of blackisting. You could do something like this in multipath.conf:

blacklist {
    devnode .*
}
blacklist_exceptions {
    devnode sdd
}

(just a very crude example, we usually discourage blacklisting by devnode).

Without "greedy", you need to add it to the WWIDs file as you figured out yourself.

pcahyna commented 4 days ago

In some cases it is more practical to execute an one-shot command than to use greedy and change multipath.conf and this is easily achieved by adding the device to the WWIDs file using multipath -a. Given that this usage is documented and the comment in the file even says

# NOTE: this file is automatically maintained by the multipath program.
# You should not need to edit this file in normal circumstances.

I would say that this usage is considered supported and should not lead to inconsistencies. And if the udev rules are not triggered and DM_MULTIPATH_DEVICE_PATH=0, this looks as an inconsistency to me.

In the meantime I found another way to configure multipath on the device using a one-shot command:

multipathd -k"add map 35000c500cbbbe1df"

This works properly and triggers the udev rules (presumably because the device is added by multipathd itself?). But as it requires me to know the WWID of the device (multipathd -k"add map /dev/sdd" does not work), using multipath -a is still the most practical way of doing this.

mwilck commented 3 days ago

0001-libmultipath-always-trigger-uevent-change-when-a-map.patch.txt

Can you try with this patch?

bmarzins commented 3 days ago

Probably the easiest way to make this work is to just run:

# multipath /dev/sdd

The fact that:

# multipathd add map /dev/sdd

doesn't work is a bug. In "multipathd add map" we assume you are passing in an identifier of a multipath device. In the multipath command, we don't make those assumptions, so multipath figures out that this is a path and you want to make a multipath device out of it. That's an easy fix.

bmarzins commented 3 days ago

@mwilck Instead of always setting needs_paths_uevent here, and then checking if it's set in domap() when a create succeeds, wouldn't it be easier to just always call trigger_paths_udev_change(mpp, true) in domap() when a create succeeds?

mwilck commented 2 days ago

Possibly. I haven't thought it through though.

Anyway we should be aware that this is not a full solution to the problem of modifying the WWIDs file asynchronously. If we handle multipath -a followed by reconfigure in this way, we should also be able to handle multipath -r, and I couldn't think of a good solution for that so far.

pcahyna commented 6 hours ago

0001-libmultipath-always-trigger-uevent-change-when-a-map.patch.txt

Can you try with this patch?

Thanks, this works in my test :+1:

pcahyna commented 6 hours ago

Probably the easiest way to make this work is to just run:

# multipath /dev/sdd

Thanks, this works even without the patch, I don't know why I thought that -a was necessary.

The fact that:

# multipathd add map /dev/sdd

doesn't work is a bug. In "multipathd add map" we assume you are passing in an identifier of a multipath device. In the multipath command, we don't make those assumptions, so multipath figures out that this is a path and you want to make a multipath device out of it. That's an easy fix.

Would be a nice extension, although the manual page describes the argument as

$map can either be a device-mapper device as listed in /sys/block (e.g. dm-0) or it can be the alias for the multipath device (e.g. mpath1) or the uid of the multipath device (e.g. 36005076303ffc56200000000000010aa).

so it currently behaves as described - I did not really expect multipathd add map /dev/sdd to work given the description.

mwilck commented 5 hours ago

Thanks, this works in my test 👍

Thank you for the feedback!

I'm not 100% convinced yet that this is the right thing to do. We'll need to do some more code review and testing to verify that we won't generate loads of additional uevents because of this change. I don't think we will, because multipathd only generates events if the state of the path in question does not match its expectations, but we must double-check, because uevent storms are a very bad thing.