Libguestfs - Aug 2016 - Re: [PATCH 8/8] v2v: linux: correctly reconfigure the initrd on Debian

Hi,

this series implements the first bits in v2v to convert Debian/Ubuntu
(and derived) guests.  The series does not complete the support (see
known issues below), but all the patches here should be fit for review
and inclusion.  The series does not enable the conversion, yet.

Known issues:

* there is no grubby nor Bootloader::Tools Perl module available in
  Debian, so there is no way to know what is the default kernel;
  parsing the Grub2 configuration is not exactly an easy task either

* currently tested with simple local guest images, hence needs testing
  with real guests on libvirt/VMware/Xen/VirtualBox/etc (which have
  various tools to be removed/tweaked)

* is tweaking the "enterprise-linux" module the correct/wanted way?
  should it be renamed to just "linux" then?

* surely something else I'm missing

Thanks,


Pino Toscano (8):
  v2v: refactor Linux.remove
  v2v: add basic support for the "deb" package manager
  v2v: linux: identify Debian-based distros as `Debian_family
  v2v: linux: add /boot/grub/grub.cfg as Grub2 config
  v2v: linux: check also kernel config for modules
  v2v: linux: check the kernel package name for Debian
  v2v: linux: adapt initrd name for Debian
  v2v: linux: correctly reconfigure the initrd on Debian

 v2v/convert_linux.ml | 39 +++++++++++++++++++++++++++++++++------
 v2v/linux.ml         | 52 +++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 72 insertions(+), 19 deletions(-)

-- 
2.7.4

Move the actual job in an helper function, so the common bits (like the
check of the size of 'packages' and the reload of Augeas) can be done
for all the package manager implementations.

This should be code motion with no behaviour change.
---
 v2v/linux.ml | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/v2v/linux.ml b/v2v/linux.ml
index 46cb3ba..ed639c1 100644
--- a/v2v/linux.ml
+++ b/v2v/linux.ml
@@ -37,22 +37,25 @@ and augeas_reload g    g#aug_load ();
   debug_augeas_errors g
 
-let remove g inspect packages +let rec remove g inspect packages    if packages
<> [] then (
-    let package_format = inspect.i_package_format in
-    match package_format with
-    | "rpm" ->
-      let cmd = [ "rpm"; "-e" ] @ packages in
-      let cmd = Array.of_list cmd in
-      ignore (g#command cmd);
+    do_remove g inspect packages;
+    (* Reload Augeas in case anything changed. *)
+    augeas_reload g
+  )
 
-      (* Reload Augeas in case anything changed. *)
-      augeas_reload g
+and do_remove g inspect packages +  assert (List.length packages > 0);
+  let package_format = inspect.i_package_format in
+  match package_format with
+  | "rpm" ->
+    let cmd = [ "rpm"; "-e" ] @ packages in
+    let cmd = Array.of_list cmd in
+    ignore (g#command cmd)
 
-    | format ->
-      error (f_"don't know how to remove packages using %s: packages:
%s")
-        format (String.concat " " packages)
-  )
+  | format ->
+    error (f_"don't know how to remove packages using %s: packages:
%s")
+      format (String.concat " " packages)
 
 let file_list_of_package (g : Guestfs.guestfs) inspect app    let
package_format = inspect.i_package_format in
-- 
2.7.4

Implement the 'remove', 'file_list_of_package', and
'file_owner' methods
of the Linux module for the "deb" package manager (dpkg basically, on
Debian and derived distributions).

Also allow it for the main conversion code.
---
 v2v/convert_linux.ml |  2 +-
 v2v/linux.ml         | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 4b1ce99..65796d6 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -79,7 +79,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      | "sles" | "suse-based" |
"opensuse" -> `SUSE_family
     | _ -> assert false in
 
-  assert (inspect.i_package_format = "rpm");
+  assert (inspect.i_package_format = "rpm" ||
inspect.i_package_format = "deb");
 
   (* We use Augeas for inspection and conversion, so initialize it early. *)
   Linux.augeas_init g;
diff --git a/v2v/linux.ml b/v2v/linux.ml
index ed639c1..5713f64 100644
--- a/v2v/linux.ml
+++ b/v2v/linux.ml
@@ -48,6 +48,10 @@ and do_remove g inspect packages    assert (List.length
packages > 0);
   let package_format = inspect.i_package_format in
   match package_format with
