#1897 PR `merged`: LVM: specifying uuid for 'lvmdev' is optional when recovering.¶

Labels: fixed / solved / done, minor bug

rmetrich opened issue at 2018-08-09 16:03:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

Type: Bug Fix / New Feature / Enhancement / Other? Bug Fix
Impact: Low / Normal / High / Critical / Urgent Low
Reference to related issue (URL):
How was this pull request tested? Tested on RHEL 7 during LVM migration
Brief description of the changes in this pull request:

Due to this, the grep must be adjusted to not expect a trailing blank at the end of the line.
This is typically used when migrating a PV to another disk while it already exists on the first disk. In such case, the uuid cannot be reused.

Example of disklayout.conf line before removing the uuid:

lvmdev /dev/rhel /dev/sda2 w6XFUx-DeeL-uAEY-0nEn-1A3o-pLp1-qcwYdO 12345678

When removing the uuid (and unused size), the current code was breaking if there was no trailing space:

lvmdev /dev/rhel /dev/sda2<space> : OK
lvmdev /dev/rhel /dev/sda2 : BAD

The new code also supports the second line, which is better for robustness.

jsmeix commented at 2018-08-10 09:06:¶

@rmetrich
could you explain in more detail how it happens that things go wrong
because I think I see what could go wrong here but I fail to understand
the way how things go wrong - i.e. I fail to imagine what happens here
when migrating a PV to another disk while it already exists on the first disk.

In general since https://github.com/rear/rear/issues/1871
and https://github.com/rear/rear/pull/1872#issuecomment-405842853
I learned to be extra careful when using grep -w because the actual
word separator characters in grep -w could be different compared to
what one might naively expected it to be.

I think the root cause of issues with grep -w in ReaR is that
the word separator characters in grep -w are much more than
the usually expected bash word separator characters in $IFS
that are usually assumed in ReaR.

For example how grep -w could result more matches than expected

# echo -e 'foo first this-that \nfoo second this ' | grep '^foo .* this '
foo second this 

# echo -e 'foo first this-that \nfoo second this ' | grep -w '^foo .* this'
foo first this-that 
foo second this

Simply put I like to verify that introducing grep -w here could not
lead to some more new and unexpected issues than it tries to avoid.

rmetrich commented at 2018-08-10 09:32:¶

@jsmeix the grep -w works for us here, because the 3rd field is the lvmdev device, which doesn't contain a dash which is considered as a word boundary.
Here we will have /dev/sda for example.
Initiallly the grep was done with a trailing space (e.g. /dev/sda) to avoid catching other devices starting similarly, such as /dev/sdaw. grep -w works for these cases also:

$ LAYOUT="$(echo -e "lvmdev /dev/myfirstvg /dev/sdaw\nlvmdev /dev/mysecondvg /dev/sda\nlvmdev /dev/mythirdvg /dev/sda1\nlvmdev /dev/myfirstvg /dev/sdax some-uuid somesize\nlvmdev /dev/mysecondvg /dev/sda2\n")"

$ echo "$LAYOUT"
lvmdev /dev/myfirstvg /dev/sdaw
lvmdev /dev/mysecondvg /dev/sda
lvmdev /dev/mythirdvg /dev/sda1
lvmdev /dev/myfirstvg /dev/sdax some-uuid somesize
lvmdev /dev/mysecondvg /dev/sda2

$ PVNAME="pv:/dev/sda"
$ echo "$LAYOUT" | grep -w "^lvmdev.*${PVNAME#pv:}"
lvmdev /dev/mysecondvg /dev/sda

jsmeix commented at 2018-08-10 10:08:¶

My point is that all characters that are not letters, digits, or the underscore
are word separators for grep -w which means that in 'lvmdev' entries
the device value must always contain only letters, digits, or underscore characters
to be reliably treated as one word by grep -w.

I assume traditional kernel device names like /dev/sda or /dev/sda1
are o.k. here regardless that also '/' is a word separator for grep -w
but I wonder if the device value in 'lvmdev' entries is always
a traditional kernel device name.

With grep -w things could easily break if the device value in 'lvmdev' entries
could be also a symlink name in one of the /dev/disk/by-*/ directories.

To stay on the safe side I would prefer to grep -E for the two known cases
which are "^lvmdev .* $VAR " or "^lvmdev .* $VAR\$" as far as I see.

For example like

# VAR="this"

# echo -e 'foo first this-that \nfoo second this \nfoo third this-that\nfoo fourth this' | grep -E "^foo .* $VAR |^foo .* $VAR\$"
foo second this 
foo fourth this

(plus a comment in the code why that two cases can happen ;-)

By the way:
I tried to specify ' ' and the '$' meta-character in some kind of grep character class
or bracket expression but I failed so that I falled back to using 'grep -E' with two
separated mostly same expressions.

rmetrich commented at 2018-08-10 11:10:¶

You're right, let me rework this.

jsmeix commented at 2018-08-10 13:52:¶

It seems on Friday my brain is already somewhat slow but
meanwhile even I found out how to specify it simpler in grep:

# VAR="this"

# echo -e 'foo first this-that \nfoo second this \nfoo third this-that\nfoo fourth this' | grep "^foo .* $VAR *$"
foo second this 
foo fourth this

i.e. specify the stuff after $VAR as none, one, or more spaces until the line ends.

jsmeix commented at 2018-08-13 07:46:¶

@rmetrich
a little riddle: one thing is missing.
If you don't know what, I could add it.
Hint: https://github.com/rear/rear/pull/1889
;-)

jsmeix commented at 2018-08-13 09:50:¶

@rear/contributors
please have a look at the code.
If there are no objections from you, I would like to merge it tomorrow.

jsmeix commented at 2018-08-14 09:23:¶

@rmetrich
thank you for making ReaR more fail-safe.

[Export of Github issue for rear/rear.]

#1897 PR merged: LVM: specifying uuid for 'lvmdev' is optional when recovering.¶