diff mbox

[Xen-devel,osstest] apt: lock osstest's usages of apt-get against each other

Message ID 1398247181-19595-1-git-send-email-ijc@hellion.org.uk
State New
Headers show

Commit Message

Ian Campbell April 23, 2014, 9:59 a.m. UTC
From: Ian Campbell <ian.campbell@citrix.com>

Currently we rely on all apt-get invocations being in a single
ts-xen-build-prep job which can't run on a shared host.

That is a bit inflexible so instead use our own lock. We wait indefinitely and
rely on osstest's existing command timeout infrastructure to catch problems.

target_install_packages*() previous estimated the time taken to install the
packages based on the number of packages. This no longer applies because the
install might get stuck behind some other large install. Use a 3000s (nearly
an hour) timeout instead (I expect failures here to be unusual so erred on the
big side)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Debian.pm      | 2 +-
 Osstest/TestSupport.pm | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Ian Campbell April 23, 2014, 10 a.m. UTC | #1
On Wed, 2014-04-23 at 10:59 +0100, Ian Campbell wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Currently we rely on all apt-get invocations being in a single
> ts-xen-build-prep job which can't run on a shared host.
> 
> That is a bit inflexible so instead use our own lock. We wait indefinitely and
> rely on osstest's existing command timeout infrastructure to catch problems.
> 
> target_install_packages*() previous estimated the time taken to install the
> packages based on the number of packages. This no longer applies because the
> install might get stuck behind some other large install. Use a 3000s (nearly
> an hour) timeout instead (I expect failures here to be unusual so erred on the
> big side)
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

I would have sworn I had sent this out already, but I can't see any sign
of it... I've been testing this along with my libvirt patch, although it
doesn't strictly depend on it.

> ---
>  Osstest/Debian.pm      | 2 +-
>  Osstest/TestSupport.pm | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
> index 059858e..0f7bb85 100644
> --- a/Osstest/Debian.pm
> +++ b/Osstest/Debian.pm
> @@ -493,7 +493,7 @@ d-i apt-setup/another boolean false
>  d-i apt-setup/non-free boolean false
>  d-i apt-setup/contrib boolean false
>  
> -d-i pkgsel/include string openssh-server, ntp, ntpdate, $extra_packages
> +d-i pkgsel/include string openssh-server, ntp, ntpdate, chiark-utils-bin, $extra_packages
>  
>  $xopts{ExtraPreseed}
>  
> diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> index 461fe63..d49ff4f 100644
> --- a/Osstest/TestSupport.pm
> +++ b/Osstest/TestSupport.pm
> @@ -430,17 +430,18 @@ sub target_putfile_root ($$$$;$) {
>  sub target_run_apt {
>      my ($ho, $timeout, @aptopts) = @_;
>      target_cmd_root($ho,
> -   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y apt-get @aptopts",
> +   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y \
> +        with-lock-ex -w /var/lock/osstest-apt apt-get @aptopts",
>                      $timeout);
>  }
>  sub target_install_packages {
>      my ($ho, @packages) = @_;
> -    target_run_apt($ho, 300 + 100 * @packages,
> +    target_run_apt($ho, 3000,
>  		   qw(-y install), @packages);
>  }
>  sub target_install_packages_norec {
>      my ($ho, @packages) = @_;
> -    target_run_apt($ho, 300 + 100 * @packages,
> +    target_run_apt($ho, 3000,
>  		   qw(--no-install-recommends -y install), @packages);
>  }
>
Ian Jackson April 24, 2014, 11:18 a.m. UTC | #2
Ian Campbell writes ("[PATCH osstest] apt: lock osstest's usages of apt-get against each other"):
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Currently we rely on all apt-get invocations being in a single
> ts-xen-build-prep job which can't run on a shared host.
...
> @@ -430,17 +430,18 @@ sub target_putfile_root ($$$$;$) {
>  sub target_run_apt {
>      my ($ho, $timeout, @aptopts) = @_;

I think this needs to lose the timeout option completely.  If some new
caller were to come along and call "apt-get moo", it ought not to be
expected to pass a timeout.  If it did it would pass 30, one would
expect, but of course it might end up blocked behind a long run.

Ian.
Ian Campbell April 24, 2014, 11:52 a.m. UTC | #3
On Thu, 2014-04-24 at 12:18 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH osstest] apt: lock osstest's usages of apt-get against each other"):
> > From: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Currently we rely on all apt-get invocations being in a single
> > ts-xen-build-prep job which can't run on a shared host.
> ...
> > @@ -430,17 +430,18 @@ sub target_putfile_root ($$$$;$) {
> >  sub target_run_apt {
> >      my ($ho, $timeout, @aptopts) = @_;
> 
> I think this needs to lose the timeout option completely.  If some new
> caller were to come along and call "apt-get moo", it ought not to be
> expected to pass a timeout.  If it did it would pass 30, one would
> expect, but of course it might end up blocked behind a long run.

I think you mean this function should just pick a suitably large timeout
for everything, rather than have no timeout at all, is that right?

Ian.
Ian Jackson April 30, 2014, 2:43 p.m. UTC | #4
Ian Campbell writes ("Re: [PATCH osstest] apt: lock osstest's usages of apt-get against each other"):
> On Thu, 2014-04-24 at 12:18 +0100, Ian Jackson wrote:
> > I think this needs to lose the timeout option completely.  If some new
> > caller were to come along and call "apt-get moo", it ought not to be
> > expected to pass a timeout.  If it did it would pass 30, one would
> > expect, but of course it might end up blocked behind a long run.
> 
> I think you mean this function should just pick a suitably large timeout
> for everything, rather than have no timeout at all, is that right?

Exactly.

Ian.
diff mbox

Patch

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index 059858e..0f7bb85 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -493,7 +493,7 @@  d-i apt-setup/another boolean false
 d-i apt-setup/non-free boolean false
 d-i apt-setup/contrib boolean false
 
-d-i pkgsel/include string openssh-server, ntp, ntpdate, $extra_packages
+d-i pkgsel/include string openssh-server, ntp, ntpdate, chiark-utils-bin, $extra_packages
 
 $xopts{ExtraPreseed}
 
diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 461fe63..d49ff4f 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -430,17 +430,18 @@  sub target_putfile_root ($$$$;$) {
 sub target_run_apt {
     my ($ho, $timeout, @aptopts) = @_;
     target_cmd_root($ho,
-   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y apt-get @aptopts",
+   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y \
+        with-lock-ex -w /var/lock/osstest-apt apt-get @aptopts",
                     $timeout);
 }
 sub target_install_packages {
     my ($ho, @packages) = @_;
-    target_run_apt($ho, 300 + 100 * @packages,
+    target_run_apt($ho, 3000,
 		   qw(-y install), @packages);
 }
 sub target_install_packages_norec {
     my ($ho, @packages) = @_;
-    target_run_apt($ho, 300 + 100 * @packages,
+    target_run_apt($ho, 3000,
 		   qw(--no-install-recommends -y install), @packages);
 }