xCAT/xcat-core
2
0
Fork 0
mirror of https://github.com/xcat2/xcat-core.git synced 2026-09-28 16:20:51 +00:00
Code Issues Releases Wiki Activity

Merge pull request #7867 from VersatusHPC/fix/build-locks-on-shared-tree

Browse Source 
fix(xcat-core): build locks the shared tree refuses to grant
This commit is contained in:
Daniel Hilst
2026-09-24 10:19:22 -03:00
committed by GitHub
parent 7f635afbd8 ed28770532
commit ba49107db3
5 changed files with 433 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
build-utils/lib/XCAT/BuildUtils.pm
+207 -8
View File
@@ -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
builddebs.pl
+14
View File
@@ -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");
buildrpms.pl
+20 -10
View File
@@ -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 <chroot>` 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 = $$;
}
}
xCAT-test/unit/builddebs_lock.t
+5 -3
View File
@@ -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();
xCAT-test/unit/builddebs_lock_cancellation.t
+187
View File
@@ -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 <path> already holds <dir> (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();