|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/7] osstest: add a FreeBSD host install recipe
On Tue, May 23, 2017 at 05:04:10PM +0100, Ian Jackson wrote:
> 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
Done.
> > 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.
(not needed anymore since we got rid of this).
> > + 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.
Done.
>
> > + 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 `.' ?)
I don't expect this names to ever have more than one dot, this is not
something that changes, so there should really be only one dot (or
none in the MANIFEST case).
I prefer the second one because it strips the ".", while the first one
doesn't.
(as you probably already guessed my perl/regexp is not very good)
>
> > +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)
> ?
Sounds fine, I guess I will add this as a pre-patch since there's already a
consumer in ts-host-install.
> Also the sha names could be in {TmpDir}/freebsd-images. Does this, in
> fact, need to be configurable at all ? I think probably not.
OK, so then I will just drop FreeBSDBase and just use
$ho->{Tftp}{TmpDir}/freebsd-images. I don't think there's a lot of
value in have it in a standard folder, since those are just random
builds. I guess this makes more sense for Debian because they are
actual releases, and thus can be used for other stuff also?
> > + 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'.)
I don't have any problems using with-lock-ex, I just didn't know it
existed :).
> 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 ?
Hm, yes, I wasn't really confident of writing this in perl and being
sure that the lock was always released. I'm more confident with shell.
> 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...)
I've changed this to use an array with all the parameters and
system_checked, it looks much better now IMHO, thanks.
> > +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 ?
I'm open to suggestions. We could also use ~/osstest_sets. I don't
really have a preference. I've used /tmp because I though it would be
less controversial.
> > + target_cmd_root($ho, 'chsh -s /bin/sh', 10);
>
> !! What's the default ?
csh.
> > +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.)
Yes, I can do that, but we would have to fill the DB manually. Would
you be OK with leaving this as-is in this patch and me adding the
property fetching later on?
> > + 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 ?
FreeBSD doesn't really have an idea of the first network interface
(unless you count the order in the output from ifconfig -l).
Also, on FreeBSD each driver has it's own device, with different name,
ie: there are em0, em1, bge0, bce0... interfaces. We could add
a host property like Freebsd_NicDevice, but this fallback should stay
in case the parameter is not defined?
> 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 ?
This is not suitable in this case because from FreeBSD installer PoV
it has been booted from a disk (because memdisk is used), so none of
the PXE variables are propagated.
> > + 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.
Yes, that's much better, thanks!
> > +# 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 ?
Yes, the installer image is built with serial output already.
The output of the installer itself (bsdinstall) goes to the job log
file.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |