[Libguestfs] [PATCH] builder: Rearrange how template-building scripts work.
Richard W.M. Jones
rjones at redhat.com
Wed Nov 30 13:14:15 UTC 2016
On Wed, Nov 30, 2016 at 01:54:43PM +0100, Pino Toscano wrote:
> On Monday, 28 November 2016 10:40:51 CET Richard W.M. Jones wrote:
> > Create a new directory (builder/template). Integrate all of the
> > scripts into a single program, so that templates are generated more
> > consistently.
> >
> > This also changes how the index file is generated. The script now
> > generates the index file fragment and saves it under version control,
> > and then generates the final index file by concatenating these.
> > (Previously the index was written by hand which was tedious and
> > error-prone.)
> >
> > The new script also saves the generated kickstart under version
> > control so it can be referenced later.
> > ---
>
> Note that the fragments for Ubuntu guests don't seem to match what the
> new make-template.ml produces -- in particular, the ones in this patch
> have osinfo=ubuntu$codename instead of osinfo=ubuntu$version as the
> script should produce. Ah I see, the current index is wrong already...
This is generally true for most of the guests. However I didn't
want to change the existing fragments.
> Also, this patch includes changes in the website index but not in the
> index.asc -- I'd just leave them unchanged in this patch (simple git
> move), and then regenerate them in a followup commit.
The index.asc has to be signed so you wouldn't want to generate
it automatically.
> > diff --git a/builder/website/Makefile.am b/builder/templates/Makefile.am
> > similarity index 66%
> > rename from builder/website/Makefile.am
> > rename to builder/templates/Makefile.am
> > index d79b9a0..d1b89f9 100644
> > --- a/builder/website/Makefile.am
> > +++ b/builder/templates/Makefile.am
> > @@ -1,5 +1,5 @@
> > # libguestfs virt-builder tool
> > -# Copyright (C) 2013 Red Hat Inc.
> > +# Copyright (C) 2013-2016 Red Hat Inc.
> > #
> > # This program is free software; you can redistribute it and/or modify
> > # it under the terms of the GNU General Public License as published by
> > @@ -17,33 +17,25 @@
> >
> > include $(top_srcdir)/subdir-rules.mk
> >
> > +index_fragments = $(wildcard *.index-fragment)
> > +
> > EXTRA_DIST = \
> > - .gitignore \
> > - compress.sh \
> > - test-guest.sh \
> > - validate.sh \
> > - README \
> > - index \
> > - index.asc \
> > - centos.sh \
> > - centos-aarch64.sh \
> > + $(index_fragments) \
> > + *.ks \
>
> Considering $(wildcard) above is already a GNU extension, I'd use it
> for *.ks as well.
>
> > debian.preseed \
> > - debian.sh \
> > - fedora.sh \
> > - fedora-aarch64.sh \
> > - fedora-armv7l.sh \
> > - fedora-i686.sh \
> > - fedora-ppc64.sh \
> > - fedora-ppc64le.sh \
> > - fedora-s390x.sh \
> > - rhel.sh \
> > - rhel-aarch64.sh \
> > - rhel-ppc64.sh \
> > - rhel-ppc64le.sh \
> > - scientificlinux.sh \
> > + make-template.ml \
> > ubuntu.preseed \
> > - ubuntu.sh \
> > - ubuntu-ppc64le.sh
> > + validate.sh
> > +
> > +# Create the index file under the top level website/ directory.
> > +noinst_DATA = $(top_builddir)/website/download/builder/index
> > +
> > +$(top_builddir)/website/download/builder/index: $(index_fragments)
> > + rm -f $@ $@-t
> > + cat *.index-fragment > $@-t
>
> Should $^ be used instead of *index-fragment? Also, the order of the
> files could be made stable with sorting, so the index does not change
> just because of different results of the wildcard expansion.
I used *.index-fragment so that the order is stable. It turns out
that $(wildcard ..) doesn't sort.
> > + (* Set the path to use locally built copies of the virt-* tools. *)
> > + let () =
> > + let orig_path = Unix.getenv "PATH" in
> > + let paths = ["builder"; "customize"; "sparsify"; "sysprep"] in
> > + let prefix = Sys.getcwd () // ".." // ".." in
> > + let paths = List.map ((//) prefix) paths in
> > + let new_path = String.concat ":" (paths @ [orig_path]) in
> > + Unix.putenv "PATH" new_path in
>
> Wouldn't be better just rely on the libguestfs tools in $PATH, i.e.
> using ./run for uninstalled versions? Just use a check similar to the
> one below (inspired from virt-builder):
>
> (* Check that we have libguestfs tools. *)
> let cmd = "virt-sparsify --help >/dev/null 2>&1" in
> if shell_command cmd <> 0 then (
> eprintf "%s: libguestfs tools not available (use ./run for uninstalled versions)\n" prog;
> exit 1
> );
Could do this. The problem is that I wanted to unconditionally use
the just-built tools, to avoid the case where (eg) I make a fix to
virt-sparsify, rebuild a template, but the template isn't sparsified
correctly because it used the wrong version of virt-sparsify.
> > +and os_of_string os ver =
> > + match os, ver with
> > + | "centos", ver -> let maj, min = parse_major_minor ver in CentOS (maj, min)
> > + | "rhel", ver -> let maj, min = parse_major_minor ver in RHEL (maj, min)
> > + | "debian", "6" -> Debian (6, "squeeze")
> > + | "debian", "7" -> Debian (7, "wheezy")
> > + | "debian", "8" -> Debian (8, "jessie")
> > + | "ubuntu", "10.04" -> Ubuntu (ver, "lucid")
> > + | "ubuntu", "12.04" -> Ubuntu (ver, "precise")
> > + | "ubuntu", "14.04" -> Ubuntu (ver, "trusty")
> > + | "ubuntu", "16.04" -> Ubuntu (ver, "xenial")
> > + | "fedora", ver -> Fedora (int_of_string ver)
> > + | _ ->
> > + eprintf "%s: unknown or unsupported OS\n" prog; exit 1
>
> Can the error message also print the distro name and version?
Sure (and others too).
> > +and make_kickstart_common ks_filename os arch =
> > + let buf = Buffer.create 4096 in
> > + let bpf fs = bprintf buf fs in
> > +
> > + bpf "\
> > +# Kickstart file for %s
> > +# Generated by libguestfs.git/builder/templates/make-template.ml
> > +
> > +install
> > +text
> > +reboot
> > +lang en_US.UTF-8
> > +keyboard us
> > +network --bootproto dhcp
> > +rootpw builder
> > +firewall --enabled --ssh
> > +timezone --utc America/New_York
> > +" (string_of_os os arch);
> > +
> > + (match os with
> > + | RHEL (3, _) ->
> > + bpf "\
> > +langsupport en_US
> > +mouse generic
> > +";
> > + | _ -> ()
> > + );
>
> rhel.sh has:
> if [ $major -le 4 ]; then
> so I think the check should rather be:
>
> | RHEL (ver, _) when ver <= 4 ->
OK
> > +
> > + (match os with
> > + | RHEL (3, _) -> ()
> > + | _ -> bpf "selinux --enforcing\n"
> > + );
>
> rhel.sh has:
> if [ $major -ge 4 ]; then
> so I think the check should rather be:
>
> | RHEL (ver, _) when ver >= 4 ->
>
> (OK, the current code does the same considering only RHEL >= 3 is
> supported, but could be better to convert it straight from the script.)
OK
> > +
> > + (match os with
> > + | RHEL (5, _) -> bpf "key --skip\n"
> > + | _ -> ()
> > + );
> > + bpf "\n";
> > +
> > + bpf "bootloader --location=mbr --append=\"%s\"\n"
> > + (kernel_cmdline_of_os os arch);
> > + bpf "\n";
> > +
> > + (match os with
> > + | CentOS _ ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +part /boot --fstype=ext4 --size=512 --asprimary
> > +part swap --size=1024 --asprimary
> > +part / --fstype=ext4 --size=1024 --grow --asprimary
> > +";
> > + | RHEL ((3|4), _) ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +part /boot --fstype=ext2 --size=512 --asprimary
> > +part swap --size=1024 --asprimary
> > +part / --fstype=ext3 --size=1024 --grow --asprimary
> > +";
> > + | RHEL (5, _) ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +part /boot --fstype=ext2 --size=512 --asprimary
> > +part swap --size=1024 --asprimary
> > +part / --fstype=ext4 --size=1024 --grow --asprimary
> > +";
> > + | RHEL (6, _) ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +part /boot --fstype=ext4 --size=512 --asprimary
> > +part swap --size=1024 --asprimary
> > +part / --fstype=ext4 --size=1024 --grow --asprimary
> > +";
> > + | RHEL (7, _) ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +part /boot --fstype=ext4 --size=512 --asprimary
> > +part swap --size=1024 --asprimary
> > +part / --fstype=xfs --size=1024 --grow --asprimary
> > +";
>
> IMHO the RHEL cases could be merged together, leaving the bootfs and
> rootfs selection by version.
I had to change this in a later version because it turns out the above
doesn't work properly for RHEL 7.
> > + | _ ->
> > + bpf "\
> > +zerombr
> > +clearpart --all --initlabel
> > +autopart --type=plain
> > +";
> > + );
> > + bpf "\n";
> > +
> > + (match os with
> > + | RHEL (3, _) -> ()
> > + | _ ->
> > + bpf "\
> > +# Halt the system once configuration has finished.
> > +poweroff
> > +";
> > + );
>
> rhel.sh has:
> if [ $major -ge 4 ]; then
> so I think the check should rather be:
>
> | RHEL (ver, _) when ver >= 4 ->
>
> (Same note above.)
OK
> > + bpf "\n";
> > +
> > + bpf "\
> > +%%packages
> > + at core
> > +";
> > +
> > + (match os with
> > + | RHEL ((3|4|5), _) -> ()
> > + | _ ->
> > + bpf "%%end\n"
> > + );
>
> rhel.sh has:
> if [ $major -ge 6 ]; then
> so I think the check should rather be:
>
> | RHEL (ver, _) when ver >= 6 ->
>
> (Same note above.)
OK
> > +and copy_preseed_to_temporary source =
> > + (* d-i only works if the file is literally called "/preseed.cfg" *)
> > + let d = "/tmp" // random8 () ^ ".tmp" in
>
> Filename.temp_dir_name instead of /tmp here.
Yup.
> > +and make_location os arch =
> > + match os, arch with
> > + | CentOS (major, _), Aarch64 ->
> > + (* XXX This always points to the latest CentOS, so
> > + * effectively the minor number is always ignored.
> > + *)
> > + sprintf "http://mirror.centos.org/altarch/%d/os/aarch64/" major
> > +
> > + | CentOS (major, _), X86_64 ->
> > + (* For 6.x we rebuild this every time there is a new 6.x release, and bump
> > + * the revision in the index.
> > + * For 7.x this always points to the latest CentOS, so
> > + * effectively the minor number is always ignored.
> > + *)
> > + sprintf "http://mirror.centos.org/centos-7/%d/os/x86_64/" major
> > +
> > + | Debian (_, dist), X86_64 ->
> > + sprintf "http://ftp.uk.debian.org/debian/dists/%s/main/installer-amd64"
> > + dist
>
> While we are here, should we use either httpredir.debian.org (but IIRC
> it's deprecated), or deb.debian.org (works best with current testing,
> but should still work also with current stable and older).
>
> > + | _ ->
> > + eprintf "%s: don't know how to calculate the --location for this OS and architecture\n" prog;
> > + exit 1
>
> Ditto as above (i.e. print details of unknown distro/version).
OK.
> > +and make_index_fragment os arch index_fragment output nvram revision expandfs =
> > + let chan = open_out (index_fragment ^ ".new") in
> > + let fpf fs = fprintf chan fs in
> > +
> > + fpf "[%s]\n" (string_of_os_noarch os);
> > + fpf "name=%s\n" (long_name_of_os os arch);
> > + fpf "osinfo=%s\n" (true_os_variant_of_os os);
> > + fpf "arch=%s\n" (string_of_arch arch);
> > + fpf "file=%s\n" output;
> > + (match revision with
> > + | None -> ()
> > + | Some i -> fpf "revision=%d\n" i
> > + );
> > + fpf "checksum[sha512]=%s\n" (sha512sum_of_file output);
> > + fpf "format=raw\n";
> > + fpf "size=%d\n" (virtual_size_gb * 1024 * 1024 * 1024);
>
> Shouldn't this be Int64?
We actually assert that the word size is 64 bits (in main ()), but
using Int64 would be less lazy.
> > +and open_guest filename =
> > + let g = new Guestfs.guestfs () in
> > + g#add_drive_opts ~format:"raw" filename;
> > + g#launch ();
> > +
> > + let roots = g#inspect_os () in
> > + if Array.length roots = 0 then (
> > + eprintf "%s: cannot inspect this guest\n" prog;
> > + exit 1
> > + );
> > + let root = roots.(0) in
> > +
> > + let mps = g#inspect_get_mountpoints root in
> > + let cmp (a,_) (b,_) = compare (String.length a) (String.length b) in
> > + let mps = List.sort cmp mps in
> > + List.iter (
> > + fun (mp, dev) ->
> > + try g#mount_ro dev mp
> > + with Guestfs.Error msg -> eprintf "%s (ignored)\n" msg
>
> Should mount errors be ignored in this case?
No they shouldn't be ignored.
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
More information about the Libguestfs
mailing list