#1463 PR `merged`: Activate btrfs filesystem creation with original uuid¶

Labels: enhancement, fixed / solved / done

schabrolles opened issue at 2017-08-30 18:49:¶

Recent btrfs version finally add the --uuid (or -U) option
to create a btrfs fs with a specifique UUID.
It avoids changing UUID of btrfs each time we recover a system
(which is useless and can potentially bring recovery issue
when using a data backup older than the recovery image
(point-in-time restore)).

jsmeix commented at 2017-08-31 09:11:¶

@schabrolles
many thanks for this enhancement!

I think your current code works
but I also think it works in a somewhat indirect
and unexpected way - in other words: I think
it should be coded more clearly what is intended.

First thing where I stumble when looking at the code is
that "mkfs -U $uuid" is called even if $uuid is empty
(i.e. outside of the "if [ -n "$uuid" ]" test below)
but "fortunately" it works because of the
subsequent "|| mkfs" call without "-U $uuid".

Second thing where I stumble when looking at the code is
that the "# Set the UUID:" part is callled even if
the "mkfs -U $uuid" before had worked which looks
as if the UUID is set twice but "fortunately"
the "# Set the UUID:" part does not harm
because it only does something when "$uuid" != "$new_uuid"
but when the "mkfs -U $uuid" before had worked then
"$uuid" = "$new_uuid" so that in the end it is o.k.
(hopefully - if I did not overlook an exceptional case).

We have this try "mkfs -U $uuid" and if that fails
fall back to the old behaviour already for the ext*
and xfs filesystems.

Have a look how it is implemented there.

Personally I like the implementation for ext* most.

There you can - by the way - also see how one can
output multiple lines into a file and keep the indentation
in the code that outputs the multiple lines i.e. via
a sub-shell that output each line via 'echo'.

In general regarding the generated code in $LAYOUT_CODE:

The $LAYOUT_CODE (i.e. diskrestore.sh)
is run with "set -e" so that one must be extra-careful
to ensure that every command succeeds
(except an intended error exit).

For example (on commandline) what does not work

# ( set -e ; var=$( grep foo /etc/fstab ) ; echo do something )
[no output]

versus how to make that work with 'set -e'

# ( set -e ; var=$( grep foo /etc/fstab || true ) ; echo do something )
do something

schabrolles commented at 2017-08-31 11:07:¶

@jsmeix I re-write the btrfs section. Tell me if you prefer this version.

jsmeix commented at 2017-08-31 13:44:¶

I still see 'echo' with double quotes inside like

echo "          if ! grep -q "${uuid}" "$FS_UUID_MAP" ; then"
echo "              echo "$uuid \$new_uuid $device" >> $FS_UUID_MAP"
...
echo "              SED_SCRIPT=";/${uuid}/s/\${old_uuid}/\${new_uuid}/g""
echo "              sed -i "\$SED_SCRIPT" "$FS_UUID_MAP""

schabrolles commented at 2017-08-31 14:35:¶

@jsmeix should be better now.
tested with sles12

schabrolles commented at 2017-08-31 15:09:¶

@jsmeix You're welcome. thanks for your careful review.
I'm gonna wait for tomorrow to merge this one.

schabrolles commented at 2017-09-01 08:58:¶

@schlomo The use of echo was requested by @jsmeix (except if I misunderstood).

Personally I like the implementation for ext* most.

There you can - by the way - also see how one can
output multiple lines into a file and keep the indentation
in the code that outputs the multiple lines i.e. via
a sub-shell that output each line via 'echo'.

From my point of view, both have pro and cons.... I decided to apply what @jsmeix suggested to bring more "unity" or "constancy" in the code as ext filesystem part is using it.

Just let me know which one you prefer.

schlomo commented at 2017-09-01 09:50:¶

I see. I'll let you guys decide what to do as I don't have time to work on that part in any case. This is a typical question where we pay the price later when we need to visit this code again.

IMHO better indentation is much less worth than having clean code that can be pasted into another shell to try out. With the double quoting introduced by the line-by-line echo this becomes impossible. It also adds a lot of potential sources for errors due to this double quoting. I personally would therefore resort to double quoting only if no other option works.

FWIW, I had the idea about dumping functions only yesterday, otherwise I would have suggested that earlier. I also haven't tried that out in a bigger context and therefore can't tell you the problems it will cause. Most likely it works well as long as all variable parameters will be passed into the function as command line arguments and not via global shell variables.

jsmeix commented at 2017-09-01 09:56:¶

@schlomo @schabrolles
for me the reason for things like

    (   echo "    if CONDITION ; then"
        echo "        do_something"
        echo "    fi"
    ) >> "$LAYOUT_CODE"

is that this way I can reliably keep the indentation
in the code that outputs the code lines
plus the indentation in the output code lines
by using spaces which is "the only right character" ;-)
for indentations (see below).

In contrast for here documents with <<-EOF
indentation with tabs can easily get wrong
as soon as someone or something replaces tabs by spaces
because he wants to get rich with spaces for indentations
or a tool does it automatically (see below).

