[p2v PATCH 0/4] improve the "virt-p2v in a VM" tests
"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
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@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=
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@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
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@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=
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
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
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
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
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@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
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@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=
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@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=
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@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 \
Series looks good, thanks. Reviewed-by: Richard W.M. Jones <rjones@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
On 08/30/22 15:11, Richard W.M. Jones wrote:
Series looks good, thanks.
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Thanks :) Let me wait for Eric's next round of comments / answers though, before I merge the series. Laszlo
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@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
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@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
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones