diff --git a/build-ubunturepo b/build-ubunturepo index a65cd75bc..bc92165ee 100755 --- a/build-ubunturepo +++ b/build-ubunturepo @@ -35,6 +35,15 @@ # # DEST= - provide a directory to contains the build result # +# Running builds in parallel on one host: the build lock is scoped to the source checkout (see +# the "build-lock" block below), so two builds from DIFFERENT +# checkouts -- e.g. the devel and stable Ubuntu CD lanes -- run +# concurrently, while two builds of the SAME checkout still fail-fast +# (they build in-place and would corrupt each other). For parallel +# builds give each a separate checkout and a separate output tree +# (a distinct DEST). GPG_HOME may be SHARED between parallel builds +# -- it is used read-only for signing. +# # For the dependency packages 1. All the xcat dependency deb packages should be uploaded to # "pokgsa/projects/x/xcat/build/ubuntu/xcat-dep/debs/" on GSA # 2. run ./build-ubunturepo -d @@ -155,13 +164,33 @@ old_pwd=`pwd` cd `dirname $0` curdir=`pwd` -# Use flock to only one person build at the same time -# Get a lock, so can not do 2 builds at once -exec 8>/var/lock/xcatbld.lock +# Scope the build lock to THIS checkout. build-ubunturepo builds the packages in-place +# in its own source tree (it rewrites debian/changelog and debian/control, drops +# *.orig.tar.gz at the checkout root and runs dpkg-buildpackage inside the package +# dirs), so the resource two builds actually contend for is the checkout -- not the +# host. The historic single /var/lock/xcatbld.lock was host-global and fail-fast, so +# two builds from *different* checkouts (e.g. the devel and stable Ubuntu CD lanes on +# one build host) collided and the loser failed the pipeline even though they share +# nothing. Key the lock on the checkout path instead: builds of the SAME checkout +# still fail-fast (they would corrupt each other in-place), while builds of DISTINCT +# checkouts get distinct locks and run in parallel. The lock file stays on the local +# /var/lock (reliable flock; the checkout may live on NFS/virtiofs where flock is not) +# and the source tree is left byte-pristine. +# +# NOTE: the two marked regions below are extracted verbatim and exercised by the unit +# test xCAT-test/unit/build_ubunturepo_lock.t (which runs them with a chosen $curdir) +# -- keep the markers, and keep each region self-contained. +# BEGIN build-lock-id +lock_id_for() { printf '%s' "$1" | md5sum | cut -c1-12; } +LOCKFILE="/var/lock/xcatbld-$(lock_id_for "$curdir").lock" +# END build-lock-id +# BEGIN build-lock-acquire +exec 8>"$LOCKFILE" if ! flock -n 8; then - echo "ERROR: Can't get lock /var/lock/xcatbld.lock. Someone else must be doing a build right now. Exiting...." + echo "ERROR: Can't get lock $LOCKFILE for checkout $curdir. Another build is already using this checkout. Exiting...." exit 1 fi +# END build-lock-acquire # for the git case, query the current branch and set REL (changing master to devel if necessary) function setbranch { diff --git a/xCAT-test/unit/build_ubunturepo_lock.t b/xCAT-test/unit/build_ubunturepo_lock.t new file mode 100644 index 000000000..8410fdfff --- /dev/null +++ b/xCAT-test/unit/build_ubunturepo_lock.t @@ -0,0 +1,155 @@ +#!/usr/bin/env perl +# +# Unit test for the build-ubunturepo build lock (issue VersatusHPC/xcat-core#52). +# +# build-ubunturepo builds its packages in-place in its own source checkout, so the +# resource two concurrent builds contend for is the checkout -- not the host. The lock +# is therefore keyed on the checkout path ($curdir): builds of the SAME checkout +# fail-fast (they would corrupt each other), builds of DISTINCT checkouts get distinct +# locks and run in parallel (this is what lets the devel and stable Ubuntu CD lanes +# build concurrently on one host). +# +# The test extracts the two marked regions from build-ubunturepo VERBATIM so it +# exercises the real code, not a copy: +# * build-lock-id -- derives LOCKFILE from $curdir (pure string computation) +# * build-lock-acquire -- opens fd 8 on LOCKFILE and flock -n's it (needs a writable +# lock dir; skipped with a diag where /var/lock isn't writable) + +use strict; +use warnings; + +use FindBin; +use File::Temp qw(tempdir tempfile); +use IPC::Open2; +use Test::More; + +my $script = "$FindBin::Bin/../../build-ubunturepo"; +ok( -f $script, "found build-ubunturepo at $script" ) + or BAIL_OUT("build-ubunturepo not found"); + +my $src = do { local ( @ARGV, $/ ) = $script; <> }; + +my $workdir = tempdir( CLEANUP => 1 ); # scratch for temp scripts + capture files + +# --- pull the two marked regions out of the script verbatim ----------------- +sub region { + my ($name) = @_; + my ($body) = $src =~ /^# BEGIN \Q$name\E\n(.*?)^# END \Q$name\E\n/ms; + ok( defined $body, "extracted the '$name' region from build-ubunturepo" ) + or BAIL_OUT("marker region '$name' missing -- did the lock block change?"); + return $body; +} +my $id_region = region('build-lock-id'); +my $acquire_region = region('build-lock-acquire'); + +# Write a self-contained shell program (the given region(s) + a tail) to a temp file +# and return its path. Using a file avoids any quoting of the extracted shell. +my $prog_seq = 0; +sub write_prog { + my ($body) = @_; + my $path = "$workdir/prog." . $prog_seq++ . ".sh"; + open( my $fh, '>', $path ) or die "write $path: $!"; + print $fh "set -eu\ncurdir=\"\$1\"\n$body"; + close $fh; + return $path; +} + +sub lockfile_for { + my ($curdir) = @_; + my $prog = write_prog( "$id_region\nprintf '%s\\n' \"\$LOCKFILE\"\n" ); + my $out = qx{bash --noprofile --norc "$prog" "$curdir"}; + chomp $out; + return $out; +} + +# --- LOCKFILE derivation (always runnable -- no filesystem writes) ----------- +subtest 'lock is scoped per checkout, under /var/lock' => sub { + my $a = tempdir( CLEANUP => 1 ); + my $b = tempdir( CLEANUP => 1 ); + my $la = lockfile_for($a); + my $lb = lockfile_for($b); + + like( $la, qr{^/var/lock/xcatbld-[0-9a-f]{12}\.lock$}, + 'LOCKFILE is /var/lock/xcatbld-.lock' ); + is( lockfile_for($a), $la, 'same checkout => same lock (deterministic)' ); + isnt( $la, $lb, 'distinct checkouts => distinct locks' ); + + # the derivation must not create anything inside the checkout itself + ok( !glob("$a/*") && !glob("$a/.*xcatbld*"), + 'source checkout is left byte-pristine (no lock file written into it)' ); +}; + +# --- flock contention (needs a writable lock dir) --------------------------- +# The acquire region hard-codes /var/lock; run it only where that is writable +# (the GitHub Actions runner's /run/lock is sticky world-writable). Elsewhere, +# skip these two with a diag rather than failing on the environment. +my $probe = "/var/lock/.xcatbld-selftest.$$"; +my $lock_writable = open( my $pf, '>', $probe ); +if ($lock_writable) { close $pf; unlink $probe; } + +SKIP: { + skip "/var/lock is not writable here -- flock contention subtests need it", 2 + unless $lock_writable; + + # Launch a holder that acquires the lock for $curdir and blocks (holding fd 8) + # until we send it a newline on stdin. Returns ($pid, $to_child, $from_child, $first). + my $hold_prog = write_prog( + "$id_region\n$acquire_region\nprintf 'ACQUIRED %s\\n' \"\$LOCKFILE\"\nIFS= read -r _ || true\n" + ); + my $holder = sub { + my ($curdir) = @_; + my $pid = open2( my $out, my $in, 'bash', '--noprofile', '--norc', + $hold_prog, $curdir ); + my $first = <$out>; # blocks until the holder has the lock (or died) + return ( $pid, $in, $out, $first ); + }; + + # A contender that tries to acquire and, if it gets past the lock, prints MARK. + my $try_prog = sub { + my ($mark) = @_; + return write_prog( + "$id_region\n$acquire_region\nprintf '%s\\n' '$mark'\n" ); + }; + my $run = sub { + my ( $prog, $curdir ) = @_; + my $cap = "$workdir/cap." . $prog_seq++ . ".out"; + my $rc = system("bash --noprofile --norc \"$prog\" \"$curdir\" >\"$cap\" 2>&1"); + my $out = do { local ( @ARGV, $/ ) = $cap; <> }; + $out = '' unless defined $out; + return ( $rc, $out ); + }; + + subtest 'same checkout: second build fails fast' => sub { + my $dir = tempdir( CLEANUP => 1 ); + my ( $pid, $in, $out, $first ) = $holder->($dir); + like( $first, qr/^ACQUIRED /, 'first build acquired the checkout lock' ) + or BAIL_OUT('holder never acquired -- cannot test contention'); + + my ( $rc, $output ) = $run->( $try_prog->('SHOULD_NOT_REACH'), $dir ); + isnt( $rc, 0, 'second build of the SAME checkout exits non-zero' ); + like( $output, qr/Can't get lock/, 'it reports the lock contention' ); + like( $output, qr/\Q$dir\E/, 'the error names the contended checkout' ); + unlike( $output, qr/SHOULD_NOT_REACH/, 'it did not proceed into the build' ); + + print $in "\n"; close $in; # release the holder + waitpid( $pid, 0 ); + }; + + subtest 'distinct checkouts: both build in parallel' => sub { + my $dir_a = tempdir( CLEANUP => 1 ); + my $dir_b = tempdir( CLEANUP => 1 ); + my ( $pid, $in, $out, $first ) = $holder->($dir_a); + like( $first, qr/^ACQUIRED /, 'checkout A acquired its lock' ); + + # while A still holds its lock, B (a different checkout) must acquire too + my ( $rc, $output ) = $run->( $try_prog->('B_ACQUIRED'), $dir_b ); + is( $rc, 0, 'the other checkout acquires concurrently (exit 0)' ); + like( $output, qr/B_ACQUIRED/, 'it ran past the lock while A held its own' ); + unlike( $output, qr/Can't get lock/, 'no contention between distinct checkouts' ); + + print $in "\n"; close $in; + waitpid( $pid, 0 ); + }; +} + +done_testing;