[Libguestfs] [PATCH v4] RFC: New tool: virt-dib

[Pino Toscano]

On Tuesday 30 June 2015 20:14:24 Richard W.M. Jones wrote:
> On Tue, Jun 16, 2015 at 12:15:22PM +0200, Pino Toscano wrote:
> [...]
> 
> There is some trailing whitespace on one line.  'git show' should
> highlight it.

Fixed, thanks.

> > diff --git a/appliance/packagelist.in b/appliance/packagelist.in
> > index 76c7293..a4f814b 100644
> > --- a/appliance/packagelist.in
> > +++ b/appliance/packagelist.in
> > @@ -255,5 +255,12 @@ zerofree
> >  
> >  ifelse(VALGRIND_DAEMON,1,valgrind)
> >  
> > +dnl tools needed by virt-dib
> > +curl
> > +qemu-img
> > +debootstrap
> > +apt
> > +which
> 
> What's the purpose of each of these new tools added to the appliance?
> Is qemu-img really run inside the appliance?

At least curl, qemu-img and which are basically needed by almost all
the common elements. Basically, we have to pull in the appliance tools
which are used by elements run outside the build chroot of
disk-image-create (i.e. on the build host).

> In particular, is apt (which probably requires perl) only used on
> Debian (host), or would it be pulled in on all builds?

apt and debootstrap are needed when you want to build a Debian guest,
and it is possible also on non-Debian hosts (for example works fine on
my Fedora).

> I think you can consider moving these to the respective
> distro-specific sections of appliance/packagelist.in, and you don't
> need the comment.  However when packaging it for Fedora we'll need to
> split out the dependencies again.

Just wondering if I should resurrect my past idea for different
appliances ("flavours") with own sets of extra packages, tailored for
specific usages (e.g. rescue, dib, etc).

> > +let prepare_external ~dib_args ~dib_vars ~out_name ~root_label ~rootfs_uuid
> > +  ~image_cache ~arch ~network ~debug
> > +  destdir libdir hooksdir tmpdir fakebindir all_elements element_paths =
> > +  let network_string = if network then "" else "1" in
> > +
> > +  let run_extra = sprintf "\
> > +#!/bin/bash
> 
> There are a bunch of embedded bash scripts.  Probably they need to use
> 'set -e'.  Quoting seems OK, but if there are any quotes wrong, then
> it might lead to an appliance exploit, which worried me.  Can we do it
> in any other way apart from using shell scripts (this is supposed to
> be a secure alternative to diskimage-builder after all ..)

Unfortunately not, because running each element needs to source all the
environment.d scripts available, and also provide the environment that
disk-image-create itself makes available.

The two bigger scripts, run-part-extra.sh and run-part.sh, are
basically smaller reimplementations of dib-run-parts, as I wanted to
avoid using it. One reason is that is executes all the scripts of a
phase (e.g. root.d, pre-install.d, etc) at once, with no
script-by-script control from the outside.

The other big script is the fake sudo: basically, some of the element
scripts are run by disk-image-create on the host, and they do things
like using root-only tools (losetup, kpartx, sfdisk, etc) or copy as
root stuff directly into the build chroot; because of this they use
sudo (which is a prerequisite for using disk-image-create).
Since we run most of them (except extra-data.d, which need to run on
the host) in the appliance, they would fail because of missing sudo.
So the alternatives are either
(a) setup sudo inside the appliance (which I guess is not acceptable)
(b) create a fake sudo script which just runs the script, mimicking
    sudo options if possible
So I chose (b).

> I thought the rest of the implementation was surprisingly simple,
> which is good.
> 
> > +=item B<--mkfs-options> C<OPTION STRING>
> > +
> > +Add the specified options to L<mkfs(1)>, to be able to fine-tune
> > +the root filesystem creation.  Note that this is not possible
> > +to override the filesystem type.
> > +
> > +You should use I<--mkfs-options> at most once.  To pass multiple
> > +options, separate them with space, eg:
> > +
> > + virt-dib ... --mkfs-options '-O someopt -I foo'
> 
> Not sure I want to know what horrors allow this to work ...

This was added upstream as https://review.openstack.org/#/c/165149/
Also I'm not taking the disk-image-create option --max-online-resize,
which so far I haven't seen used at all, so in the remote case people
need it then --mkfs-options will be their way to use it.

> And quoting -- is it safe?  What if I want to put a space into a mkfs
> parameter?

I think --mkfs-options '-foo "bar extra"' should work.

> > +=item B<--qemu-img-options> option[,option,...]
> > +
> > +Pass I<--qemu-img-options> option(s) to the L<qemu-img(1)> command
> > +to fine-tune the output format.  Options available depend on
> > +the output format (see I<--formats>) and the installed version
> > +of the qemu-img program.
> > +
> > +You should use I<--qemu-img-options> at most once.  To pass multiple
> > +options, separate them with commas, eg:
> > +
> > + virt-dib ... -qemu--img-options cluster_size=512,preallocation=metadata ...
> 
> Ditto.

Note that this comes from virt-sparsify, and it is quoted when running
qemu-img.

> > +Do not compress resulting qcow2 images.  The default is
> > +to compressed them.
> 
> ^ compress

Fixed, thanks.

> > +=head1 COMPARISON WITH DISKIMAGE-BUILDER
> > +
> > +Virt-dib is intended as safe replacement for C<diskimage-builder>
> > +and its C<ramdisk-image-create> mode; the user-notable differences
> > +consist in:
> > +
> > +=over 4
> > +
> > +=item
> > +
> > +the command line arguments; some of the arguments are the same as
> > +available in C<diskimage-builder>, while some have different names:
> > +
> > + disk-image-create             virt-dib
> > + -----------------             --------
> > + -a ARCH                       --arch ARCH
> > + --image-size SIZE             --size SIZE
> > + --max-online-resize SIZE      doable using --mkfs-options
> > + -n                            --skip-base
> > + -o IMAGENAME                  --name IMAGENAME
> > + -p PACKAGE(S)                 --extra-packages PACKAGE(S)
> > + -t FORMAT(S)                  --formats FORMAT(S)
> > + -x                            --debug N
> 
> Good way to get people started with virt-dib.

A table would have been nice, but POD does not support them :-/

Thanks,
-- 
Pino Toscano