[OSSTEST PATCH 0/7] Fix Debian HVM ssh ECONNREFUSED race

[OSSTEST PATCH 0/7] Fix Debian HVM ssh ECONNREFUSED race

[Ian Jackson]

This race seems actually to exist in principle in all of our hosts and
guests.

The first patch nobbles it for all installs based on preseeding, by
overwriting the unnecessary and problematic hook script in our
overlay.

The remaining patches arrange to install the overlay in
ts-debian-fixup, which is called for installs done with
xen-create-image rather than d-i.

[OSSTEST PATCH 1/7] Debian installs: Nobble /etc/network/if-up.d/openssh-server

[Ian Jackson]

(See the comment in the new file for the explanation.)

This change affects all our Debian installs (both hosts and guests)
which are done with preseeding, because preseed_base() arranges to
install overlay/.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 overlay/etc/network/if-up.d/openssh-server |   32 ++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100755 overlay/etc/network/if-up.d/openssh-server

diff --git a/overlay/etc/network/if-up.d/openssh-server b/overlay/etc/network/if-up.d/openssh-server
new file mode 100755
index 0000000..9fe2faf
--- /dev/null
+++ b/overlay/etc/network/if-up.d/openssh-server
@@ -0,0 +1,32 @@
+#!/bin/sh
+exit 0
+
+# In a default Debian install, this script reloads (or, in some
+# versions of Debian, restarts) sshd as new network interfaces come
+# up.  This is in case you have specific listen addresses specified in
+# the config.
+#
+# But the default config listens on 0.0.0.0 and ::.  So sshd is active
+# as soon as an interface is up, and does not need to be restarted or
+# reloaded.
+#
+# This restarting or reloading is harmful because it causes ssh to
+# stop listening briefly.  We can see the following race:
+#
+#  target sshd       target dhcp client     osstest controller
+#
+#   starts            starts
+#   binds to ANY      obtains lease
+#                     configures eth0
+#                                            connects to :22 with nc
+#   accepts conn                             nc succeeds
+#                                            decides target sshd is up
+#                     runs ifup hook
+#                     ifup hook reloads
+#
+#   gets SIGHUP
+#   closes listen socket
+#   rereads config                           runs ssh root@target
+#                                            ssh gets ECONNREFUSED
+#   opens new listen socket
+#                                            declares test fail
-- 
1.7.10.4

Re: [OSSTEST PATCH 1/7] Debian installs: Nobble /etc/network/if-up.d/openssh-server

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> (See the comment in the new file for the explanation.)
> 
> This change affects all our Debian installs (both hosts and guests)
> which are done with preseeding, because preseed_base() arranges to
> install overlay/.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 2/7] TestSupport::open_unique_stashfile: Provide a RDWR filehandle

[Ian Jackson]

