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

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

Merged
merged 5 commits into from Aug 14, 2018

Conversation

rmetrich
Copy link
Contributor

@rmetrich rmetrich commented Aug 9, 2018

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.

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 ' : OK
'lvmdev /dev/rhel /dev/sda2': BAD

The new code also supports the second line, which is better for robustness.
@jsmeix
Copy link
Member

jsmeix commented Aug 10, 2018

@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 #1871
and #1872 (comment)
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.

@jsmeix jsmeix added the minor bug An alternative or workaround exists label Aug 10, 2018
@jsmeix jsmeix added this to the ReaR v2.5 milestone Aug 10, 2018
@jsmeix jsmeix self-assigned this Aug 10, 2018
@rmetrich
Copy link
Contributor Author

@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
Copy link
Member

jsmeix commented Aug 10, 2018

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
Copy link
Contributor Author

You're right, let me rework this.

@jsmeix
Copy link
Member

jsmeix commented Aug 10, 2018

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
Copy link
Member

jsmeix commented Aug 13, 2018

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

Copy link
Member

@jsmeix jsmeix left a comment

Choose a reason for hiding this comment

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

Thank you!

@jsmeix jsmeix requested a review from a team August 13, 2018 09:49
@jsmeix
Copy link
Member

jsmeix commented Aug 13, 2018

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

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

looks ok to me

@jsmeix jsmeix merged commit 18bd85c into rear:master Aug 14, 2018
@jsmeix
Copy link
Member

jsmeix commented Aug 14, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed / solved / done minor bug An alternative or workaround exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants