[Libguestfs] [v2v PATCH 4/4] convert_linux: install the QEMU guest agent with a firstboot script

[Laszlo Ersek]

On 06/07/22 14:59, Richard W.M. Jones wrote:
> On Mon, Jun 06, 2022 at 04:19:41PM +0200, Laszlo Ersek wrote:
>> Register a firstboot script, for installing the guest agent with the
>> guest's own package manager -- that is, "Guest_packages.install_command".
>>
>> For installing the package, network connectivity is required; for lack of
>> a universal "wait online" dependency expression, register "sleep 60" as a
>> preceding firstboot script.
>>
>> The source domain's SELinux policy may not allow our firstboot service to
>> execute the package's installation scripts (if any). For that reason,
>> temporarily disable SELinux around package installation.
>>
>> After installation, register another script for launching the agent.
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  convert/convert_linux.ml | 73 +++++++++++++++++++-
>>  common                   |  2 +-
>>  2 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
>> index 2ddbc07aa86a..1dcd7abbcee2 100644
>> --- a/convert/convert_linux.ml
>> +++ b/convert/convert_linux.ml
>> @@ -562,8 +562,77 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
>>                name = qga_pkg
>>            ) inspect.i_apps in
>>          if not has_qemu_guest_agent then
>> -          (* FIXME -- install qemu-guest-agent here *)
>> -          ()
>> +          let pkg_mgmt = g#inspect_get_package_management inspect.i_root in
> 
> You can just use: inspect.i_package_management to avoid round-tripping
> through the appliance for information that we already have locally.

Ah great, will do that.

> 
>> +          try
>> +            let inst_cmd = Guest_packages.install_command [qga_pkg] pkg_mgmt in
>> +
>> +            (* Use only the portable filename character set in this. *)
>> +            let selinux_enforcing = "/root/virt-v2v-fb-selinux-enforcing" in
>> +            let fbs =
>> +              Firstboot.add_firstboot_script g inspect.i_root
>> +            in
>> +            info (f_"The QEMU Guest Agent will be installed for this guest at \
>> +                     first boot.");
>> +
>> +            (* Waiting for the guest to go "online" is very complicated. On
>> +             * systemd-based distros, our firstboot service could perhaps depend
>> +             * on "systemd-networkd-wait-online.service" or
>> +             * "NetworkManager-wait-online.service". But not all Linux distros
>> +             * are based on systemd, and even with systemd, a guest could use
>> +             * systemd-networkd vs. NetworkManager... So just do what we do in
>> +             * various Windows firstboot scripts: wait. We could cut the waiting
>> +             * short by pinging a well-known IP address, or by resolving a
>> +             * well-known FQDN, but those might qualify as "phoning home"... So
>> +             * just wait.
>> +             *)
>> +            fbs "wait online"
>> +              "#!/bin/sh\n\
>> +               sleep 60\n";
> 
> Hmmm.
> 
> virt-p2v uses:
> 
>   nm-online -t 30
> 
> Apparently systemd-networkd (which I've never knowingly used) has
> another command:
> 
>   /usr/lib/systemd/systemd-networkd-wait-online
> 
> which ought to do the same thing.  (It defaults to 120 second timeout.)

It's horrible that these incantations are mostly impossible to figure
out :/ anyway, definitely an improvement over the static sleep, so I'll
use your suggestion from below:

> Can we not do something like:
> 
>   nm-online -t 30 ||:
>   /usr/lib/systemd/systemd-networkd-wait-online ||:
> 
> as a best effort?  Seems a lot better idea than sleeping.
> 
> Long term we really ought to add this as a feature of the Firstboot
> module, ie. a parameter which allows a firstboot script to be flagged
> as needing the network, and we'll insert the appropriate code for
> those scripts automatically.

Let's do that when we have three (or at least two) clients for that feature.

> 
>> +            (* Disable SELinux temporarily around package installation. Refer to
>> +             * <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c7> and
>> +             * <https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c8>.
>> +             *)
>> +            fbs "setenforce 0"
>> +              (sprintf "#!/bin/sh\n\
>> +                        rm -f %s\n\
>> +                        if command -v getenforce >/dev/null &&\n\
>> +                        \ \ test Enforcing = \"$(getenforce)\"\n\
>> +                        then\n\
>> +                        \ \ touch %s\n\
>> +                        \ \ setenforce 0\n\
>> +                        fi\n" selinux_enforcing selinux_enforcing);
>> +            fbs "install qga" inst_cmd;
>> +            fbs "setenforce restore"
>> +              (sprintf "#!/bin/sh\n\
>> +                        if test -f %s; then\n\
>> +                        \ \ setenforce 1\n\
>> +                        \ \ rm -f %s\n\
>> +                        fi\n" selinux_enforcing selinux_enforcing);
> 
> Sounds horrible!  But if that's what is needed ...
> 
>> +            (* Start the agent now and at subsequent boots. The following
>> +             * commands should work on both sysvinit distros / distro versions
>> +             * (regardless of "/etc/rc.d/" vs. "/etc/init.d/" being the scheme
>> +             * in use) and systemd distros (via redirection to systemctl).
>> +             *
>> +             * On distros where the chkconfig command is redirected to
>> +             * systemctl, the chkconfig command is likely superfluous. That's
>> +             * because on systemd distros, the QGA package comes with such
>> +             * runtime dependencies / triggers that the presence of the
>> +             * virtio-serial port named "org.qemu.guest_agent.0" automatically
>> +             * starts the agent during (second and later) boots. However, even
>> +             * on such distros, the chkconfig command should do no harm.
>> +             *)
>> +            fbs "start qga"
>> +              (sprintf "#!/bin/sh\n\
>> +                        service %s start\n\
>> +                        chkconfig %s on\n" qga_pkg qga_pkg)
>> +          with
>> +          | Guest_packages.Unknown_package_manager msg
>> +          | Guest_packages.Unimplemented_package_manager msg ->
>> +            warning (f_"The QEMU Guest Agent will not be installed.  The \
>> +                        install command for package ‘%s’ could not be created: \
>> +                        %s.") qga_pkg msg
> 
> The patch seems fine in general, but definitely want to get
> rid of that sleep.

Yep, I'll rework that.

Thanks for the review!
Laszlo