The caller can then rewind and reread it if they feel like.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/TestSupport.pm |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 2b67e32..7b9d815 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1005,7 +1005,7 @@ sub open_unique_stashfile ($) {
     my $dh;
     for (;;) {
         my $df= $$leafref;
-        $dh= new IO::File "$stash/$df", O_WRONLY|O_EXCL|O_CREAT;
+        $dh= new IO::File "$stash/$df", O_RDWR|O_EXCL|O_CREAT;
         last if $dh;
         die "$df $!" unless $!==&EEXIST;
         next_unique_name $leafref;
-- 
1.7.10.4

Re: [OSSTEST PATCH 2/7] TestSupport::open_unique_stashfile: Provide a RDWR filehandle

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> The caller can then rewind and reread it if they feel like.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 3/7] TestSupport: Honour $stdin fh argument to cmd, tcmd and tcmdex

[Ian Jackson]

These are internal functions, so the change is entirely within
TestSupport.  All the call sites are adjusted to pass undef so there
is no functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/TestSupport.pm |   27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 7b9d815..91829a0 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -323,9 +323,13 @@ sub target_adjust_timeout ($$) {
 #---------- running commands eg on targets ----------
 
 sub cmd {
-    my ($timeout,$stdout,@cmd) = @_;
+    my ($timeout,$stdin,$stdout,@cmd) = @_;
     my $child= fork;  die $! unless defined $child;
     if (!$child) {
+        if (defined $stdin) {
+            open STDIN, '<&', $stdin
+                or die "STDIN $stdin $cmd[0] $!";
+        }
         if (defined $stdout) {
             open STDOUT, '>&', $stdout
                 or die "STDOUT $stdout $cmd[0] $!";
@@ -395,19 +399,20 @@ sub sshopts () {
 }
 
 sub tcmdex {
-    my ($timeout,$stdout,$cmd,$optsref,@args) = @_;
+    my ($timeout,$stdin,$stdout,$cmd,$optsref,@args) = @_;
     logm("executing $cmd ... @args");
     # We use timeout(1) as a backstop, in case $cmd doesn't die.  We
     # need $cmd to die because we won't release the resources we own
     # until all of our children are dead.
-    my $r= cmd($timeout,$stdout, 'timeout',$timeout+30, $cmd,@$optsref,@args);
+    my $r= cmd($timeout,$stdin,$stdout,
+	       'timeout',$timeout+30, $cmd,@$optsref,@args);
     $r and die "status $r";
 }
 
 sub tgetfileex {
     my ($ruser, $ho,$timeout, $rsrc,$ldst) = @_;
     unlink $ldst or $!==&ENOENT or die "$ldst $!";
-    tcmdex($timeout,undef,
+    tcmdex($timeout,undef,undef,
            'scp', sshopts(),
            sshuho($ruser,$ho).":$rsrc", $ldst);
 } 
@@ -424,12 +429,12 @@ sub tputfileex {
     my ($ruser, $ho,$timeout, $lsrc,$rdst, $rsync) = @_;
     my @args= ($lsrc, sshuho($ruser,$ho).":$rdst");
     if (!defined $rsync) {
-        tcmdex($timeout,undef,
+        tcmdex($timeout,undef,undef,
                'scp', sshopts(),
                @args);
     } else {
         unshift @args, $rsync if length $rsync;
-        tcmdex($timeout,undef,
+        tcmdex($timeout,undef,undef,
                'rsync', [ '-e', 'ssh '.join(' ',@{ sshopts() }) ],
                @args);
     }
@@ -625,19 +630,19 @@ sub target_await_down ($$) {
 }    
 
 sub tcmd { # $tcmd will be put between '' but not escaped
-    my ($stdout,$user,$ho,$tcmd,$timeout,$extrasshopts) = @_;
+    my ($stdin,$stdout,$user,$ho,$tcmd,$timeout,$extrasshopts) = @_;
     $timeout=30 if !defined $timeout;
     target_adjust_timeout($ho,\$timeout);
-    tcmdex($timeout,$stdout,
+    tcmdex($timeout,$stdin,$stdout,
            'ssh', sshopts(), @{ $extrasshopts || [] },
            sshuho($user,$ho), $tcmd);
 }
-sub target_cmd ($$;$$) { tcmd(undef,'osstest',@_); }
-sub target_cmd_root ($$;$$) { tcmd(undef,'root',@_); }
+sub target_cmd ($$;$$) { tcmd(undef,undef,'osstest',@_); }
+sub target_cmd_root ($$;$$) { tcmd(undef,undef,'root',@_); }
 
 sub tcmdout {
     my $stdout= IO::File::new_tmpfile();
-    tcmd($stdout,@_);
+    tcmd(undef,$stdout,@_);
     $stdout->seek(0,0) or die "$stdout $!";
     my $r;
     { local ($/) = undef;
-- 
1.7.10.4

Re: [OSSTEST PATCH 3/7] TestSupport: Honour $stdin fh argument to cmd, tcmd and tcmdex

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> These are internal functions, so the change is entirely within
> TestSupport.  All the call sites are adjusted to pass undef so there
> is no functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 4/7] TestSupport: Provide target_cmd_inputfh_root

[Ian Jackson]

No caller yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/TestSupport.pm |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 91829a0..3fc8e15 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -50,6 +50,7 @@ BEGIN {
 
                       target_cmd_root target_cmd target_cmd_build
                       target_cmd_output_root target_cmd_output
+                      target_cmd_inputfh_root
                       target_getfile target_getfile_root
                       target_putfile target_putfile_root
                       target_putfilecontents_stash
@@ -655,6 +656,11 @@ sub tcmdout {
 sub target_cmd_output ($$;$) { tcmdout('osstest',@_); }
 sub target_cmd_output_root ($$;$) { tcmdout('root',@_); }
 
+sub target_cmd_inputfh_root ($$$;$$) {
+    my ($tho,$stdinfh,$tcmd,@rest) = @_;
+    tcmd($stdinfh,undef,'root',$tho,$tcmd,@rest);
+}
+
 sub poll_loop ($$$&) {
     my ($maxwait, $interval, $what, $code) = @_;
     # $code should return undef when all is well
-- 
1.7.10.4

Re: [OSSTEST PATCH 4/7] TestSupport: Provide target_cmd_inputfh_root

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> No caller yet.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  Osstest/TestSupport.pm |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> index 91829a0..3fc8e15 100644
> --- a/Osstest/TestSupport.pm
> +++ b/Osstest/TestSupport.pm
> @@ -50,6 +50,7 @@ BEGIN {
>  
>                        target_cmd_root target_cmd target_cmd_build
>                        target_cmd_output_root target_cmd_output
> +                      target_cmd_inputfh_root
>                        target_getfile target_getfile_root
>                        target_putfile target_putfile_root
>                        target_putfilecontents_stash
> @@ -655,6 +656,11 @@ sub tcmdout {
>  sub target_cmd_output ($$;$) { tcmdout('osstest',@_); }
>  sub target_cmd_output_root ($$;$) { tcmdout('root',@_); }
>  
> +sub target_cmd_inputfh_root ($$$;$$) {
> +    my ($tho,$stdinfh,$tcmd,@rest) = @_;
> +    tcmd($stdinfh,undef,'root',$tho,$tcmd,@rest);
> +}
> +
>  sub poll_loop ($$$&) {
>      my ($maxwait, $interval, $what, $code) = @_;
>      # $code should return undef when all is well

[OSSTEST PATCH 5/7] Debian preseed: Break out debian_overlays

[Ian Jackson]

We are going to want to handle the overlays elswhere too, so factor
out the iteration over them.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 Osstest/Debian.pm |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index a158f34..47d1767 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -34,6 +34,7 @@ BEGIN {
     $VERSION     = 1.00;
     @ISA         = qw(Exporter);
     @EXPORT      = qw(debian_boot_setup
+                      debian_overlays
                       %preseed_cmds
                       preseed_base
                       preseed_create
@@ -775,14 +776,23 @@ echo COMPRESS=/usr/sbin/osstest-initramfs-gzip >> \\
 END
 }
 
+sub debian_overlays ($) {
+    my ($func) = @_;
+    $func->($c{OverlayLocal}, 'overlay-local.tar');
+    $func->('overlay', 'overlay.tar');
+}
+
 sub preseed_base ($$$$;@) {
     my ($ho,$suite,$sfx,$extra_packages,%xopts) = @_;
 
     $xopts{ExtraPreseed} ||= '';
 
     preseed_ssh($ho, $sfx);
-    preseed_hook_overlay($ho, $sfx, $c{OverlayLocal}, 'overlay-local.tar');
-    preseed_hook_overlay($ho, $sfx, 'overlay', 'overlay.tar');
+
+    debian_overlays(sub {
+	my ($srcdir, $tfilename) = @_;
+	preseed_hook_overlay($ho, $sfx, $srcdir, $tfilename);
+    });
 
     my $preseed = <<"END";
 d-i debian-installer/locale string en_GB
-- 
1.7.10.4

Re: [OSSTEST PATCH 5/7] Debian preseed: Break out debian_overlays

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> We are going to want to handle the overlays elswhere too, so factor

"elsewhere"

> out the iteration over them.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 6/7] ts-debian-fixup: Put "/mnt" in a Perl variable

[Ian Jackson]

No functional change now, but this will allow us to change the
mountpoint.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ts-debian-fixup |   14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/ts-debian-fixup b/ts-debian-fixup
index beae049..6d24587 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -50,15 +50,17 @@ sub ether () {
     store_runvar("$gho->{Guest}_tcpcheckport", 22);
 }
 
+our $mountpoint = '/mnt';
+
 sub access () {
     guest_umount_lv($ho, $gho);
     target_cmd_root($ho, <<END);
         set -ex
-        mount /dev/$gho->{Vg}/$gho->{Lv} /mnt
+        mount /dev/$gho->{Vg}/$gho->{Lv} $mountpoint
         perl -i~ -pe "s/^root:[^:]+:/root::/" /etc/shadow
-        mkdir -p /mnt/root/.ssh /mnt/etc/ssh
-        cp -a /root/.ssh/* /mnt/root/.ssh/.
-        cp -a /etc/ssh/ssh_host_*key* /mnt/etc/ssh/.
+        mkdir -p $mountpoint/root/.ssh $mountpoint/etc/ssh
+        cp -a /root/.ssh/* $mountpoint/root/.ssh/.
+        cp -a /etc/ssh/ssh_host_*key* $mountpoint/etc/ssh/.
 END
 }
 
@@ -66,7 +68,7 @@ our $extra;
 
 sub console () {
     my $console=
-        target_kernkind_console_inittab($ho,$gho,"/mnt");
+        target_kernkind_console_inittab($ho,$gho,"$mountpoint");
     return unless length $console;
     
     my $xextra= "console=$console earlyprintk=xen";
@@ -97,7 +99,7 @@ sub filesystems () {
         set -ex
         perl -i~ -pe "
             s,^(/dev/)sda(\\\\d+),\\\${1}$rootdev\\\$2,;
-        " /mnt/etc/fstab
+        " $mountpoint/etc/fstab
 END
 }
 
-- 
1.7.10.4

Re: [OSSTEST PATCH 6/7] ts-debian-fixup: Put "/mnt" in a Perl variable

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> No functional change now, but this will allow us to change the
> mountpoint.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[OSSTEST PATCH 7/7] ts-debian-fixup: Install the overlays

[Ian Jackson]

We want debootstrap-installed guests to get these overlays too.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 ts-debian-fixup |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/ts-debian-fixup b/ts-debian-fixup
index 6d24587..3261185 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -19,6 +19,7 @@ use strict qw(vars);
 use DBI;
 use Osstest;
 use Osstest::TestSupport;
+use Osstest::Debian;
 
 tsreadconfig();
 
@@ -64,6 +65,18 @@ sub access () {
 END
 }
 
+sub overlay ($$) {
+    my ($srcdir, $tfilename) = @_;
+
+    my $leaf = "$gho->{Guest}-$tfilename";
+    my $fh = open_unique_stashfile(\$leaf);
+    contents_make_cpio($fh,'ustar',$srcdir);
+    seek $fh,0,0 or die "$leaf: $!";
+    target_cmd_inputfh_root($ho, $fh, <<END);
+        tar -C $mountpoint -xf - # $leaf
+END
+}
+
 our $extra;
 
 sub console () {
@@ -175,6 +188,7 @@ savecfg();
 ether();
 target_kernkind_check($gho);
 access();
+debian_overlays(\&overlay);
 console();
 filesystems();
 otherfixupcfg();
-- 
1.7.10.4

Re: [OSSTEST PATCH 7/7] ts-debian-fixup: Install the overlays

[Ian Campbell]

On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> We want debootstrap-installed guests to get these overlays too.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'm not especially sure it is necessary to stash the tarball, we don't for
HVM or other d-i based installs (AFAICS), but it's done now and no point
undoing it.

Ian.

Re: [OSSTEST PATCH 7/7] ts-debian-fixup: Install the overlays

[Ian Jackson]

Ian Campbell writes ("Re: [OSSTEST PATCH 7/7] ts-debian-fixup: Install the overlays"):
> On Tue, 2015-09-29 at 14:37 +0100, Ian Jackson wrote:
> > Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'm not especially sure it is necessary to stash the tarball, we don't for
> HVM or other d-i based installs (AFAICS), but it's done now and no point
> undoing it.

I guess I could have used tmpfile.

Ian.