diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 3ca224908..4eacd6abb 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -107,10 +107,33 @@ sub rewrite_file { # status, which is the exit code times 256, so it is shifted here: a caller # comparing the result against a specific code gets the code it expects, not a # multiple of it. +# The pid of the command sh() is running, so a cancellation handler can stop it before the +# build lock is released. system() gives no pid, which is why this forks explicitly. +our $CURRENT_CHILD; + +# Locks this process holds, weakly. Released by a signal handler or at exit, because +# _BuildLock::DESTROY does not run when a signal ends the process. +our @LIVE_LOCKS; +END { release_build_locks() } + sub sh { my ($cmd) = @_; say "Running: $cmd" if $VERBOSE; - system($cmd); + my $pid = fork(); + unless (defined $pid) { + warn "FATAL: cannot fork to run: $cmd\n"; + return 127; + } + unless ($pid) { + # _exit, not exit: the child must not run the parent's END block and release a lock + # the parent still holds. + require POSIX; + exec('/bin/sh', '-c', $cmd) or POSIX::_exit(127); + } + local $CURRENT_CHILD = $pid; + # waitpid returns -1 with EINTR when a signal arrives, and a build takes signals. + my $got; + do { $got = waitpid($pid, 0) } while ($got == -1 && $!{EINTR}); return $? >> 8; } @@ -478,12 +501,131 @@ sub take_build_lock { if (open(my $ow, '>', "$lockdir/owner")) { print {$ow} "pid=$$\n"; close $ow } # The caller keeps the returned value; release is by pid so a fork cannot free the parent's. my $owner = $$; - return XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); + my $lock = XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); + # Registered weakly, so holding it here does not keep the lock alive past its caller's + # scope. The registry exists only so a signal can release what DESTROY will not. + require Scalar::Util; + push @LIVE_LOCKS, $lock; + Scalar::Util::weaken($LIVE_LOCKS[-1]); + return $lock; +} + +#------------------------------------------------------------------------------- + +=head3 release_build_locks + +Descriptions: + Release every build lock this process still holds. + + DESTROY does not run when a signal terminates the process, so a cancelled build left its + lock directory behind and the next build of that checkout died on "another build already + holds" naming a pid that had long exited. One such directory blocked an openSUSE target + across three consecutive runs before anyone looked. + +Arguments: + None. +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub release_build_locks { + for my $l (@LIVE_LOCKS) { $l->release if defined $l } + return; +} + +#------------------------------------------------------------------------------- + +=head3 install_build_cancellation + +Descriptions: + Install the INT and TERM handlers that stop the build and release its locks. + + It lives here rather than in the builder so the behaviour can be tested. A builder that + wired its own handler inline could only be covered by reading its source, and a test that + installs an equivalent handler of its own proves the helper works while saying nothing + about whether anything calls it. + + The signal is re-raised with the default disposition afterwards, so the exit status still + tells a caller the build was cancelled rather than that it failed. + +Arguments: + $announce - optional coderef called with the signal name before the build is stopped +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub install_build_cancellation { + my ($announce) = @_; + for my $sig (qw(INT TERM)) { + $SIG{$sig} = sub { + my ($caught) = @_; + $announce->($caught) if $announce; + cancel_build($caught); + $SIG{$caught} = 'DEFAULT'; + kill $caught => $$; + }; + } + return; +} + +#------------------------------------------------------------------------------- + +=head3 cancel_build + +Descriptions: + Stop the command in flight, then release the build locks. + + The order matters. Releasing first would hand the checkout to a second build while + dpkg-buildpackage is still rewriting debian/changelog and debian/control in it. + + The wait is bounded: a child that ignores the signal must not keep the lock for ever, so + it is given a few seconds and then killed outright. + +Arguments: + $sig - the signal name that started the cancellation +Returns: + Nothing. + +=cut + +#------------------------------------------------------------------------------- +sub cancel_build { + my ($sig) = @_; + require POSIX; + if ($CURRENT_CHILD) { + kill $sig => $CURRENT_CHILD; + my $gone = 0; + for (1 .. 50) { + if (waitpid($CURRENT_CHILD, POSIX::WNOHANG()) > 0) { $gone = 1; last } + select undef, undef, undef, 0.1; + } + unless ($gone) { + kill 'KILL' => $CURRENT_CHILD; + waitpid($CURRENT_CHILD, 0); + } + } + release_build_locks(); + return; } { package XCAT::BuildUtils::_BuildLock; - sub new { my ($c,$d,$p)=@_; return bless { dir=>$d, pid=>$p }, $c } - sub DESTROY { my $s=shift; return unless $$ == $s->{pid}; unlink "$s->{dir}/owner"; rmdir $s->{dir} } + sub new { my ($c,$d,$p)=@_; return bless { dir=>$d, pid=>$p, released=>0 }, $c } + # Idempotent: a signal handler and then DESTROY both reach here, and the second must not + # remove a directory a LATER build has since taken. + sub release { + my $s = shift; + return if $s->{released}; + $s->{released} = 1; + return unless $$ == $s->{pid}; + unlink "$s->{dir}/owner"; + rmdir $s->{dir}; + return; + } + sub DESTROY { shift->release } } # The rpm architecture a mock target builds for. A target carries the arch as its diff --git a/builddebs.pl b/builddebs.pl index def0fd89b..4c53ca722 100755 --- a/builddebs.pl +++ b/builddebs.pl @@ -335,6 +335,20 @@ SCRIPT } # ----------------------------------------------------------------- main ------ +# A cancelled build must not keep the checkout. The lock releases in DESTROY, which perl does +# not run when a signal ends the process, so a killed build left its directory behind and the +# next build of that checkout died on "another build already holds" naming a pid that had +# already exited. buildrpms.pl has released its lock on cancellation for some time; this is the +# Debian builder catching up. +# +# cancel_build stops the command in flight BEFORE releasing: handing the checkout to a second +# build while dpkg-buildpackage is still rewriting debian/changelog in it is worse than holding +# the lock a moment longer. +XCAT::BuildUtils::install_build_cancellation(sub { + my ($caught) = @_; + print STDERR "\n[builddebs] SIG$caught: stopping the build and releasing the lock\n"; +}); + my $lock = take_build_lock($ROOT); my $dest = resolve_dest($opts{dest}, "$ROOT/dist/debs"); diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t new file mode 100644 index 000000000..b2258306c --- /dev/null +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -0,0 +1,124 @@ +#!/usr/bin/env perl +# A KILLED BUILD MUST NOT KEEP THE CHECKOUT. +# +# The build lock is released in DESTROY, and perl does not run DESTROY when a signal ends the +# process. So a cancelled build left its lock directory behind, and the next build of that +# checkout died on +# FATAL: another build of already holds (held by [pid=NNNN]) +# naming a pid that had already exited. One such directory blocked an openSUSE target across +# three consecutive runs before anyone looked at it. +# +# buildrpms.pl has released its lock on cancellation for some time. This covers the Debian +# builder doing the same, and the ORDER it must do it in: the command in flight is stopped +# before the lock is released, because handing the checkout to a second build while +# dpkg-buildpackage is still rewriting debian/changelog in it is worse than holding the lock a +# moment longer. +use strict; +use warnings; +use Test::More; +use File::Temp qw(tempdir); +use POSIX qw(WNOHANG); +use FindBin; +use lib "$FindBin::Bin/../../build-utils/lib"; +use XCAT::BuildUtils (); + +my $lockdir = tempdir(CLEANUP => 1); +my $ckout = tempdir(CLEANUP => 1); + +# --------------------------------------------------------------------------- +# 1. The lock is taken, and a second taker is refused. Without this the test below could pass +# by the lock never having worked at all. +# --------------------------------------------------------------------------- +{ + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + ok($l, 'a build takes the checkout lock'); + my $path = XCAT::BuildUtils::lock_path_for($ckout, $lockdir) . '.d'; + ok(-d $path, 'the lock directory exists while it is held'); + my $second = eval { XCAT::BuildUtils::take_build_lock($ckout, $lockdir) }; + ok(!$second, 'a second build of the same checkout is refused'); + like($@, qr/already holds/, 'and told which lock is held'); + undef $l; + ok(!-d $path, 'releasing it removes the directory'); +} + +# --------------------------------------------------------------------------- +# 2. THE REGRESSION. Terminate the holder with SIGTERM, then take the lock again. +# --------------------------------------------------------------------------- +{ + my $path = XCAT::BuildUtils::lock_path_for($ckout, $lockdir) . '.d'; + my $ready = "$lockdir/ready"; + + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + # the child is the build: it takes the lock, says so, and waits to be killed + # exactly what builddebs.pl does -- not a hand-rolled equivalent, or this would pass + # with the wiring removed and prove nothing. + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $r, '>', $ready)) { close $r } + sleep 30; + POSIX::_exit(0); + } + + # wait for the child to actually hold it, rather than guessing with a sleep + my $held = 0; + for (1 .. 100) { if (-e $ready) { $held = 1; last } select undef, undef, undef, 0.1 } + ok($held, 'the build says it holds the lock'); + ok(-d $path, 'and the directory is there while it runs'); + + kill 'TERM' => $pid; + my $reaped = 0; + for (1 .. 100) { if (waitpid($pid, WNOHANG) > 0) { $reaped = 1; last } select undef, undef, undef, 0.1 } + ok($reaped, 'the build stops when it is terminated'); + + ok(!-d $path, 'a TERMINATED build leaves no lock behind'); + + # the point of all of it: the next build can run + my $next = eval { XCAT::BuildUtils::take_build_lock($ckout, $lockdir) }; + ok($next, 'the next build of that checkout takes the lock') + or diag("still refused: $@"); + undef $next; +} + +# --------------------------------------------------------------------------- +# 3. The command in flight is STOPPED, not orphaned. +# Checked from the parent, because the cancelled build re-raises the signal with the default +# disposition and so never reaches an END block of its own. A distinctive sleep makes the +# build subprocess findable; the pattern is bracketed so the search cannot match itself. +# --------------------------------------------------------------------------- +{ + my $started = "$lockdir/started3"; + my $marker = '778349'; # nothing else on this host sleeps for this long + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + XCAT::BuildUtils::install_build_cancellation(); + my $l = XCAT::BuildUtils::take_build_lock($ckout, $lockdir); + if (open(my $st, '>', $started)) { close $st } + XCAT::BuildUtils::sh("sleep $marker"); + POSIX::_exit(0); + } + my $up = 0; + for (1 .. 100) { if (-e $started) { $up = 1; last } select undef, undef, undef, 0.1 } + ok($up, 'the build subprocess is running'); + my $live = `pgrep -f "[s]leep $marker" 2>/dev/null | wc -l`; chomp $live; + cmp_ok($live, '>', 0, 'and the test can see it -- the control for the check below'); + + kill 'TERM' => $pid; + my $reaped = 0; + for (1 .. 100) { if (waitpid($pid, WNOHANG) > 0) { $reaped = 1; last } select undef, undef, undef, 0.1 } + ok($reaped, 'the cancelled build exits'); + + my $left = 1; + for (1 .. 50) { + $left = `pgrep -f "[s]leep $marker" 2>/dev/null | wc -l`; chomp $left; + last if $left == 0; + select undef, undef, undef, 0.1; + } + is($left, 0, 'the build subprocess was stopped, not left running without its lock') + or do { diag('orphaned build subprocess still holds the checkout'); + system("pkill -f '[s]leep $marker'") }; +} + +done_testing();