And it is really hard to distinguish by plain looking at the code
between the tabs indentation in the code that outputs
the code lines and the indentation with spaces in the
output code lines itself - for example as in

#!/bin/bash
if true ; then
    cat >> /tmp/heredoctestout.sh <<-EOF
        #!/bin/bash
        if true ; then
            echo Hello
        fi
        exit 0
        EOF
fi
echo wrote /tmp/heredoctestout.sh

Furthermore this way it is harder to distinguish
between the code that outputs the code lines
and the output code lines itself because both
look very similar (one has to carefully recognize
where the output code lines itself start and the 'EOF')
otherwise by "just looking" at the code it may look
as if the 'echo Hello' is run directly.

Note that here it already happened that my tabs
were automatically replaced by spaces when I
copy&pasted my original code (with tabs) hereto.

My original code is:

#!/bin/bash
if true ; then
    cat >> /tmp/heredoctestout <<-EOF
__tab__>#!/bin/bash
__tab__>if true ; then
__tab__>    echo Hello
__tab__>fi
__tab__>exit 0
__tab__>EOF
fi
echo wrote /tmp/heredoctestout

where '__tab__>' denotes a tab.

So when someone also copy&pastes the above code
from here he will not get my original code (with tabs)
but the automatically replaced stuff with all spaces.

Simply put:
Tabs for indentation just do not work reliably in practice
which is - from my point of view - the reason behind
why tabs for indentation are not in compliance with
https://github.com/rear/rear/wiki/Coding-Style

Furthermore:
Tabs for indentation prohibit we ever coud get rich:
https://stackoverflow.blog/2017/06/15/developers-use-spaces-make-money-use-tabs/

Regarding <<'EOF':
In this case I think <<'EOF' to prevent
variable replacements in here documents
cannot be used because we need both:
Some variables need to be evaluated (e.g. $fstype $uuid $device)
but others must not be evaluated (e.g. \$new_uuid \$SED_SCRIPT).

schlomo commented at 2017-09-01 10:32:¶

@jsmeix yes, I am actually very upset about Bash only supporting TABs for <<-EOF, I think that this is a very very outdated approach.

However, this still doesn't convince me that indentation is more important than keeping it simple and avoiding double quoting which I find extremely error prone.

The need to have some variables replaced now while writing the script fragment while protecting (quoting) other variables shows to me that this code doesn't have a good separation of concerns. Hence my suggestion to use functions and passing all required variables as arguments to the functions. This approach provides a very good separation of concerns and makes it very simple to test the code that is generated.

But again, please you guys decide. I don't have time to rework this code now so that those who write also decide.

jsmeix commented at 2017-09-01 10:33:¶

@schlomo
I agree that the double quoting hell with

  echo "...\"...\"..."

is ugly and bad but I also find here documents ugly and bad
so that there is a dilemma.

jsmeix commented at 2017-09-01 10:46:¶

@schlomo
"since ever" I have a very general question about
the generated code in $LAYOUT_CODE (i.e. diskrestore.sh):

Do you know the reason behind why "rear recover"
has this "another level of indirection" (cf. RFC 1925 item 6a)
by doing the disk layout recreation indirectly
via generating diskrestore.sh that is then run.

That indirection via generating diskrestore.sh
has often caused some confusion in my mind
and I always need to think complicated when
implementing code for disk layout recreation.

Why is the disk layout recreation not done
as anything else during "rear recover"
by directly running the commands (e.g. parted, mkfs, mount)
from ReaR scripts - e.g. in the same way as the bootloader
is directly reinstalled via a 620_install_grub2.sh script ?

schlomo commented at 2017-09-01 10:49:¶

I would order the different values like this:

readability in the sense of seeing at a glance what is going on. This is mostly about using forward logic, self explanatory variable names, no complex nested expressions or nested quoting, relative indentation
testability - it should be easy to copy & paste a piece of code into a (ReaR) shell to try it out
testability - it should be easy to write a unit test for a piece of code, this is mostly about good separation of concerns
readability in the sense of beauty

That is why for me a here document is the lesser evil. It creates a complete block of code that might have a different indentation. If you want you can also indent the code within the here document as it fits the surrounding code. The only downside is that the generated code is more indented than needed. Again, I find both to be the lesser evil as long as the relative indentation within a code block is correct. If different code blocks (e.g. before here document, the here document, after the here document) have different indentations then I accept that as a shortcoming of Bash.

BTW, https://stackoverflow.com/a/41154835 shows a nice hack how to pretty-print Bash code. It is similar to my suggestion to declare functions instead of here documents.

So, can you please let us know why you dislike here documents? Is this mostly personal taste or do you also have other arguments against here documents?

schlomo commented at 2017-09-01 11:01:¶

@jsmeix 😃 you ask the right question and in the end only @dagwieers and @jhoekx can answer that. You should know the history: We had a static version of that stuff before (which I wrote) which was becoming more and more impossible to extend with new feature wishes. It was very obvious back then that we won't be able to accommodate all required features with that static approach.

