[Libguestfs] [PATCH] builder: Rearrange how template-building scripts work.
Pino Toscano
ptoscano at redhat.com
Wed Nov 30 12:54:43 UTC 2016
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...
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.
> 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.
> diff --git a/builder/templates/make-template.ml b/builder/templates/make-template.ml
> new file mode 100755
> index 0000000..71940ff
> --- /dev/null
> +++ b/builder/templates/make-template.ml
> @@ -0,0 +1,1014 @@
> +#!/usr/bin/env ocaml
> +(* libguestfs
> + * Copyright (C) 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
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *)
> +
> +(* This script is used to create the virt-builder templates hosted
> + * http://libguestfs.org/download/builder/
> + *
> + * Usage:
> + * ./make-template.ml os version [arch]
> + * eg:
> + * ./make-template.ml fedora 25
> + * ./make-template.ml rhel 7.3 ppc64le
> + *
> + * The arch defaults to x86_64. Note that i686 is a separate arch.
> + *
> + * Prior to November 2016, the templates were generated using
> + * shell scripts located in libguestfs.git/builder/website.
> + *)
> +
> +open Printf
> +
> +let prog = "make-template"
> +
> +(* Check we are being run from the correct directory. We must do this
> + * before attempting to load libraries below.
> + *)
> +let () =
> + if not (Sys.file_exists "debian.preseed") then (
> + eprintf "%s: run this script from the builder/templates subdirectory\n"
> + prog;
> + exit 1
> + );;
> +
> +#load "str.cma";;
> +#load "unix.cma";;
> +#directory "../../ocaml";;
> +#load "mlguestfs.cma";;
> +
> +type os =
> + | CentOS of int * int (* major, minor *)
> + | RHEL of int * int
> + | Debian of int * string (* version, dist name like "wheezy" *)
> + | Ubuntu of string * string
> + | Fedora of int (* version number *)
> +type arch = X86_64 | Aarch64 | Armv7 | I686 | PPC64 | PPC64le | S390X
> +
> +let quote = Filename.quote
> +let (//) = Filename.concat
> +
> +let virtual_size_gb = 6
> +
> +let rec main () =
> + assert (Sys.word_size = 64);
> + Random.self_init ();
> +
> + (* Get the os, version, arch from the command line. *)
> + if Array.length Sys.argv < 3 || Array.length Sys.argv > 4 then (
> + eprintf "%s os version [arch]\n" prog;
> + exit 1
> + );
> +
> + let os = os_of_string Sys.argv.(1) Sys.argv.(2)
> + and arch =
> + if Array.length Sys.argv <= 3 then X86_64
> + else arch_of_string Sys.argv.(3) in
> +
> + (* 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
);
> +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?
> +and parse_major_minor ver =
> + let rex = Str.regexp "^\\([0-9]+\\)\\.\\([0-9]+\\)$" in
> + if Str.string_match rex ver 0 then (
> + int_of_string (Str.matched_group 1 ver),
> + int_of_string (Str.matched_group 2 ver)
> + )
> + else (
> + eprintf "%s: cannot parse major.minor\n" prog;
> + exit 1
> + )
Ditto.
> +and arch_of_string = function
> + | "x86_64" -> X86_64
> + | "aarch64" -> Aarch64
> + | "armv7l" -> Armv7
> + | "i686" -> I686
> + | "ppc64" -> PPC64
> + | "ppc64le" -> PPC64le
> + | "s390x" -> S390X
> + | _ ->
> + eprintf "%s: unknown or unsupported arch\n" prog; exit 1
Ditto.
> +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 ->
> +
> + (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.)
> +
> + (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.
> + | _ ->
> + 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.)
> + 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.)
> +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.
> +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).
> +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?
> +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?
Thanks,
--
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20161130/d36a3681/attachment.sig>
More information about the Libguestfs
mailing list