[Libguestfs] [v2v PATCH] convert_linux: start the QEMU guest agent in a distro-specific way

[Laszlo Ersek]

On 08/17/22 18:03, Richard W.M. Jones wrote:
> On Wed, Aug 17, 2022 at 04:47:36PM +0200, Laszlo Ersek wrote:
>> The current command "service <package-name> start" does not apply to
>> RHEL-6; the service name ("qemu-ga") differs from the package name
>> ("qemu-guest-agent") there.
>>
>> Overhaul the logic -- detach the command from the package name; cover the
>> RHEL, ALT, SUSE and Debian families separately. Remove the "chkconfig"
>> command, as in all tested / investigated cases, it is unnecessary.
>>
>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2028764
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>>  convert/convert_linux.ml | 56 ++++++++++++++++++++++++++++------------
>>  1 file changed, 40 insertions(+), 16 deletions(-)
>>
>> diff --git a/convert/convert_linux.ml b/convert/convert_linux.ml
>> index 2aaa438e42ac..b8e9ad15e22d 100644
>> --- a/convert/convert_linux.ml
>> +++ b/convert/convert_linux.ml
>> @@ -66,6 +66,34 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
>>      | _ -> None
>>    in
>>  
>> +  let qga_svc_start_cmd family distro major =
>> +    match family, distro, major with
>> +    | `RHEL_family, ( "rhel" | "centos" | "scientificlinux" | "redhat-based" |
>> +                      "oraclelinux" ), 6 ->
> 
> Shouldn't this be:
>                                  ...    , i when i <= 6 ->

No, I checked RHEL5, and it had never shipped the guest agent:

https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c52

Most probably, upstream QEMA had not introduced QGA by then.

In fact, RHEL-6's QEMU version is based on upstream 0.12, which is
(IIRC) the very first version of upstream QEMU that integrated KVM
support; so that was when we switched from "kvm" to "qemu" in userspace.
So even if upstream QEMU had introduced QGA before 0.12, RHEL5 would not
have used it.

> 
> ?
> 
>> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c52 *)
>> +      Some "service qemu-ga start"
>> +
>> +    | `RHEL_family, _, _ ->
>> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c52 *)
>> +      Some "systemctl start qemu-guest-agent"
>> +
>> +    | `ALT_family, _, _ ->
>> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c45 *)
>> +      Some "systemctl start qemu-guest-agent"
>> +
>> +    | `SUSE_family, _, _ ->
>> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c51 *)
>> +      None
>> +
>> +    | `Debian_family, _, _ ->
>> +      (* https://bugzilla.redhat.com/show_bug.cgi?id=2028764#c42 *)
>> +      Some "service qemu-guest-agent start"
>> +
>> +    | _ ->
>> +      (* should never be called when "qga_pkg_of_family" returns None *)
>> +      assert false
>> +  in
>> +
>>    assert (inspect.i_package_format = "rpm" || inspect.i_package_format = "deb");
>>  
>>    (* Fail early if i_apps is empty.  Certain steps such as kernel
>> @@ -615,23 +643,19 @@ let convert (g : G.guestfs) source inspect keep_serial_console _ =
>>                          \ \ rm -f %s\n\
>>                          fi\n" selinux_enforcing selinux_enforcing);
>>  
>> -            (* 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.
>> +            (* On all the distro families covered by "qga_pkg_of_family" and
>> +             * "qga_svc_start_cmd", the QEMU guest agent service is always
>> +             * enabled by package installation for *subsequent* boots. Package
>> +             * installation may or may not enable the service for the current
>> +             * (i.e., first) boot, however, so try that here manually.
>>               *)
>> -            fbs "start qga"
>> -              (sprintf "#!/bin/sh\n\
>> -                        service %s start\n\
>> -                        chkconfig %s on\n" qga_pkg qga_pkg)
>> +            match qga_svc_start_cmd family inspect.i_distro inspect.i_major_version
>> +            with
>> +            | None -> ()
>> +            | Some start_cmd ->
>> +              fbs "start qga"
>> +                (sprintf "#!/bin/sh\n\
>> +                          %s\n" start_cmd)
>>            with
>>            | Guest_packages.Unknown_package_manager msg
>>            | Guest_packages.Unimplemented_package_manager msg ->
> 
> If you wanted to reduce the long range dependency between
> qga_pkg_of_family and qga_svc_start_cmd (and hence the assert) you
> could make a qga_something function that returns both the package name
> and the service start command as a pair of strings (or None).  It
> could make the code more maintainable long term.

I considered this, but opted out of it, because the call to
"qga_svc_start_cmd" depends on additional conditions relative to the
dependencies of the call to "qga_pkg_of_family": (1) If the package is
already installed, we don't need the startup command; (2) if the package
management system is unknown or unimplemented in v2v, we don't need the
startup command. I don't think it would be a huge waste of CPU cycles, I
just conceptually don't like computing something that we might throw
away later. (Unless precomputing can significantly improve performace
later on, of course.)

... We can still refactor it later, should we ever need to extend both
functions with a new OS pattern.

> 
> It all looks fine, so ACK either way.

Thanks; I'll go ahead and merge it, and backport it too, in order to let
QE start testing it soon.

Laszlo