I never particularly liked the code as it is but I am extremely thankful to the authors for bringing ReaR to the next level through this code. IIRC the main objective of this design was to be very flexible and to give the user a review before doing the job.

I'll be very happy if we can evolve the code to be better to read and better to maintain. Maybe an approach could be to create a function for every type of line in the disklayout.conf and then to reduce the diskrestore.sh to be a list of function calls with arguments.

jsmeix commented at 2017-09-01 11:20:¶

@schlomo
no - I do not really know the history of that time - because
at that time I was not yet involved in upstream ReaR development.
I only know the old 'dr' workflow was replaced by the new 'layout'
workflow but I do not know what reasons behind had led to the
new 'layout' workflow design.

schlomo commented at 2017-09-01 12:20:¶

The new layout workflows were introduced to solve the requirements that @dagwieers and @jhoekx were hired to solve.

What do you think about the idea to introduce functions that encapsulate a topic instead of generating scripts?

jsmeix commented at 2017-09-01 12:31:¶

@schlomo
currently I do not yet know if functions instead of
directly output code is better:
On first glance I fear introducing functions to encapsulate
something could be "another level of indirection".
On the other hand introducing functions is helpful to avoid
multiple code places that implement the same thing.
Furthermore if diskrestore.sh would be a list of function calls
with arguments it could make its intent that the user can
directly edit the commands in diskrestore.sh more
complicated for the user (because of the indirection
via functions instead of the pure commands).
I think I have to meditate on it... ;-)

schlomo commented at 2017-09-01 12:34:¶

@jsmeix the indirection of functions comes to replace the indirection caused by the script generation (either as here document or as lots of echo lines with double quoting).

If the functions are properly tested then I would hope that nobody would have to change the implementation of a function during disaster recovery. Showing only the abstraction of the function calls will actually enable much more people to change stuff that seems wrong without fearing to mess up everything. For example remove a UUID parameter or something like this.

jsmeix commented at 2017-09-01 13:31:¶

@schlomo
but shouldn't the disklayout.conf file already provide
such an abstraction of the low level details?
I mean:
Wouldn't then those functions in diskrestore.sh
basically contain the values from disklayout.conf
as function parameters?

If yes, then diskrestore.sh would contain basically
the same information as disklayout.conf
so that diskrestore.sh would have no longer
an actual purpose.

In the end instead of the indirection via functions in diskrestore.sh
those reliably working functions (where nobody would have
to change their implementation during a disaster recovery)
could as well be called directly by scripts that run
during "rear recover".

Simply put:
Why an editable diskrestore.sh if all what the user is meant
to change during a disaster recovery are values like the
ones that are currently in disklayout.conf?

Or in other words:
I would much more prefer to enhance the syntax
of the disklayout.conf entries to make it much more obvious
and easier for the user to change that instead of the
low level code in diskrestore.sh.

Then a disaster recovery with issues that need to be solved
could happen like

# rear recover
Error foo

# vi disklayout.conf

# rear recover
Error bar

# vi disklayout.conf

# rear recover
[success]

schlomo commented at 2017-09-01 13:34:¶

Yes, I agree with you. Maybe it is just a question of improving the quality to remove the need for manually adjusting the diskrestore.sh. Seems like a worthwhile goal.

jsmeix commented at 2017-09-01 13:34:¶

@schabrolles
just ignore @schlomo and @jsmeix conversation here
(we terribly misuse your pull request here)
and just merge it to have a nice weekend!

jsmeix commented at 2017-09-01 13:44:¶

@schlomo
I think I have a plan how we can move
towards that goal step by step:

We can keep the diskrestore.sh code generation
in all its ugliness as is (provided it works).

We only need to to enhance the syntax of disklayout.conf
to make it much more obvious and easier for the user
to change that - plus:

We need to ensure that "rear recover" also works
on an already partially recovered system
(i.e. after a previous "rear recover" had failed).

For the latter I hope adding an early "cleanupdisk" script
https://github.com/rear/rear/issues/799
is all what is needed because when such a "cleanupdisk" script
completely wipes the disk a second "rear recover" gets again
the same clean empty disk as the first "rear recover" had.

schlomo commented at 2017-09-01 13:46:¶

Yes, that sounds good. In any case, we can start from converting a single topic to a function and see how that behaves. No need to change everything at once. Dumped functions and generated bash code are compatible 😄

jsmeix commented at 2017-09-01 13:59:¶

@schlomo
"since ages" I have the plan to enhance the disklayout.conf
entries for disks and partitions to that not only byte values
but also MiB or GiB units are supported and perhaps
even more advanced units like e.g. '8MiB' as currently
in USB_PARTITION_ALIGN_BLOCK_SIZE.
By the way:
Because older parted on SLE11 does not support MiB or GiB
(only MB and GB according to "man parted) I would convert
any values from disklayout.conf into byte values for the pated calls
in diskrestore.sh to be on the safe side (which nicely shows
that the low-level stuff may even have to be kept ugly).

[Export of Github issue for rear/rear.]

#1463 PR merged: Activate btrfs filesystem creation with original uuid¶