6:39 a.m.
"make run-virt-p2v-in-a-vm" and "make run-virt-p2v-in-an-nvme-vm"
suffer
from two problems: (a) possibly duplicate XFS UUIDs between the
$PHYSICAL_MACHINE disk and the "virt-p2v.img" boot media, (b) slow guest
(= fake "physical machine") boot. This series fixes these issues.
Tested with actual conversions (libvirt output) and post-conversion
booting.
Laszlo
Laszlo Ersek (4):
Makefile.am: factor out "make-physical-machine.sh"
make-physical-machine.sh: set root password to "p2v-phys"
make-physical-machine.sh: regenerate filesystem UUIDs
Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
Makefile.am | 9 +-
.gitignore | 1 +
make-physical-machine.sh | 117 ++++++++++++++++++++
3 files changed, 124 insertions(+), 3 deletions(-)
create mode 100755 make-physical-machine.sh
6:39 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
Extract and somewhat generalize the recipe for the $(PHYSICAL_MACHINE)
target to a separate shell script. In preparation for the multiple steps
we're going to introduce later, redirect virt-builder to a temp file at
first (placed in the same directory as the finally expected disk image),
and rename that file upon success.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
Makefile.am | 5 ++-
.gitignore | 1 +
make-physical-machine.sh | 37 ++++++++++++++++++++
3 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/Makefile.am b/Makefile.am
index d94ac6fdaf94..f25de74b8690 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -41,6 +41,7 @@ EXTRA_DIST = \
kiwi-config.xml.in \
launch-virt-p2v \
libguestfs/README \
+ make-physical-machine.sh \
miniexpect/README \
p2v.ks.in \
p2v.service \
@@ -66,6 +67,7 @@ CLEANFILES += \
$(generated_sources) \
$(PHYSICAL_MACHINE) $(BLANK_DISK) \
about-authors.c \
+ physical-machine.tmp.* \
stamp-test-virt-p2v-pxe-data-files \
stamp-test-virt-p2v-pxe-kernel \
test-virt-p2v-pxe.authorized_keys \
@@ -255,6 +257,7 @@ if HAVE_LIBGUESTFS
check_DATA += \
$(PHYSICAL_MACHINE) \
$(BLANK_DISK)
+check_SCRIPTS = make-physical-machine.sh
endif HAVE_LIBGUESTFS
run-virt-p2v-directly: $(PHYSICAL_MACHINE)
@@ -317,7 +320,7 @@ run-virt-p2v-non-gui-conversion: $(PHYSICAL_MACHINE)
stamp-test-virt-p2v-pxe-dat
SLOW=1 $(top_builddir)/run ./test-virt-p2v-pxe.sh
$(PHYSICAL_MACHINE):
- $(top_builddir)/run virt-builder --format raw -o $@ fedora-35
+ $(top_builddir)/run ./make-physical-machine.sh $@
$(BLANK_DISK):
$(top_builddir)/run guestfish -N $@=part exit
diff --git a/.gitignore b/.gitignore
index a870e2b6186b..eea5128411c2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -41,6 +41,7 @@ Makefile.in
/m4/ltsugar.m4
/m4/ltversion.m4
/p2v-config.h
+/physical-machine.tmp.*
/podwrapper.pl
/run
/stamp-h1
diff --git a/make-physical-machine.sh b/make-physical-machine.sh
new file mode 100755
index 000000000000..fb42cda34fca
--- /dev/null
+++ b/make-physical-machine.sh
@@ -0,0 +1,37 @@
+#!/bin/bash -
+# Copyright (C) 2022 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <https://www.gnu.org/licenses/>;.
+
+set -e -u -C
+
+disk=
+
+cleanup()
+{
+ set +e
+ if test -n "$disk"; then
+ rm -f -- "$disk"
+ disk=
+ fi
+}
+
+trap cleanup EXIT
+
+output=$1
+outdir=$(dirname -- "$output")
+disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
+virt-builder --format raw -o "$disk" fedora-35
+mv -- "$disk" "$output"
+disk=
7:39 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Fri, Aug 26, 2022 at 01:39:05PM +0200, Laszlo Ersek wrote:
Extract and somewhat generalize the recipe for the
$(PHYSICAL_MACHINE)
target to a separate shell script. In preparation for the multiple steps
we're going to introduce later, redirect virt-builder to a temp file at
first (placed in the same directory as the finally expected disk image),
and rename that file upon success.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
+++ b/make-physical-machine.sh
@@ -0,0 +1,37 @@
+#!/bin/bash -
See the response in the other thread about not needing the - here.
Are we sure that /bin/bash is on all systems where this script will be
run, or is it better as '#!/bin/env bash'?
+
+set -e -u -C
My personal opinion is that 'set -e' is a crutch that should be
avoided because of its unintended interaction with functions; but I'm
not adamant enough about it to tell people to rip it out of scripts.
For short scripts, like this one, it's easy enough to check that we
aren't tripping over any of -e's surprises.
+
+disk=
+
+cleanup()
+{
+ set +e
+ if test -n "$disk"; then
+ rm -f -- "$disk"
+ disk=
+ fi
+}
+
+trap cleanup EXIT
Is it intentional that you are not also cleaning up on signals like
INT and HUP?
+
+output=$1
+outdir=$(dirname -- "$output")
+disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
+virt-builder --format raw -o "$disk" fedora-35
+mv -- "$disk" "$output"
+disk=
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
12:28 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On 08/26/22 14:39, Eric Blake wrote:
On Fri, Aug 26, 2022 at 01:39:05PM +0200, Laszlo Ersek wrote:
> Extract and somewhat generalize the recipe for the $(PHYSICAL_MACHINE)
> target to a separate shell script. In preparation for the multiple steps
> we're going to introduce later, redirect virt-builder to a temp file at
> first (placed in the same directory as the finally expected disk image),
> and rename that file upon success.
>
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
> +++ b/make-physical-machine.sh
> @@ -0,0 +1,37 @@
> +#!/bin/bash -
See the response in the other thread about not needing the - here.
Are we sure that /bin/bash is on all systems where this script will be
run, or is it better as '#!/bin/env bash'?
I had grepped the virt-p2v and virt-v2v shell scripts for shebangs:
1 #!/bin/sh
2 #!/bin/bash
4 #!/usr/bin/env perl
17 #!/bin/bash -
1 #!/bin/bash
1 #!/usr/bin/env python3
1 #!/usr/bin/sh
5 #!/usr/bin/env perl
67 #!/bin/bash -
The thinking seems to have been that
- python3 and perl may "move",
- bash is considered always available.
So I wouldn't break consistency with "#!/bin/bash". I did feel like
breaking consistency with the hyphen "#!/bin/bash -" -- in that,
minimally, I would write "#!/bin/bash --".
Anyway, as a temporary approach, I just stuck with the tradition (the
hyphen), and ended up posting the patch like that. It's not "wrong"
technically, just strange to my taste. I no longer feel strongly about
it (but I'm happy to remove the hyphen if that's desired).
> +
> +set -e -u -C
My personal opinion is that 'set -e' is a crutch that should be
avoided because of its unintended interaction with functions;
Can you please elaborate?
POSIX writes, "When a function is executed, it shall have the
syntax-error and variable-assignment properties described for special
built-in utilities in the enumerated list at the beginning of Special
Built-In Utilities", and I've checked that list -- I don't know what you
mean.
but I'm
not adamant enough about it to tell people to rip it out of scripts.
For short scripts, like this one, it's easy enough to check that we
aren't tripping over any of -e's surprises.
> +
> +disk=
> +
> +cleanup()
> +{
> + set +e
> + if test -n "$disk"; then
> + rm -f -- "$disk"
> + disk=
> + fi
> +}
> +
> +trap cleanup EXIT
Is it intentional that you are not also cleaning up on signals like
INT and HUP?
Yes, as EXIT covers those.
Easy to test with:
------
#!/bin/bash
cleanup()
{
echo done >zzz
}
trap cleanup EXIT
sleep 100
------
rm -f zzz
./hello.sh
and then hitting either ^C or Alt-F4 in/on the terminal window.
It's for a similar reason I didn't specify ERR -- ERR applies roughly
under the same conditions where "set -e" causes the shell to exit, so
"set -e" causes (in effect) the EXIT trap handler to cover ERR too.
Laszlo
> +
> +output=$1
> +outdir=$(dirname -- "$output")
> +disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
> +virt-builder --format raw -o "$disk" fedora-35
> +mv -- "$disk" "$output"
> +disk=
2:34 p.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Tue, Aug 30, 2022 at 07:28:43AM +0200, Laszlo Ersek wrote:
I had grepped the virt-p2v and virt-v2v shell scripts for shebangs:
1 #!/bin/sh
2 #!/bin/bash
4 #!/usr/bin/env perl
17 #!/bin/bash -
1 #!/bin/bash
1 #!/usr/bin/env python3
1 #!/usr/bin/sh
5 #!/usr/bin/env perl
67 #!/bin/bash -
The thinking seems to have been that
- python3 and perl may "move",
- bash is considered always available.
For p2v, yeah, we are pretty much guaranteed to be running on Linux,
and therefore have bash (even if /bin/sh is not bash).
>
>> +
>> +set -e -u -C
>
> My personal opinion is that 'set -e' is a crutch that should be
> avoided because of its unintended interaction with functions;
Can you please elaborate?
POSIX writes, "When a function is executed, it shall have the
syntax-error and variable-assignment properties described for special
built-in utilities in the enumerated list at the beginning of Special
Built-In Utilities", and I've checked that list -- I don't know what you
mean.
Here's a good writeup:
http://mywiki.wooledge.org/BashFAQ/105
One of the simplest demonstrations on that page:
$ bash -c 'set -e; f() { set -e; false; echo huh; }; f; echo survived'
$ bash -c 'set -e; f() { set -e; false; echo huh; }; f && echo survived'
huh
survived
That is, using 'set -e' to end function f early "works", but ONLY in
situations where f itself is not invoked in a context where 'set -e'
is suppressed because of a conditional in an outer scope. And once
'if' or '&&' suppresses 'set -e', you cannot re-enable it
within f
itself. Context-sensitive behavior of your function body based on the
context of the caller is NOT intuitive.
That said, if you KNOW that -e behaves non-intuitively, and plan to
write your script with that in mind, it can be helpful. The complaint
is that because -e is disabled in so many situations, it is harder to
prove that -e catches all the scenarios that you WANTED to be caught
than it is to just write the error handling yourself.
> but I'm
> not adamant enough about it to tell people to rip it out of scripts.
> For short scripts, like this one, it's easy enough to check that we
> aren't tripping over any of -e's surprises.
>
>> +
>> +disk=
>> +
>> +cleanup()
>> +{
>> + set +e
>> + if test -n "$disk"; then
>> + rm -f -- "$disk"
>> + disk=
>> + fi
>> +}
>> +
>> +trap cleanup EXIT
>
> Is it intentional that you are not also cleaning up on signals like
> INT and HUP?
Yes, as EXIT covers those.
Okay; currently true enough for bash, but not portable to other
shells. In fact, the Austin Group visited that topic just this week:
https://austingroupbugs.net/view.php?id=621
After 2018 edition page 2420 line 77499 section 2.14 trap, add a new
paragraph:
The EXIT condition shall occur when the shell terminates normally
(exits), and may occur when the shell terminates abnormally as a
result of delivery of a signal (other than SIGKILL) whose trap
action is the default.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
1:59 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On 08/30/22 21:34, Eric Blake wrote:
On Tue, Aug 30, 2022 at 07:28:43AM +0200, Laszlo Ersek wrote:
> I had grepped the virt-p2v and virt-v2v shell scripts for shebangs:
>
> 1 #!/bin/sh
> 2 #!/bin/bash
> 4 #!/usr/bin/env perl
> 17 #!/bin/bash -
>
> 1 #!/bin/bash
> 1 #!/usr/bin/env python3
> 1 #!/usr/bin/sh
> 5 #!/usr/bin/env perl
> 67 #!/bin/bash -
>
> The thinking seems to have been that
> - python3 and perl may "move",
> - bash is considered always available.
For p2v, yeah, we are pretty much guaranteed to be running on Linux,
and therefore have bash (even if /bin/sh is not bash).
>>
>>> +
>>> +set -e -u -C
>>
>> My personal opinion is that 'set -e' is a crutch that should be
>> avoided because of its unintended interaction with functions;
>
> Can you please elaborate?
>
> POSIX writes, "When a function is executed, it shall have the
> syntax-error and variable-assignment properties described for special
> built-in utilities in the enumerated list at the beginning of Special
> Built-In Utilities", and I've checked that list -- I don't know what
you
> mean.
Here's a good writeup:
http://mywiki.wooledge.org/BashFAQ/105
(Some of the points this page makes fall in the category "utilities may
have unexpected exit statuses" -- but that isn't a problem only for such
scripts that use "set -e", but also for such scripts that meticulously
check the explicit exit status of each command.)
One of the simplest demonstrations on that page:
$ bash -c 'set -e; f() { set -e; false; echo huh; }; f; echo survived'
$ bash -c 'set -e; f() { set -e; false; echo huh; }; f && echo survived'
huh
survived
That is, using 'set -e' to end function f early "works", but ONLY in
situations where f itself is not invoked in a context where 'set -e'
is suppressed because of a conditional in an outer scope. And once
'if' or '&&' suppresses 'set -e', you cannot re-enable it
within f
itself. Context-sensitive behavior of your function body based on the
context of the caller is NOT intuitive.
That said, if you KNOW that -e behaves non-intuitively, and plan to
write your script with that in mind, it can be helpful. The complaint
is that because -e is disabled in so many situations, it is harder to
prove that -e catches all the scenarios that you WANTED to be caught
than it is to just write the error handling yourself.
I agree this behavior is not intuitive. It does not seem to follow from
the POSIX language
The -e setting shall be ignored when executing the compound list
following the while, until, if, or elif reserved word, a pipeline
beginning with the ! reserved word, or any command of an AND-OR list
other than the last.
(which is the language I found "least distantly related" to this
behavior). Such "context rules" are frequent in other languages too, but
they only ever apply directly, and not recursively -- sub-contexts tend
to have their own separate environments.
I've checked the subject script now and it does not seem to suffer from
this "set -e" pitfall; thus, I'm going to merge it.
Now, whether this kills "set -e" for me for good... I'm not so sure.
I'm
trying to think up a shell function that I would want to (a) call from
an outer conditional context, and at the same time (b) cause the whole
script to abort due to an internal error.
I'm coming up empty here: those goals look mutually exclusive. Here's
why I think so: whether the exit status ("return value") of a function
matters or not is part of the function's specification; i.e., design. If
I design a function such that it return a meaningful value, I *already*
cannot allow any errors to go uncaught in the function body, and I
*also* cannot allow the function to kill the outer context due to any
internal problems. Conversely, if I only need a simple code extraction
from the outer, larger context, I will certainly rely on internal errors
in the function to abort the whole script -- but then I will have *zero
reason* to invoke the function from within an outer conditional.
This is why I think that, although I've been using "set -e" for years
(decades?), I may not have written a single script plagued by this
particular misbehavior. Not because I'm that clever, but because (I
suspect) the situation demonstrated above "almost never" occurs in practice.
So while I'm very surprised by the above demonstration, I'm quite
tempted to believe that the "set -e" masking behavior, albeit not
intuitive, is correct (and that at least I personally can continue using
"set -e", while keeping this non-intuitive behavior in mind).
Either way: what would be an alternative to "set -e" that:
(1) scaled (in the sense that it does not mangle the whole script to
unreadability),
(2) did not introduce the Arrow anti-pattern due to deeply nested "if"s
<https://blog.codinghorror.com/flattening-arrow-code/>;,
<http://wiki.c2.com/?ArrowAntiPattern>?
Would we have to write code like
foo=$(somecommand ...)
ret=$?
if [ $ret -ne 0 ]; then
exit $ret
fi
some_other_command -- "$foo"
ret=$?
if [ $ret -ne 0 ]; then
exit $ret
fi
?
(That's what I mean by "scaling".)
>
>> but I'm
>> not adamant enough about it to tell people to rip it out of scripts.
>> For short scripts, like this one, it's easy enough to check that we
>> aren't tripping over any of -e's surprises.
>>
>>> +
>>> +disk=
>>> +
>>> +cleanup()
>>> +{
>>> + set +e
>>> + if test -n "$disk"; then
>>> + rm -f -- "$disk"
>>> + disk=
>>> + fi
>>> +}
>>> +
>>> +trap cleanup EXIT
>>
>> Is it intentional that you are not also cleaning up on signals like
>> INT and HUP?
>
> Yes, as EXIT covers those.
Okay; currently true enough for bash, but not portable to other
shells. In fact, the Austin Group visited that topic just this week:
https://austingroupbugs.net/view.php?id=621
After 2018 edition page 2420 line 77499 section 2.14 trap, add a new
paragraph:
The EXIT condition shall occur when the shell terminates normally
(exits), and may occur when the shell terminates abnormally as a
result of delivery of a signal (other than SIGKILL) whose trap
action is the default.
Thanks!
Laszlo
7:20 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Wed, Aug 31, 2022 at 08:59:15AM +0200, Laszlo Ersek wrote:
Now, whether this kills "set -e" for me for good... I'm
not so sure. I'm
trying to think up a shell function that I would want to (a) call from
an outer conditional context, and at the same time (b) cause the whole
script to abort due to an internal error.
I'm going to add it to the large and growing pile of "gotchas to be
aware of when writing shell scripts", but I also don't think it
invalidates using set -e in general.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
10:26 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Wed, Aug 31, 2022 at 08:59:15AM +0200, Laszlo Ersek wrote:
> One of the simplest demonstrations on that page:
>
> $ bash -c 'set -e; f() { set -e; false; echo huh; }; f; echo survived'
> $ bash -c 'set -e; f() { set -e; false; echo huh; }; f && echo
survived'
> huh
> survived
>
I agree this behavior is not intuitive. It does not seem to follow from
the POSIX language
The -e setting shall be ignored when executing the compound list
following the while, until, if, or elif reserved word, a pipeline
beginning with the ! reserved word, or any command of an AND-OR list
other than the last.
(which is the language I found "least distantly related" to this
behavior). Such "context rules" are frequent in other languages too, but
they only ever apply directly, and not recursively -- sub-contexts tend
to have their own separate environments.
Actually, this is exactly the POSIX language that describes the
behavior above. As soon as you invoke 'f && ...', f is now on the
left-hand side of an AND-OR list, and the entire body of f is invoked
in a scenario where you cannot re-enable 'set -e' no matter how hard
you try.
I've checked the subject script now and it does not seem to suffer from
this "set -e" pitfall; thus, I'm going to merge it.
Now, whether this kills "set -e" for me for good... I'm not so sure.
I'm
trying to think up a shell function that I would want to (a) call from
an outer conditional context, and at the same time (b) cause the whole
script to abort due to an internal error.
I'm coming up empty here: those goals look mutually exclusive. Here's
why I think so: whether the exit status ("return value") of a function
matters or not is part of the function's specification; i.e., design. If
I design a function such that it return a meaningful value, I *already*
cannot allow any errors to go uncaught in the function body, and I
*also* cannot allow the function to kill the outer context due to any
internal problems. Conversely, if I only need a simple code extraction
from the outer, larger context, I will certainly rely on internal errors
in the function to abort the whole script -- but then I will have *zero
reason* to invoke the function from within an outer conditional.
Yes, this is the counter-argument for why some people use 'set -e' in
a shell script with functions, despite the potential for pitfall. The
rule of thumb for such a script becomes: never invoke a function in a
conditional if the function was not careful about handling errors
without reliance on 'set -e'; or more generally, write all functions
to assume that 'set -e' is a no-op. At which point, 'set -e' is only
useful for the top-level code outside of functions; the more your
script relies on functions instead of top-level code, the less likely
'set -e' is something you want to use.
This is why I think that, although I've been using "set -e" for years
(decades?), I may not have written a single script plagued by this
particular misbehavior. Not because I'm that clever, but because (I
suspect) the situation demonstrated above "almost never" occurs in practice.
So while I'm very surprised by the above demonstration, I'm quite
tempted to believe that the "set -e" masking behavior, albeit not
intuitive, is correct (and that at least I personally can continue using
"set -e", while keeping this non-intuitive behavior in mind).
Whether it is "intuitive" or "correct" may be a matter of
interpretation; but at the end of the day, POSIX standardized what
existing practice does ('set -e' being disabled on the left side of &&
was historical practice even before shell functions were introduced),
rather than what would be sane if the shell language were being
developed from scratch.
Either way: what would be an alternative to "set -e" that:
(1) scaled (in the sense that it does not mangle the whole script to
unreadability),
(2) did not introduce the Arrow anti-pattern due to deeply nested "if"s
<https://blog.codinghorror.com/flattening-arrow-code/>;,
<http://wiki.c2.com/?ArrowAntiPattern>?
Would we have to write code like
foo=$(somecommand ...)
ret=$?
if [ $ret -ne 0 ]; then
exit $ret
fi
some_other_command -- "$foo"
ret=$?
if [ $ret -ne 0 ]; then
exit $ret
fi
You can trim it down to something more legible:
foo=$(somecommand ...) || fatal
some_other_command -- "$foo" || fatal
where you write a helper function that encapulates the repetitive
nature of invoking the command and checking for expected exit status.
That particular style is how GNU coreutils writes much of its
testsuite; picking a random example:
https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=tests/ls/a-option.sh
But yeah, converting a script that relied on 'set -e' to one that is
equally safe without requires a framework of helper functions and a
whole-script audit, which is not as scalable as designing that way
from the outset.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
8:07 a.m.
New subject: [p2v PATCH 1/4] Makefile.am: factor out "make-physical-machine.sh"
On Fri, Aug 26, 2022 at 07:39:31AM -0500, Eric Blake wrote:
On Fri, Aug 26, 2022 at 01:39:05PM +0200, Laszlo Ersek wrote:
> Extract and somewhat generalize the recipe for the $(PHYSICAL_MACHINE)
> target to a separate shell script. In preparation for the multiple steps
> we're going to introduce later, redirect virt-builder to a temp file at
> first (placed in the same directory as the finally expected disk image),
> and rename that file upon success.
>
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
> +++ b/make-physical-machine.sh
> @@ -0,0 +1,37 @@
> +#!/bin/bash -
See the response in the other thread about not needing the - here.
Are we sure that /bin/bash is on all systems where this script will be
run, or is it better as '#!/bin/env bash'?
Realisticly virt-p2v only makes sense on Linux, both when used to
prepare the P2V ISO, as well as the environment in which the ISO
boots.
eg: Using virt-p2v on (say) FreeBSD to prepare a Linux ISO? The
FreeBSD folk would prefer that the built ISO contained FreeBSD, but
that would require a small mountain of work.
> +
> +set -e -u -C
My personal opinion is that 'set -e' is a crutch that should be
avoided because of its unintended interaction with functions; but I'm
not adamant enough about it to tell people to rip it out of scripts.
For short scripts, like this one, it's easy enough to check that we
aren't tripping over any of -e's surprises.
I'm confused too: what's the problem with set -e? Compared to _not_
having set -e which allows scripts to continue running after errors.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit
6:39 a.m.
New subject: [p2v PATCH 2/4] make-physical-machine.sh: set root password to "p2v-phys"
It helps with testing if we set the fake physical machine's password to a
simple known string, rather than letting virt-builder generate a complex,
random password.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
make-physical-machine.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/make-physical-machine.sh b/make-physical-machine.sh
index fb42cda34fca..ff6db4940b51 100755
--- a/make-physical-machine.sh
+++ b/make-physical-machine.sh
@@ -32,6 +32,6 @@ trap cleanup EXIT
output=$1
outdir=$(dirname -- "$output")
disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
-virt-builder --format raw -o "$disk" fedora-35
+virt-builder --format raw -o "$disk" --root-password password:p2v-phys
fedora-35
mv -- "$disk" "$output"
disk=
6:39 a.m.
New subject: [p2v PATCH 3/4] make-physical-machine.sh: regenerate filesystem UUIDs
Some test targets in "Makefile.am", such as "run-virt-p2v-in-a-vm"
and
"run-virt-p2v-in-an-nvme-vm", use both the PHYSICAL_MACHINE disk (as the
conversion subject) and "virt-p2v.img" (as boot media).
Unless the user overrides the PHYSICAL_MACHINE macro, we generate it with
virt-builder, using a distro that we open-code (currently: Fedora 35).
"virt-p2v.img" is also generated with virt-builder, but the distro
installed in it follows the user's host OS (therefore we have less control
over the distro choice in "virt-p2v.img").
The user's host OS may happen to match the OS we hardcode for
PHYSICAL_MACHINE (currently: Fedora 35), in which case we end up using the
same virt-builder template for both images. This is a problem: the
filesystems between both images will have identical UUIDs, which may cause
the kernel loaded from "virt-p2v.img" to mount the root filesystem from
PHYSICAL_MACHINE.
Prevent this by regenerating the filesystem UUIDs in PHYSICAL_MACHINE
(whose distro we closely control, unless the user sets PHYSICAL_MACHINE
themselves). Instead of the virt-sysprep operation "fs-uuids", which
currently cannot update the filesystem UUID references in the guest's
config files (<https://libguestfs.org/virt-sysprep.1.html#fs-uuids>;,
<https://bugzilla.redhat.com/show_bug.cgi?id=991641>), implement a
somewhat crude guest-side shell command for the same purpose. Such a
command would likely not be flexible / robust enough for "fs-uuids" in
virt-sysprep, but -- due to our tight control over the PHYSICAL_MACHINE
operating system in virt-p2v -- it should suffice for virt-p2v testing.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
make-physical-machine.sh | 82 +++++++++++++++++++-
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/make-physical-machine.sh b/make-physical-machine.sh
index ff6db4940b51..946d59f0c30b 100755
--- a/make-physical-machine.sh
+++ b/make-physical-machine.sh
@@ -16,11 +16,16 @@
set -e -u -C
+GUESTFISH_PID=
disk=
cleanup()
{
set +e
+ if test -n "$GUESTFISH_PID"; then
+ guestfish --remote -- exit >/dev/null 2>&1
+ GUESTFISH_PID=
+ fi
if test -n "$disk"; then
rm -f -- "$disk"
disk=
@@ -32,6 +37,81 @@ trap cleanup EXIT
output=$1
outdir=$(dirname -- "$output")
disk=$(mktemp -p "$outdir" physical-machine.tmp.XXXXXXXXXX)
-virt-builder --format raw -o "$disk" --root-password password:p2v-phys
fedora-35
+# Delay the SELinux relabeling.
+virt-builder --format raw -o "$disk" --root-password password:p2v-phys \
+ --no-selinux-relabel fedora-35
+
+# Start a guestfish server on the disk image, so that each of the several
+# UUID-manipulation commands below not need a separate guestfish launch.
+guestfish_set_pid=$(guestfish --listen --format=raw --add "$disk")
+eval "$guestfish_set_pid"
+guestfish --remote -- run
+
+# The array below open-codes the partition:filesystem layout of the virt-builder
+# disk template used above. Whenever you bump the Fedora version, update the
+# array below as needed.
+#
+# We cannot use inspection either before or after the filesystem UUID changes:
+#
+# - the UUID of a mounted filesystem cannot be changed (at least with XFS);
+#
+# - right after the UUID changes, inspection does not work correctly, as the new
+# fs UUIDs are out of sync with the UUID references in those config files that
+# inspection relies upon.
+#
+# Note that the order of entries is important too: the mount points must be
+# listed in dependency order (put the dependees first, the dependants last).
+fsdevs=(/dev/sda3:/ /dev/sda2:/boot)
+
+# For each filesystem:
+#
+# - regenerate the UUID,
+#
+# - produce a sed command (scriptlet) that performs the same UUID replacement in
+# a text file,
+#
+# - mount the filesystem.
+#
+# Note that later we're going to rely on the fact that the generated sed
+# commands *require no quoting* on the shell command line.
+declare -a sed_script
+sed_idx=0
+for fsdev in "${fsdevs[@]}"; do
+ device=${fsdev%:*}
+ mountpoint=${fsdev#*:}
+
+ old_uuid=$(guestfish --remote -- get-uuid "$device")
+ guestfish --remote -- set-uuid-random "$device"
+ new_uuid=$(guestfish --remote -- get-uuid "$device")
+
+ sed_script[sed_idx++]=-e
+ sed_script[sed_idx++]=s/$old_uuid/$new_uuid/ig
+
+ guestfish --remote -- mount "$device" "$mountpoint"
+done
+
+# Prepare the UUID replacement shell command for the appliance.
+refresh_uuid_refs=(find /boot /etc -type f -print0 '|'
+ xargs -0 -r -- sed -i "${sed_script[@]}" --)
+
+# Passing the shell command to the appliance is where we rely on the fact that
+# the sed commands for replacing UUIDs require no quoting.
+guestfish --remote -- sh "${refresh_uuid_refs[*]}"
+
+# Tear down the guestfish server before we use virt-customize.
+waitpid=$GUESTFISH_PID
+guestfish --remote -- exit
+GUESTFISH_PID=
+while kill -s 0 -- "$waitpid" 2>/dev/null; do
+ sleep 1
+done
+
+# Reapply the SELinux labels now. Use virt-customize for this, rather than
+# guestfish's "selinux-relabel", as virt-customize contains some heavy
logic
+# related to "specfile". Inspection does work here, because the config files
are
+# in sync again with the filesystem UUIDs.
+virt-customize --format raw --add "$disk" --selinux-relabel
+
+# We're done; rename the temporary disk image to the expected output file.
mv -- "$disk" "$output"
disk=
6:39 a.m.
New subject: [p2v PATCH 4/4] Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
The "piix3-usb-uhci" QEMU device emulates a USB1 controller. Not only does
USB1 feature amazingly low transfer speeds -- which QEMU appears to
emulate faithfully --, USB1 (and USB2 too) are distinctly
virtualization-unfriendly, as the emulated controllers require constant
polling.
This is visible in two symptoms: (a) the "run-virt-p2v-in-a-vm" and
"run-virt-p2v-in-an-nvme-vm" targets load data (such as the root
filesystem) from the "virt-p2v.img" file very slowly, while the host CPU
utilization of QEMU never exceeds ~30%, (b) once the virt-p2v application
launches in the guest, and the cursor is blinking in the "Conversion
server" text field, the host CPU utilization does not sink under ~12%
(while "top" in the guest XTerm reports 99+% idleness).
Fix this problem by using the qemu-xhci USB3 controller. USB3 both enables
much higher transfer speeds and is event-driven, requiring no polling.
Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
---
Makefile.am | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index f25de74b8690..f2fe0e3efc99 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -270,7 +270,7 @@ run-virt-p2v-in-a-vm: virt-p2v.img $(PHYSICAL_MACHINE)
-cpu host \
-m 2048 \
-drive id=hd0,file=$(PHYSICAL_MACHINE),format=raw,if=ide \
- -device piix3-usb-uhci \
+ -device qemu-xhci \
-drive id=usb0,file=$<,format=raw,snapshot=on,if=none \
-device usb-storage,bootindex=1,drive=usb0 \
-boot menu=on \
@@ -294,7 +294,7 @@ run-virt-p2v-in-an-nvme-vm: virt-p2v.img $(PHYSICAL_MACHINE)
$(BLANK_DISK)
-device nvme,drive=phys,serial=01234567 \
\
-drive id=usb0,if=none,format=raw,file=$<,snapshot=on \
- -device piix3-usb-uhci,id=usb-bus \
+ -device qemu-xhci,id=usb-bus \
-device usb-storage,drive=usb0,bootindex=1,bus=usb-bus.0 \
\
-drive id=blank1,if=none,format=raw,file=$(BLANK_DISK),snapshot=on \
8:11 a.m.
New subject: [p2v PATCH 4/4] Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
Series looks good, thanks.
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
8:34 a.m.
New subject: [p2v PATCH 4/4] Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
On 08/30/22 15:11, Richard W.M. Jones wrote:
Series looks good, thanks.
Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Thanks :)
Let me wait for Eric's next round of comments / answers though, before I
merge the series.
Laszlo
2:53 p.m.
New subject: [p2v PATCH 4/4] Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
On Tue, Aug 30, 2022 at 03:34:46PM +0200, Laszlo Ersek wrote:
On 08/30/22 15:11, Richard W.M. Jones wrote:
>
> Series looks good, thanks.
>
> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
Thanks :)
Let me wait for Eric's next round of comments / answers though, before I
merge the series.
I think I've answered everything pending on the 'set -e' and
'#!/bin/bash -' questions. I'm not opposed to them going in as-is,
since it matches existing usage, and as long as we are aware of the
potential for pitfalls. Beyond that, I don't think I had any other
comments, so I'm okay if you are ready to merge.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
2:12 a.m.
New subject: [p2v PATCH 4/4] Makefile.am: speed up the boot phase of the "virt-p2v in a VM" tests
On 08/30/22 21:53, Eric Blake wrote:
On Tue, Aug 30, 2022 at 03:34:46PM +0200, Laszlo Ersek wrote:
> On 08/30/22 15:11, Richard W.M. Jones wrote:
>>
>> Series looks good, thanks.
>>
>> Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com>
>
> Thanks :)
>
> Let me wait for Eric's next round of comments / answers though, before I
> merge the series.
I think I've answered everything pending on the 'set -e' and
'#!/bin/bash -' questions. I'm not opposed to them going in as-is,
since it matches existing usage, and as long as we are aware of the
potential for pitfalls. Beyond that, I don't think I had any other
comments, so I'm okay if you are ready to merge.
Commit range a1eeb11c87f9..0687cea6a86e.
Thanks!
Laszlo
814
days inactive
819
days old
15 comments
3 participants
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones