#1314 PR `merged`: Allowing multipath devices to be candidate disk during mapping.¶

Labels: enhancement, fixed / solved / done

schabrolles opened issue at 2017-04-17 14:28:¶

During migration, ReaR recovery propose candidate disk if they have the same (or bigger) size than the original.
The problem is that size of mulitpath device is currently not stored in layout file:

This patch propose :

To store multipath device size in $LAYOUT_FILE
Add line beginning by multipath to be part of the comparison process when searching for device with the same size or finding unmapped candidate.

gdha commented at 2017-04-18 12:59:¶

@schabrolles Is this part 1 of a pull request? Or am I missing the point here?

schabrolles commented at 2017-04-18 13:29:¶

@gdha You are right in a way. (#1309 #1315)
This pull request treat what we have to modify to have a multipathed disk proposed as a replacement disk during migration.

(before this one) You will need (#1309) to activate multipath if you migrate from non-multipathed to multipathed with BOOT_OVER_SAN=y
(after in the process) You will nedd (#1315) to properly map the old <=> new device with good multipath partition naming.

jsmeix commented at 2017-04-19 10:29:¶

@schabrolles
in layout/save/GNU/Linux/280_multipath_layout.sh
you inserted output of $dm_size in the 'multipath' lines
in $DISKLAYOUT_FILE via

echo "multipath /dev/mapper/$dm_name $dm_size ${slaves%,}" >> $DISKLAYOUT_FILE

but I miss adapted code where those 'multipath' lines are input
because now ${slaves%,} has become the third parameter
(before it was the second parameter).

As far as I see
layout/prepare/GNU/Linux/210_load_multipath.sh
does not need adaptions because it only
uses the first parameter /dev/mapper/$dm_name

As far as I see also
layout/save/default/335_remove_excluded_multipath_vgs.sh
does not need adaptions because it also only
uses the first parameter /dev/mapper/$dm_name

Now I wonder where ${slaves%,} is used at all?

If ${slaves%,} is nowhere used we should remove it.

jsmeix commented at 2017-04-19 10:39:¶

I think I found where ${slaves%,} is used: In
layout/save/default/320_autoexclude.sh
(which is run after layout/save/GNU/Linux/280_multipath_layout.sh)
as

while read multipath device slaves junk ; do
...
done < <(grep ^multipath $LAYOUT_FILE)

where now "read multipath device slaves junk"
must become "read multipath device dm_size slaves junk"

Furthermore with positional parameters
one must ensure no positional parameter is empty
so that in layout/save/GNU/Linux/280_multipath_layout.sh
plain

dm_size=$(cat /sys/block/$name/size)

is not fail-safe and must be at least

dm_size=$(cat /sys/block/$name/size)
test "$dm_size" || Error "Failed to get /sys/block/$name/size"

so that it errors out early during "rear mkbackup/mkrescue"
instead of blindly proceed and let it fail arbitrarily later
because "read multipath device dm_size slaves junk"
reads wrong parameters when dm_size is empty.

jsmeix commented at 2017-04-19 10:43:¶

Same in
layout/prepare/default/520_exclude_components.sh

while read multipath device slaves junk ; do
...
done < <(grep ^multipath "$LAYOUT_FILE")

must now be

while read multipath device dm_size slaves junk ; do

jsmeix commented at 2017-04-19 10:45:¶

FYI:
See https://github.com/rear/rear/issues/718 regarding
DISKLAYOUT_FILE versus LAYOUT_FILE inconsistency.

schabrolles commented at 2017-04-19 11:08:¶

@jsmeix Good catch... I will do this.

schabrolles commented at 2017-04-19 16:02:¶

@jsmeix Do you want me to change LAYOUT_FILE to DISKLAYOUT_FILE in the following files ?

layout/save/default/320_autoexclude.sh
layout/prepare/default/520_exclude_components.sh
layout/prepare/default/250_compare_disks.sh
layout/prepare/default/300_map_disks.sh
layout/save/GNU/Linux/280_multipath_layout.sh

schabrolles commented at 2017-04-19 16:05:¶

@jsmeix it seems that the latest changes in layout/save/default/320_autoexclude.sh
and layout/prepare/default/520_exclude_components.sh brake the recovery process when recover from (multipath) => (non-mulitpath). In the other way, it is working.
I need more time to understand what is happening here.

schabrolles commented at 2017-04-19 19:57:¶

@jsmeix, I think I found it. (9359a1a)

generate_layout_dependencies needed to be updated for multipath after the addition of dm_size parameter at field 2. (cf commit dd65bd0).
This change shifts the slave disk at field 3.

I'm gonna test this tomorrow with Sles11sp4, Sles12, RHEL 6 and RHEL 7.
single path => multipath AND multipath => single path

jsmeix commented at 2017-04-20 10:41:¶

@schabrolles
many thanks for your careful testing.
That positional parameters in LAYOUT_FILE / DISKLAYOUT_FILE
make things fragile and caused several weird looking issues
in the past where things fell apart at totally unexpected places :-(

schabrolles commented at 2017-04-20 11:21:¶

@jsmeix if I understand well, we should use $DISKLAYOUT_FILE each time we want to use /var/lib/rear/layout/disklayout.conf ... So if you want, I can start to make the changes in the files used in this pull request.

What do you think ?

jsmeix commented at 2017-04-20 13:04:¶

@schabrolles
I would very much appreciate it when one same name
is used for one same thing.

To fix the DISKLAYOUT_FILE versus LAYOUT_FILE
inconsistency please do it in a separated pull request
for https://github.com/rear/rear/issues/718
after the current pending pull requests were merged.

I fear there is perhaps some obscure reason why
different variable names are used?

I would prefer to keep separated issues separated (KSIS;-)
and not mix them up in one pull request (cf. RFC 1925 item 5).

schabrolles commented at 2017-04-21 12:08:¶

@jsmeix I've finished my test and the migration works for all of the tests.

I only have one issue regarding xfs with rhel7 when moving from multipath to not multipath .... but it is not related to this pull request. I will investigate and open another pull request/issue if needed.

[Export of Github issue for rear/rear.]

#1314 PR merged: Allowing multipath devices to be candidate disk during mapping.¶