+  | "deb" ->
+    let cmd = [ "dpkg"; "--purge" ] @ packages in
+    let cmd = Array.of_list cmd in
+    ignore (g#command cmd);
   | "rpm" ->
     let cmd = [ "rpm"; "-e" ] @ packages in
     let cmd = Array.of_list cmd in
@@ -61,6 +65,12 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app   
let package_format = inspect.i_package_format in
 
   match package_format with
+  | "deb" ->
+    let cmd = [| "dpkg"; "-L"; app.G.app2_name |] in
+    debug "%s" (String.concat " " (Array.to_list cmd));
+    let files = g#command_lines cmd in
+    let files = Array.to_list files in
+    List.sort compare files
   | "rpm" ->
     (* Since RPM allows multiple packages installed with the same
      * name, always check the full ENVR here (RHBZ#1161250).
@@ -98,6 +108,19 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app 
let rec file_owner (g : G.guestfs) inspect path    let package_format =
inspect.i_package_format in
   match package_format with
+  | "deb" ->
+      let cmd = [| "dpkg"; "-S"; path |] in
+      debug "%s" (String.concat " " (Array.to_list cmd));
+      let lines +        try g#command_lines cmd
+        with Guestfs.Error msg as exn ->
+          if String.find msg "no path found matching pattern" >= 0
then
+            raise Not_found
+          else
+            raise exn in
+      if Array.length lines = 0 then
+        error (f_"internal error: file_owner: dpkg command returned no
output");
+      lines.(0)
   | "rpm" ->
       (* Although it is possible in RPM for multiple packages to own
        * a file, this deliberately only returns one package.
-- 
2.7.4

Identify these distributions, so it is possible to add specific code
for them.

This does not allow them as supported distributions, yet.
---
 v2v/convert_linux.ml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 65796d6..52d06bd 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -77,6 +77,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      | "rhel" | "centos" |
"scientificlinux" | "redhat-based"
     | "oraclelinux" -> `RHEL_family
     | "sles" | "suse-based" | "opensuse" ->
`SUSE_family
+    | "debian" | "ubuntu" | "linuxmint" ->
`Debian_family
     | _ -> assert false in
 
   assert (inspect.i_package_format = "rpm" ||
inspect.i_package_format = "deb");
-- 
2.7.4

This is the location of the Grub2 configuration in Debian-based systems.
---
 v2v/convert_linux.ml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 52d06bd..acc91dc 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -94,6 +94,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps    let grub_config, grub      let locations = [
       "/boot/grub2/grub.cfg", `Grub2;
+      "/boot/grub/grub.cfg", `Grub2;
       "/boot/grub/menu.lst", `Grub1;
       "/boot/grub/grub.conf", `Grub1;
     ] in
-- 
2.7.4

When checking whether a kernel supports virtio or it is Xen-based, it is
assumed that the feature has the kernel module for it; this will fail if
the feature is built-in in the kernel, misrepresenting it.

The solution is to check the kernel configuration (/boot/config-$kver)
whether the feature is built-in, in case there is no module available.

This fixes the virtio detection on Ubuntu kernels, where virtio is
built in and not as module.
---
 v2v/convert_linux.ml | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index acc91dc..6ca7cdb 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -125,6 +125,21 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps    (* What kernel/kernel-like packages are installed on the
current guest? *)
   let installed_kernels : kernel_info list      let rex_ko = Str.regexp
".*\\.k?o\\(\\.xz\\)?$" in
+    let check_config version feature +      let prefix = "^CONFIG_" ^
String.uppercase_ascii feature ^ "=" in
+      let lines = g#grep ~extended:true prefix ("/boot/config-" ^
version) in
+      let lines = Array.to_list lines in
+      match lines with
+      | [] -> false
+      | line :: _ ->
+        let kind = snd (String.split "=" line) in
+        (match kind with
+        | "m" (* Theoretically this should not be needed, since the
module
+               * would be found. *)
+        | "y" -> true
+        | _ -> false
+        )
+    in
     let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
     let rex_initrd = Str.regexp
"^initr\\(d\\|amfs\\)-.*\\(\\.img\\)?$" in
     filter_map (
@@ -223,8 +238,11 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps               ) modules in
              assert (List.length modules > 0);
 
-             let supports_virtio = List.mem "virtio_net" modules in
-             let is_xen_kernel = List.mem "xennet" modules in
+             let kernel_supports what +               List.mem what modules ||
check_config version what in
+
+             let supports_virtio = kernel_supports "virtio_net" in
+             let is_xen_kernel = kernel_supports "xennet" in
 
              (* If the package name is like "kernel-debug", then
it's
               * a debug kernel.
-- 
2.7.4

On Debian-based systems, the kernel packages are named like
"linux-image-$kver", so check for them.
---
 v2v/convert_linux.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 6ca7cdb..1800d3d 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -145,7 +145,8 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      filter_map (
       function
       | { G.app2_name = name } as app
-          when name = "kernel" || String.is_prefix name
"kernel-" ->
+          when name = "kernel" || String.is_prefix name
"kernel-"
+               || String.is_prefix name "linux-image-" ->
         (try
            (* For each kernel, list the files directly owned by the kernel. *)
            let files = Linux.file_list_of_package g inspect app in
-- 
2.7.4

The name of the initrd image on Debian-based systems is different from
what used on Fedora/RHEL/SUSE and derived; set a different regexp to
avoid making the current regexp even more complex.
---
 v2v/convert_linux.ml | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 1800d3d..cfe46b8 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -141,7 +141,11 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps          )
     in
     let rex_ko_extract = Str.regexp
".*/\\([^/]+\\)\\.k?o\\(\\.xz\\)?$" in
-    let rex_initrd = Str.regexp
"^initr\\(d\\|amfs\\)-.*\\(\\.img\\)?$" in
+    let rex_initrd +      if family = `Debian_family then
+        Str.regexp "^initrd.img-.*$"
+      else
+        Str.regexp "^initr\\(d\\|amfs\\)-.*\\(\\.img\\)?$" in
     filter_map (
       function
       | { G.app2_name = name } as app
-- 
2.7.4

Use the canonical way to regenerate the initrd images for all the
installed kernels, i.e. reconfigure the initramfs-tools which will
trigger the kernel postinst scripts.
---
 v2v/convert_linux.ml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index cfe46b8..a556ce6 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -866,7 +866,9 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps          ignore (g#command (Array.of_list args))
       in
 
-      if g#is_file ~followsymlinks:true "/sbin/dracut" then
+      if family = `Debian_family then
+        ignore (g#command ([| "dpkg-reconfigure";
"--frontend=noninteractive"; "initramfs-tools" |]))
+      else if g#is_file ~followsymlinks:true "/sbin/dracut" then
         run_dracut_command "/sbin/dracut"
       else if g#is_file ~followsymlinks:true "/usr/bin/dracut" then
         run_dracut_command "/usr/bin/dracut"
-- 
2.7.4

On Mon,  8 Aug 2016 18:38:49 +0200
Pino Toscano <ptoscano@redhat.com> wrote:
> Implement the 'remove', 'file_list_of_package', and
'file_owner' methods
> of the Linux module for the "deb" package manager (dpkg
basically, on
> Debian and derived distributions).
> 
> Also allow it for the main conversion code.
> ---
>  v2v/convert_linux.ml |  2 +-
>  v2v/linux.ml         | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index 4b1ce99..65796d6 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -79,7 +79,7 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >      | "sles" | "suse-based" |
"opensuse" -> `SUSE_family
>      | _ -> assert false in
>  
> -  assert (inspect.i_package_format = "rpm");
> +  assert (inspect.i_package_format = "rpm" ||
inspect.i_package_format = "deb");
>  
>    (* We use Augeas for inspection and conversion, so initialize it early.
*)
>    Linux.augeas_init g;
> diff --git a/v2v/linux.ml b/v2v/linux.ml
> index ed639c1..5713f64 100644
> --- a/v2v/linux.ml
> +++ b/v2v/linux.ml
> @@ -48,6 +48,10 @@ and do_remove g inspect packages >    assert
(List.length packages > 0);
>    let package_format = inspect.i_package_format in
>    match package_format with
> +  | "deb" ->
> +    let cmd = [ "dpkg"; "--purge" ] @ packages in
> +    let cmd = Array.of_list cmd in
> +    ignore (g#command cmd);
>    | "rpm" ->
>      let cmd = [ "rpm"; "-e" ] @ packages in
>      let cmd = Array.of_list cmd in
> @@ -61,6 +65,12 @@ let file_list_of_package (g : Guestfs.guestfs) inspect
app >    let package_format = inspect.i_package_format in
>  
>    match package_format with
> +  | "deb" ->
> +    let cmd = [| "dpkg"; "-L"; app.G.app2_name |] in
> +    debug "%s" (String.concat " " (Array.to_list
cmd));
> +    let files = g#command_lines cmd in
> +    let files = Array.to_list files in
> +    List.sort compare files
>    | "rpm" ->
>      (* Since RPM allows multiple packages installed with the same
>       * name, always check the full ENVR here (RHBZ#1161250).
> @@ -98,6 +108,19 @@ let file_list_of_package (g : Guestfs.guestfs) inspect
app >  let rec file_owner (g : G.guestfs) inspect path >    let
package_format = inspect.i_package_format in
>    match package_format with
> +  | "deb" ->
> +      let cmd = [| "dpkg"; "-S"; path |] in
> +      debug "%s" (String.concat " " (Array.to_list
cmd));
> +      let lines > +        try g#command_lines cmd
This is not good. dpkg-query command behaves differently from rpm.
First, the returned packages are all on one line separated by commas.
What I came up with is this:

      let cmd = [| "dpkg-query"; "-S"; path |] in
      debug "%s" (String.concat " " (Array.to_list cmd));
      (try
         let pkg = g#command cmd in
         (* What we get is a string of form "pkg1, pkg2:arch, pkg3:
/path" *)
         let len = String.length pkg in
         let rec loop i              if i >= len then
               (* This is fishy. Internal bug? *)
               error (f_"internal error: file_owner: failed to process
string '%s'") pkg
             else if pkg.[i] = ' ' &&
               (pkg.[i-1] = ':' || pkg.[i-1] = ',') then
                 String.sub pkg 0 (i-1)
             else loop (i+1)
         in loop 1
       with Guestfs.Error msg as exn ->
         if String.find msg "no path found matching pattern" >= 0
then
           raise Not_found
         else
           raise exn
      )

The other concern is, whether we should escape the argument somehow. The
fact is that dpkg-query accepts a glob expression, not just a simple
path. If you provide it with a pattern you will get all the matching
paths, each on a single line (prefixed by the package list).

This might not be an issue. Depends on how we use the method. Thoughts?

> +        with Guestfs.Error msg as exn ->
> +          if String.find msg "no path found matching pattern"
>= 0 then
> +            raise Not_found
> +          else
> +            raise exn in
> +      if Array.length lines = 0 then
> +        error (f_"internal error: file_owner: dpkg command returned
no output");
> +      lines.(0)
>    | "rpm" ->
>        (* Although it is possible in RPM for multiple packages to own
>         * a file, this deliberately only returns one package.

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

On Mon,  8 Aug 2016 18:38:55 +0200
Pino Toscano <ptoscano@redhat.com> wrote:
> Use the canonical way to regenerate the initrd images for all the
> installed kernels, i.e. reconfigure the initramfs-tools which will
> trigger the kernel postinst scripts.
> ---
>  v2v/convert_linux.ml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
> index cfe46b8..a556ce6 100644
> --- a/v2v/convert_linux.ml
> +++ b/v2v/convert_linux.ml
> @@ -866,7 +866,9 @@ let rec convert ~keep_serial_console (g : G.guestfs)
inspect source rcaps >          ignore (g#command (Array.of_list args))
>        in
>  
> -      if g#is_file ~followsymlinks:true "/sbin/dracut" then
> +      if family = `Debian_family then
> +        ignore (g#command ([| "dpkg-reconfigure";
"--frontend=noninteractive"; "initramfs-tools" |]))
> +      else if g#is_file ~followsymlinks:true "/sbin/dracut" then
>          run_dracut_command "/sbin/dracut"
>        else if g#is_file ~followsymlinks:true "/usr/bin/dracut"
then
>          run_dracut_command "/usr/bin/dracut"
It might be better to call update-initramfs directly. I don't know, is
using dpkg-reconfigure maybe safer across distributions? The slight
advantage of calling update-initramfs is that we can turn on verbose
mode, just like for dracut:

      let run_update_initramfs_command ()         let args          
"update-initramfs"  ::
            (if verbose () then [ "-v" ] else [])
          @ [ "-c"; "-k"; mkinitrd_kv ]
        in
        ignore (g#command (Array.of_list args))
      in

The other advantage is that we can provide the kernel version we want.
The default is to update the initramfs for the latest kernel, which in
theory might not be the one we want if it were missing virtio drivers
(not sure if this can happen). According to ordering we do any kernel
without virtio drivers is worse than any kernel with virtio drivers.

What we can also do is specify list of modules we require. Similar to
what we do for dracut/mkinitrd. Debian uses a file for that:

        (* The modules to add to initrd are defined in a file. *)
        ignore (g#sh "sh -c 'echo \\# Added by virt-v2v >>
/etc/initramfs-tools/modules'");
        let cmd = (sprintf "sh -c 'echo %s >>
/etc/initramfs-tools/modules'"
          (String.concat " " modules)) in
        ignore (g#sh cmd);



The commits I didn't reply to LGTM. 

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

Implement the 'remove', 'file_list_of_package', and
'file_owner' methods
of the Linux module for the "deb" package manager (dpkg basically, on
Debian and derived distributions).

Also allow it for the main conversion code.
---
 v2v/convert_linux.ml |  2 +-
 v2v/linux.ml         | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml
index 4b1ce99..65796d6 100644
--- a/v2v/convert_linux.ml
+++ b/v2v/convert_linux.ml
@@ -79,7 +79,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect
source rcaps      | "sles" | "suse-based" |
"opensuse" -> `SUSE_family
     | _ -> assert false in
 
-  assert (inspect.i_package_format = "rpm");
+  assert (inspect.i_package_format = "rpm" ||
inspect.i_package_format = "deb");
 
   (* We use Augeas for inspection and conversion, so initialize it early. *)
   Linux.augeas_init g;
diff --git a/v2v/linux.ml b/v2v/linux.ml
index ed639c1..d449e10 100644
--- a/v2v/linux.ml
+++ b/v2v/linux.ml
@@ -48,6 +48,10 @@ and do_remove g inspect packages    assert (List.length
packages > 0);
   let package_format = inspect.i_package_format in
   match package_format with
+  | "deb" ->
+    let cmd = [ "dpkg"; "--purge" ] @ packages in
+    let cmd = Array.of_list cmd in
+    ignore (g#command cmd);
   | "rpm" ->
     let cmd = [ "rpm"; "-e" ] @ packages in
     let cmd = Array.of_list cmd in
@@ -61,6 +65,12 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app   
let package_format = inspect.i_package_format in
 
   match package_format with
+  | "deb" ->
+    let cmd = [| "dpkg"; "-L"; app.G.app2_name |] in
+    debug "%s" (String.concat " " (Array.to_list cmd));
+    let files = g#command_lines cmd in
+    let files = Array.to_list files in
+    List.sort compare files
   | "rpm" ->
     (* Since RPM allows multiple packages installed with the same
      * name, always check the full ENVR here (RHBZ#1161250).
@@ -98,6 +108,29 @@ let file_list_of_package (g : Guestfs.guestfs) inspect app 
let rec file_owner (g : G.guestfs) inspect path    let package_format =
inspect.i_package_format in
   match package_format with
+  | "deb" ->
+      (* With dpkg usually the directories are owned by all the packages
+       * that install anything in them.  Also with multiarch the same
+       * package is allowed (although with different architectures).
+       * This function returns only one package in all the cases.
+       *)
+      let cmd = [| "dpkg"; "-S"; path |] in
+      debug "%s" (String.concat " " (Array.to_list cmd));
+      let lines +        try g#command_lines cmd
+        with Guestfs.Error msg as exn ->
+          if String.find msg "no path found matching pattern" >= 0
then
+            raise Not_found
+          else
+            raise exn in
+      if Array.length lines = 0 then
+        error (f_"internal error: file_owner: dpkg command returned no
output");
+      let line = lines.(0) in
+      let line +        try String.sub line 0 (String.rindex line ':')
+        with Invalid_argument _ ->
+          error (f_"internal error: file_owner: invalid dpkg output:
'%s'") line in
+      fst (String.split "," line)
   | "rpm" ->
       (* Although it is possible in RPM for multiple packages to own
        * a file, this deliberately only returns one package.
-- 
2.7.4