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

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 ->

?
> +      (* 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.

It all looks fine, so ACK either way.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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