diff --git a/build-utils/lib/XCAT/BuildUtils.pm b/build-utils/lib/XCAT/BuildUtils.pm index 54df11487..3ca224908 100644 --- a/build-utils/lib/XCAT/BuildUtils.pm +++ b/build-utils/lib/XCAT/BuildUtils.pm @@ -463,12 +463,27 @@ 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 = $$; + return XCAT::BuildUtils::_BuildLock->new($lockdir, $owner); +} + +{ 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} } } # The rpm architecture a mock target builds for. A target carries the arch as its diff --git a/buildrpms.pl b/buildrpms.pl index e14bdd436..fe2768d0f 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; @@ -870,9 +870,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. @@ -929,6 +937,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 } @@ -958,9 +969,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" @@ -968,9 +980,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();