diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 54df11487..5eb897799 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -107,11 +107,62 @@ 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) = @_; + require POSIX; say "Running: $cmd" if $VERBOSE; - system($cmd); - return $? >> 8; + + # Do not let cancellation run between fork and publishing the process group. + my $blocked = POSIX::SigSet->new(POSIX::SIGINT(), POSIX::SIGTERM()); + my $oldmask = POSIX::SigSet->new(); + POSIX::sigprocmask(POSIX::SIG_BLOCK(), $blocked, $oldmask) + or die "Cannot block build cancellation signals: $!\n"; + + my $pid = fork(); + unless (defined $pid) { + my $error = "$!"; + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); + warn "FATAL: cannot fork to run $cmd: $error\n"; + return 127; + } + unless ($pid) { + $SIG{INT} = $SIG{TERM} = 'DEFAULT'; + POSIX::setpgid(0, 0) or POSIX::_exit(127); + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) + or POSIX::_exit(127); + exec('/bin/sh', '-c', $cmd) or POSIX::_exit(127); + } + + local $CURRENT_CHILD = $pid; # also the command's process-group ID + # Both sides set the group, so neither depends on which side runs first. + # EACCES means the child already exec'd, after setting its group; ESRCH + # means it has already gone away. + unless (POSIX::setpgid($pid, $pid) || $!{EACCES} || $!{ESRCH}) { + warn "FATAL: cannot create build process group $pid: $!; retaining locks\n"; + kill 'KILL', $pid; + POSIX::_exit(127); + } + POSIX::sigprocmask(POSIX::SIG_SETMASK(), $oldmask) or do { + warn "FATAL: cannot restore signal mask: $!\n"; + cancel_build('TERM'); + POSIX::_exit(127); + }; + + my $got; + do { $got = waitpid($pid, 0) } while ($got == -1 && $!{EINTR}); + my $status = $?; + return 127 if $got == -1; + return ($status & 127) ? 128 + ($status & 127) : $status >> 8; } # pod2usage reads the POD of the running program, so each builder keeps its own @@ -463,12 +514,160 @@ sub lock_path_for { # Returns the open handle -- the lock is held for as long as the caller keeps it. sub take_build_lock { my ($path, $dir) = @_; - require Fcntl; - my $lockfile = lock_path_for($path, $dir); - open my $fh, '>', $lockfile or die "FATAL: cannot open $lockfile: $!\n"; - flock($fh, Fcntl::LOCK_EX() | Fcntl::LOCK_NB()) - or die "FATAL: another build of $path already holds $lockfile\n"; - return $fh; + require POSIX; + # A directory, not an flock. A build tree can live on an NFS re-export, where the kernel + # refuses locks outright: every attempt answers errno 524. mkdir(2) is arbitrated by the + # server and needs no lock daemon. + my $lockdir = lock_path_for($path, $dir) . '.d'; + unless (mkdir $lockdir) { + die "FATAL: cannot take $lockdir: $!\n" unless $! == POSIX::EEXIST(); + my $who = ''; if (open(my $h, '<', "$lockdir/owner")) { local $/; $who = <$h> // ''; close $h } + chomp $who; + die "FATAL: another build of $path already holds $lockdir" + . ($who ? " (held by [$who])" : "") . "\n"; + } + 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 = $$; + 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; + # A second Ctrl-C must not interrupt cleanup and release the lock early. + local $SIG{INT} = 'IGNORE'; + local $SIG{TERM} = 'IGNORE'; + + if (my $pgid = $CURRENT_CHILD) { + my $reaped = 0; + for my $stop_signal ($sig, 'KILL') { + kill $stop_signal, -$pgid; + for (1 .. 50) { + unless ($reaped) { + my $got = waitpid($pgid, POSIX::WNOHANG()); + $reaped = 1 if $got == $pgid || ($got == -1 && $!{ECHILD}); + } + # The shell exiting is not enough: its workers may still exist. + if (!kill(0, -$pgid) && $!{ESRCH}) { + $CURRENT_CHILD = undef; + release_build_locks(); + return; + } + select undef, undef, undef, 0.1; + } + } + # Never let END/DESTROY unlock a checkout whose workers may still run. + warn "FATAL: build process group $pgid has not disappeared; retaining locks\n"; + POSIX::_exit(1); + } + release_build_locks(); + return; +} + +{ package XCAT::BuildUtils::_BuildLock; + 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/buildrpms.pl b/buildrpms.pl index df22aaea2..42b87bb7a 100755 --- a/buildrpms.pl +++ b/buildrpms.pl @@ -45,7 +45,7 @@ use FindBin qw($Bin); use lib "$Bin/build-utils/lib"; use XCAT::BuildUtils qw(git_revision source_date_epoch sh sh_or_die usage buildinfo_text write_script read_line targetarch_from_target); -use Fcntl qw(:flock); # per-target build lock (concurrency guard; see main()) +use POSIX (); # EEXIST, for the directory build lock (see main()) use Getopt::Long qw(GetOptions); use POSIX qw(strftime); use Parallel::ForkManager; @@ -876,9 +876,17 @@ sub merge_core_repos { # file and the cached chroot dirs left behind are normal mock state, not leaks.) my %MOCK_INFLIGHT; # ForkManager child pid => mock chroot (-r) name it is building my $ABORTING = 0; -my $BUILD_LOCK_FH; # per-target build flock; MUST stay file-scoped so the fd (and thus the - # lock) lives for the whole process. A lexical inside main()'s block would - # be DESTROYED at block exit -> lock released before any worker forks. +my $BUILD_LOCK_DIR; # per-target build lock, an atomic mkdir rather than an flock. The build + # tree can live on an NFS re-export, where the kernel refuses locks + # outright ("Clients are not allowed to get file locks or delegations from + # a reexport server"), so every flock there fails with errno 524. mkdir(2) + # is arbitrated by the server and needs no lock daemon. File-scoped so the + # release runs once, from END, for the whole process. +my $BUILD_LOCK_PID; # the pid that took it. ForkManager children inherit $BUILD_LOCK_DIR, and + # their END would release the PARENT's lock -- the flock this replaces + # could not be released by a child, and neither may this. +END { rmdir $BUILD_LOCK_DIR if defined $BUILD_LOCK_DIR and defined $BUILD_LOCK_PID and $$ == $BUILD_LOCK_PID } + my @CHILD_FAILURES; # idents (chroot names) of ForkManager children that exited non-zero # PIDs of running mock processes whose `-r ` matches one of @chroots. @@ -935,6 +943,9 @@ sub abort_builds { sweep_mock_mounts(@chroots); } warn "[buildrpms] abort cleanup done\n"; + # The re-raise below kills this process by signal, and END blocks do not run then. Release the + # build lock here or a killed build strands it for every later run. + rmdir $BUILD_LOCK_DIR if defined $BUILD_LOCK_DIR and defined $BUILD_LOCK_PID and $$ == $BUILD_LOCK_PID; $SIG{$sig} = 'DEFAULT'; kill $sig, $$; # re-raise for the correct exit status } @@ -964,9 +975,10 @@ sub main { my $key = join('-', $opts{targets}->@*) . ($opts{mock_uniqueext} ? "-$opts{mock_uniqueext}" : ""); $key =~ s/[^A-Za-z0-9._-]/-/g; - my $lock = "/var/lock/buildrpms.$key.lock"; - if (open($BUILD_LOCK_FH, '>', $lock)) { - unless (flock($BUILD_LOCK_FH, LOCK_EX | LOCK_NB)) { + my $lock = "/var/lock/buildrpms.$key.lock.d"; + { + unless (mkdir $lock) { + die "FATAL: cannot take the build lock $lock: $!\n" unless $! == POSIX::EEXIST(); die "FATAL: another buildrpms.pl is already building target '@{$opts{targets}}'" . ($opts{mock_uniqueext} ? " (uniqueext=$opts{mock_uniqueext})" : "") . ".\n" . " ($lock is held). Concurrent builds of the same target collide on the shared\n" @@ -974,9 +986,7 @@ sub main { . " cleanup unmounts this build's chroot). Serialize them, or pass a distinct\n" . " --mock-uniqueext per build.\n"; } - # $BUILD_LOCK_FH is file-scoped, so the fd stays open (lock held) until this process - # exits. Child forks inherit the fd but their exits never release it (the parent's - # still-open fd keeps the lock), which is exactly what we want. + $BUILD_LOCK_DIR = $lock; $BUILD_LOCK_PID = $$; } } diff --git a/xCAT-test/unit/builddebs_lock.t b/xCAT-test/unit/builddebs_lock.t index cba190133..f022fc08c 100644 --- a/xCAT-test/unit/builddebs_lock.t +++ b/xCAT-test/unit/builddebs_lock.t @@ -44,9 +44,11 @@ like( $@, qr/already holds/, 'and says which checkout is already building' ); my $stable = eval { take_build_lock('/opt/builds/stable/xcat-core', $lockdir) }; ok( $stable, 'a build of a DIFFERENT checkout runs concurrently' ); -# Releasing lets the next build in. -close $first; +# Releasing lets the next build in. The lock is a directory now, not an flock on a filehandle -- +# an NFS re-export refuses locks outright (errno 524) -- so it is freed when the returned object +# goes out of scope, not when a handle is closed. +undef $first; my $again = eval { take_build_lock($devel, $lockdir) }; -ok( $again, 'the lock is released when the handle is closed' ); +ok( $again, 'the lock is released when the returned value goes out of scope' ); done_testing(); diff --git a/xCAT-test/unit/builddebs_lock_cancellation.t b/xCAT-test/unit/builddebs_lock_cancellation.t new file mode 100644 index 000000000..37d9de525 --- /dev/null +++ b/xCAT-test/unit/builddebs_lock_cancellation.t @@ -0,0 +1,187 @@ +#!/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'") }; +} + +# --------------------------------------------------------------------------- +# 4. THE WHOLE PROCESS GROUP GOES, not just the shell. +# Killing /bin/sh does not kill what it started: dpkg-buildpackage leaves workers behind, and +# those keep writing the checkout after the lock would otherwise have been handed to the next +# build. The command runs in its own process group so cancellation can take all of it. +# --------------------------------------------------------------------------- +{ + my $tag = '661277'; # the worker, a grandchild of the build + my $started = "$lockdir/started4"; + 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 } + # a shell that spawns a worker and waits: the worker is a GRANDCHILD of this process + XCAT::BuildUtils::sh("sleep $tag & sleep $tag"); + 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 started'); + + my $workers = 0; + for (1 .. 100) { + $workers = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $workers; + last if $workers >= 2; + select undef, undef, undef, 0.1; + } + cmp_ok($workers, '>=', 2, 'the build has a worker of its own -- the control for the check below'); + + kill 'TERM' => $pid; + for (1 .. 100) { last if waitpid($pid, WNOHANG) > 0; select undef, undef, undef, 0.1 } + + my $left = 1; + for (1 .. 100) { + $left = `pgrep -f "[s]leep $tag" 2>/dev/null | wc -l`; chomp $left; + last if $left == 0; + select undef, undef, undef, 0.1; + } + is($left, 0, 'cancelling the build takes its workers with it, not just its shell') + or do { diag('a build worker outlived the cancellation and can still write the checkout'); + system("pkill -f '[s]leep $tag'") }; +} + +# --------------------------------------------------------------------------- +# 5. A command killed by a signal reports as killed, not as success. +# $? >> 8 is 0 for a signalled child, so a build stopped mid-way looked like it had worked. +# --------------------------------------------------------------------------- +{ + my $rc = -1; + my $pid = fork(); + die "cannot fork\n" unless defined $pid; + unless ($pid) { + my $got = XCAT::BuildUtils::sh("kill -TERM \$\$"); + POSIX::_exit($got); + } + waitpid($pid, 0); + $rc = $? >> 8; + is($rc, 128 + 15, 'a command killed by SIGTERM reports 128+15, not 0'); +} + + +done_testing();