Re: [Xen-devel] [PATCH 4/7] osstest: add a FreeBSD host install recipe

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH 4/7] osstest: add a FreeBSD host install 
recipe"):
> The installation is performed using the bsdinstall tool, which is
> part of the FreeBSD base system. The installer image is setup with
> the osstest ssh keys and sshd enabled by default, which allows the
> test harness to just ssh into the box, create the install config
> file and launch the scripted install.

Thanks.  I've read this in some detail now.  My comments follow.

> In order to support the FreeBSD installer a new method is added,
> that allows setting the pxe boot of a host using a memdisk. Also, a
> new tftp path is added in order to point to the place where the
> FreeBSD installed images should be stored.

Can you please add to the commit message a deployment note that the
tftp root needs to be provide with a copy of memdisk, eg
   /home/tftp/memdisk -> /usr/lib/syslinux/memdisk

> diff --git a/Osstest.pm b/Osstest.pm
> index a78728cd..a0c0339c 100644
> --- a/Osstest.pm
> +++ b/Osstest.pm
> @@ -227,6 +227,7 @@ sub readglobalconfig () {
>      $c{TftpTmpDir} ||= "$c{TftpPlayDir}tmp/";
>  
>      $c{TftpDiBase} ||= "$c{TftpPlayDir}debian-installer";
> +    $c{TftpFreeBSDBase} ||= "$c{TftpPlayDir}freebsd-installer";

You need to document this in README.

> +    foreach (@sets, "MANIFEST") {

I think it would be better to give this loop control variable a name.
When the scope of a use of $_ is too big, $_ has a tendency to get
trashed; this makes the code fragile in the face of future changes.

> +        if (!defined($r{"freebsd_sets"})) {
> +            # Get everything before the "." (ie: get base from base.txz)
> +            my $stash_name = lc((split /\./)[0]);

IMO that's not very idomatic perl.  I won't insist you change it, but
maybe this would be better

               my $stash_name = $set;
               $stash_name =~ s/[^.]+$//;

or

               $set =~ m/^[^.]+/ or die "$set ?";
               $stash_name = $&;

(Do you intend to split on the first `.' ?)

> +sub setup_netboot_installer () {
> +    my $pxeimg = "$ho->{Tftp}{FreeBSDBase}/install-$ho->{Name}.img";

I think this would be better under TftpTmpDir.  Cf ts-host-install's
placement of $ho--initrd.gz.

Maybe we want a function target_tftp_prefix or something which returns
   $ho->{Tftp}{TmpDir}".hostnamepath($ho)
?

Also the sha names could be in {TmpDir}/freebsd-images.  Does this, in
fact, need to be configurable at all ?  I think probably not.

> +    my $script = <<END;
> +set -ex
> +basedir=$ho->{Tftp}{Path}/$ho->{Tftp}{FreeBSDBase}/
> +hash=`sha256sum $image|head -c 64`
> +localpath="$r{arch}/\$hash/install.img";
> +mkdir -p \$basedir
> +(
> +flock -x -w 600 200

In osstest we generally use with-lock-ex from chiark-utils, rather
than flock from util-linux.  (Bonus: it should be more portable for if
someone wants to run a controller on BSD...)

with-lock-ex needs to be used in the wrapper way.
(Also, calling flock on a directory is rather outre'.)

You've written all this in bash because you found it too hard or
annoying in Perl ?  I don't much mind that, but the escaping is a bit
irritating.  Maybe you want to pass the script its parameters as
arguments so that you can use <<'END' rather than <<END ?

However,

> +    logm("Executing:\n$script");
> +    my $ret = system("/bin/bash -c '$script'");

This is quite wrong.  Your unparsing is unsound.  Amazingly your
script doesn't currently contain ' so this doesn't matter right now,
but that could change.

You should use the multi-argument form of system(3perl).  And, of
course, you should use Osstest::system_checked which does the error
handling for you.

You might want to build a command in an array @cmd, like copydir in
cr-publish-flight-logs does.  (Not sure why that doesn't use
system_checked...)

> +sub install () {
> +    my $authkeys = authorized_keys();
> +    my $knownhosts = known_hosts();
> +    my $sshd_keys_url = create_ssh_overlay();
> +    my $disk_names = "ada0 da0 ad0";

This should probably be
       my @disk_names = qw(ada0 da0 ad0);
in case anyone wants to manipulate it in perl.  You can substitute
it straight into the here document; perl will put in the " " again.

> +    my $installer_sets = join(" ", @sets);
> +    my $target_sets = "/tmp/osstest_sets";

Hardcoded /tmp antipattern.  Maybe this is technicallty OK because
it's an installer environment, but I think it sets a very bad
example.  Is there some other path you could use ?

> +    target_cmd_root($ho, 'chsh -s /bin/sh', 10);

!!  What's the default ?

> +for disk in $disk_names; do
> +    if [ -c "/dev/\$disk" ]; then
> +        echo \$disk
> +        exit 0
> +    fi
> +done
> +exit 1
> +END
> +    defined($disk) or die "Unable to find a valid disk";
> +    logm("Using $disk as destination disk device");

I have found that on some hosts, when installing Debian GNU/Linux, I
have to expect a nonstandard disk name.  Currently in the DB I can
only find this
   hydrazine          disk-device                    /dev/cciss/c0d0
(in Cambridge; referring to a gone-away machine; NB that the property
name is in the obsolete containing-spaces syntax and is equivalent to
DiskDevice.)

I think you may want to check a host property.  It should probably be
called Freebsd_DiskDevice or something.  (Weird capitalisation required
by the word splitting name transformation rules for host propertiess.)

> +    logm("Trying to figure out primary nic device name");
> +    $nic = target_cmd_output_root($ho, <<END, 30);
> +nics=`ifconfig -l`
> +for nic in \$nics; do
> +    addr=`ifconfig \$nic inet|grep inet|awk {'print \$2'}`
> +    if [ "\$addr" = "$ho->{Ip}" ]; then
> +        echo \$nic
> +        exit 0
> +    fi
> +done
> +exit 1

I have quite a lot of this kind of thing:

  gall-mite           'interface force'              eth0
  itch-mite           'interface force'              eth0
  grain-weevil        'interface force'              eth1
  rice-weevil         'interface force'              eth1
  arndale-bluewater   Interface_Force                eth0
  arndale-lakeside    Interface_Force                eth0
  arndale-metrocentre Interface_Force                eth0
  arndale-westfield   Interface_Force                eth0

This may not be needed for FreeBSD, but I thought I would mention it.

What would happen if you tried to run this setup on a host where
FreeBSD's idea of the first network interface is not the one which
osstest did the install on ?

IIRC there is a way for pxelinux to pass the NIC MAC address to the OS
it is running.  Also, the host database contains the MAC address of
the host (and this is generally necessary for osstest to work even in
standalone mode I think), so you might use that rather than the IP
address ?

> +    foreach (get_sets_path()) {
> +        target_putfile_root($ho, 600, $_->{path}, "$target_sets/$_->{name}"
);

Needs linewrap.

> +    logm("Creating the installer script");
> +    target_cmd_root($ho, <<END, 60);
> +        set -e
> +        cat << ENDSCRIPT > installscript

OK, my brain is fully bent now.  Can we not create installscript on
the controller and transfer it with target_putfilecontents_root_stash ?
That way the logs would contain a copy, too.

> +# Setup serial console
> +printf "%s" "-h -S$c{Baud}" >> /boot.config
> +cat << ENDBOOT >> /boot/loader.conf
> +boot_serial="YES"
> +comconsole_speed="$c{Baud}"
> +console="comconsole"
> +boot_verbose="YES"
> +beastie_disable="YES"
> +ENDBOOT

Where does the installer's output go ?  Ie, does booting the installer
image produce serial log output ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel