fix(xcat-core): a SOURCES directory per package and target outlives the build

The staging directory was keyed by package, target and uniqueext. Nothing
deleted it, so every pair left a SOURCES tree under $HOME/rpmbuild, and an
archive from an earlier run stayed beside the next run's sources.

Each forked builder now stages into $HOME/rpmbuild/sources/SOURCES.<host>.<pid>.
The pid is unique among live processes on one host, and the host name keeps two
hosts apart when $HOME is on NFS. prepare_build_sources_dir creates the directory
empty. The parent deletes it when it reaps the builder, whatever the exit, and
abort_builds deletes it for each builder that exits after SIGTERM. At start, the
run deletes the directories of this host whose pid is not alive.

buildrpms_sources_per_package.t covers the name, the empty directory, the
deletion after success and failure, and the sweep of a dead pid next to a live
one and another host.

Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com>

build-utils/lib/XCAT/BuildUtils.pm

@@ -20,6 +20,7 @@ use File::Path qw(make_path remove_tree);
 use File::Temp qw(tempdir tempfile);
 use File::Slurper qw(read_text write_text);
 use POSIX qw(strftime);
+use Sys::Hostname ();
 use Pod::Usage qw(pod2usage);
 use feature 'say';
 
@@ -32,7 +33,8 @@ our @EXPORT_OK = qw(
     pin_control_version rewrite_changelog_header
     reprepro_distributions reprepro_options
     lock_id_for take_build_lock
-    build_sources_dir prepare_build_sources_dir
+    build_sources_base build_sources_dir prepare_build_sources_dir
+    remove_build_sources_dir sweep_build_sources_dirs
     sh_quote clean_debian_residue git_revision
     backup_file restore_file
     sh sh_or_die usage
@@ -972,44 +974,65 @@ sub genesis_log_errors {
 
 #-------------------------------------------------------------------------------
 
-=head3 build_sources_dir
+=head3 build_sources_base
 
-Descriptions: The rpmbuild SOURCES directory for one package and one target.
+Descriptions: The directory that holds the per-process rpmbuild SOURCES directories.
 
 Arguments:
-  $package   - the package being built, e.g. xCAT
-  $target    - the mock target, e.g. openeuler-24.03sp4-x86_64
-  $uniqueext - the mock uniqueext, or undef
-  $home      - the home directory; defaults to $ENV{HOME}
+  $home - the home directory; defaults to $ENV{HOME}
+
+Returns: the absolute path of the base directory.
+
+=cut
+
+#-------------------------------------------------------------------------------
+sub build_sources_base {
+    my ($home) = @_;
+    $home = $ENV{HOME} unless defined $home && length $home;
+    die "build_sources_base: no home directory\n" unless defined $home && length $home;
+    return "$home/rpmbuild/sources";
+}
+
+#-------------------------------------------------------------------------------
+
+=head3 build_sources_dir
+
+Descriptions: The rpmbuild SOURCES directory of one build process.
+
+Arguments:
+  $base - the directory from build_sources_base
+  $pid  - the pid of the process that stages; defaults to $$
+  $host - the host name; defaults to this host
 
 Returns: the absolute path of the staging directory.
 
 =cut
 
 #-------------------------------------------------------------------------------
-# Keyed like the mock chroot. buildrpms.pl forks a child per package and target, and
-# mock --sources copies the whole directory, so one shared directory lets a peer's tar
-# truncate an archive mid-copy. xCAT and xCATsn both write etc.tar.gz.
+# Keyed by the process that stages. buildrpms.pl forks a child per package and target,
+# and mock --sources copies the whole directory, so a shared directory lets a peer's tar
+# truncate an archive mid-copy. A pid is unique only on one host, and $HOME can be on NFS.
 sub build_sources_dir {
-    my ($package, $target, $uniqueext, $home) = @_;
+    my ($base, $pid, $host) = @_;
+    die "build_sources_dir: no base directory\n" unless defined $base && length $base;
+    $pid  = $$ unless defined $pid;
+    die "build_sources_dir: pid '$pid' is not a number\n" unless $pid =~ /^[0-9]+$/;
+    $host = _sources_host($host);
+    return "$base/SOURCES.$host.$pid";
+}
 
-    # An empty key names the shared directory again.
-    die "build_sources_dir: package is required\n" unless defined $package && length $package;
-    die "build_sources_dir: target is required\n"  unless defined $target  && length $target;
-
-    $home = $ENV{HOME} unless defined $home && length $home;
-    die "build_sources_dir: no home directory\n" unless defined $home && length $home;
-
-    my $key = "$package-$target";
-    $key .= "-$uniqueext" if defined $uniqueext && length $uniqueext;
-    return "$home/rpmbuild/$key/SOURCES";
+sub _sources_host {
+    my ($host) = @_;
+    $host = Sys::Hostname::hostname() unless defined $host && length $host;
+    $host =~ s/[^A-Za-z0-9._-]/-/g;
+    return $host;
 }
 
 #-------------------------------------------------------------------------------
 
 =head3 prepare_build_sources_dir
 
-Descriptions: Create the staging directory for one package and target, and return it.
+Descriptions: Create the staging directory of one build process, empty, and return it.
 
 Arguments: the same as build_sources_dir.
 
@@ -1020,9 +1043,75 @@ Returns: the absolute path of the staging directory.
 #-------------------------------------------------------------------------------
 sub prepare_build_sources_dir {
     my $dir = build_sources_dir(@_);
+    # A directory with this name belongs to a dead process that had the same pid.
+    remove_tree($dir) if -e $dir;
     make_path($dir);
     die "build_sources_dir: $dir was not created\n" unless -d $dir;
     return $dir;
 }
 
+#-------------------------------------------------------------------------------
+
+=head3 remove_build_sources_dir
+
+Descriptions: Delete the staging directory of one build process.
+
+Arguments: the same as build_sources_dir. The caller makes sure the process has exited.
+
+Returns: true when the directory is gone.
+
+=cut
+
+#-------------------------------------------------------------------------------
+sub remove_build_sources_dir {
+    my $dir = build_sources_dir(@_);
+    remove_tree($dir) if -e $dir;
+    return !-e $dir;
+}
+
+#-------------------------------------------------------------------------------
+
+=head3 sweep_build_sources_dirs
+
+Descriptions: Delete the staging directories that processes on this host left behind
+              when they were killed before their parent could delete them.
+
+Arguments:
+  $base  - the directory from build_sources_base
+  $host  - the host name; defaults to this host
+  $alive - a sub that takes a pid and returns true when that process exists;
+           defaults to kill 0
+
+Returns: the list of directories deleted.
+
+=cut
+
+#-------------------------------------------------------------------------------
+sub sweep_build_sources_dirs {
+    my ($base, $host, $alive) = @_;
+    $host  = _sources_host($host);
+    $alive //= \&_pid_alive;
+    opendir(my $dh, $base) or return ();
+    my @entries = readdir $dh;
+    closedir $dh;
+
+    my @removed;
+    for my $entry (sort @entries) {
+        # Another host's pids say nothing about this host's processes.
+        my ($pid) = $entry =~ /^SOURCES\.\Q$host\E\.([0-9]+)$/ or next;
+        next if $alive->($pid);
+        remove_tree("$base/$entry");
+        push @removed, "$base/$entry" unless -e "$base/$entry";
+    }
+    return @removed;
+}
+
+# EPERM means the process exists and belongs to another user. An unreaped zombie
+# also answers here, and its parent deletes the directory when it reaps it.
+sub _pid_alive {
+    my ($pid) = @_;
+    return 1 if kill 0, $pid;
+    return $!{EPERM} ? 1 : 0;
+}
+
 1;

buildrpms.pl

@@ -55,7 +55,8 @@ use File::Temp qw(tempdir tempfile);
 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
-    prepare_build_sources_dir stage_xcat_probe_sources stage_xcatsn_templates
+    build_sources_base prepare_build_sources_dir remove_build_sources_dir
+    sweep_build_sources_dirs stage_xcat_probe_sources stage_xcatsn_templates
                         stage_genesis_base_sources
                         write_script read_line targetarch_from_target
                         openeuler_build_target openeuler_repo_subdir);
@@ -69,8 +70,9 @@ use autodie;
 use autodie qw(cp);
 
 
-# Set per package and target in buildall, so each forked child stages alone.
+# Set in buildall to a directory named for the child's pid, so each forked child stages alone.
 my $SOURCES = '';
+my $SOURCES_BASE = build_sources_base();
 # Ensure the rpmbuild tree exists. buildrpms stages source tarballs into $SOURCES, but it only
 # runs rpmdev-setuptree in the one-time env-setup path -- so on a host where that never ran (or
 # $HOME/rpmbuild was cleaned) source staging fails with "SOURCES/...: No such file or directory",
@@ -527,8 +529,8 @@ EOF
 
 sub buildall {
     my ($pkg, $target) = @_;
-    # This process is the child for one pair, so the assignment cannot reach a peer.
-    $SOURCES = prepare_build_sources_dir($pkg, $target, $opts{mock_uniqueext});
+    # The parent deletes this directory when it reaps the child.
+    $SOURCES = prepare_build_sources_dir($SOURCES_BASE, $$);
     createmockconfig($pkg, $target);
     buildsources($pkg, $target);
     buildspkgs($pkg, $target);
@@ -938,7 +940,8 @@ sub abort_builds {
     warn "\n[buildrpms] caught SIG$sig: aborting -- signalling mock to self-clean...\n";
     my @chroots = values %MOCK_INFLIGHT;
     kill 'TERM', mock_pids(@chroots);               # mock unmounts + orphanKills itself
-    kill 'TERM', keys %MOCK_INFLIGHT;               # unwind the ForkManager builders too
+    my @builders = keys %MOCK_INFLIGHT;
+    kill 'TERM', @builders;                         # unwind the ForkManager builders too
     my @mock;
     for (1 .. 30) {                                 # wait for mock to finish its own cleanup
         @mock = mock_pids(@chroots);
@@ -951,6 +954,17 @@ sub abort_builds {
         select undef, undef, undef, 2;
         sweep_mock_mounts(@chroots);
     }
+    # Delete the staging directory of each builder that has exited. A builder that is
+    # still alive keeps its directory, and the sweep in the next run deletes it.
+    for my $pid (@builders) {
+        my $reaped = 0;
+        for (1 .. 10) {
+            $reaped = waitpid($pid, POSIX::WNOHANG());
+            last if $reaped;
+            select undef, undef, undef, 0.5;
+        }
+        remove_build_sources_dir($SOURCES_BASE, $pid) if $reaped == $pid;
+    }
     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.
@@ -999,6 +1013,9 @@ sub main {
         }
     }
 
+    # A builder killed with SIGKILL together with this parent leaves its directory behind.
+    sweep_build_sources_dirs($SOURCES_BASE);
+
     my @rpms = product($opts{packages}, $opts{targets});
     my $pm = Parallel::ForkManager->new($opts{nproc});
 
@@ -1009,6 +1026,7 @@ sub main {
     $pm->run_on_finish(sub {
         my ($pid, $exit_code, $ident, $exit_signal, $core_dump) = @_;
         delete $MOCK_INFLIGHT{$pid};
+        remove_build_sources_dir($SOURCES_BASE, $pid);
         # A child that die()s exits non-zero; one killed by a SIGNAL (SIGKILL / OOM-killer) is reaped
         # with $exit_code==0 but $exit_signal!=0 (and maybe $core_dump). Checking $exit_code alone
         # would let an OOM-killed worker through and the parent would index+sign a partial repository

xCAT-test/unit/buildrpms_source_only.t

@@ -52,7 +52,7 @@ our @STAGES;
     sub buildsources     { push @main::STAGES, 'buildsources'; }
     sub buildspkgs       { push @main::STAGES, 'buildspkgs'; }
     sub buildpkgs        { push @main::STAGES, 'buildpkgs'; }
-    # buildall assigns a staging directory per package and target.
+    # buildall assigns a staging directory per build process.
     sub prepare_build_sources_dir { return '/nonexistent/rpmbuild/stub/SOURCES'; }
 }
 
@@ -64,6 +64,7 @@ my $harness = join "\n",
     'use strict; use warnings;',
     'our %opts;',
     'our $SOURCES = \'\';',
+    'our $SOURCES_BASE = \'/nonexistent/rpmbuild/sources\';',
     'sub say { }',
     # write_repo_metadata_dir does real work past the guard; stop it there so the
     # test observes the guard and nothing else.

xCAT-test/unit/buildrpms_sources_per_package.t

@@ -1,63 +1,92 @@
 #!/usr/bin/env perl
-# buildrpms.pl staged every source tarball into one $HOME/rpmbuild/SOURCES while forking a child
-# per package and target, so a build could read a truncated archive:
-#
-#   error: File /builddir/build/SOURCES/xCAT-test-2.20.0.tar.gz is smaller than 13 bytes
-#
-# Two packages of one target collide the same way: xCAT and xCATsn both write etc.tar.gz.
+# buildrpms.pl forks a child per package and target. Each child stages its source
+# tarballs into a directory named for its pid, and the parent deletes it after the child.
 use strict;
 use warnings;
 
-use File::Spec ();
+use File::Path qw(make_path);
+use File::Slurper qw(write_text);
 use File::Temp qw(tempdir);
 use FindBin;
 use lib "$FindBin::Bin/../../build-utils/lib";
 use Test::More;
 
-use XCAT::BuildUtils qw(build_sources_dir prepare_build_sources_dir);
+use XCAT::BuildUtils qw(build_sources_base build_sources_dir prepare_build_sources_dir
+    remove_build_sources_dir sweep_build_sources_dirs);
 
 my $home = tempdir(CLEANUP => 1);
+my $base = build_sources_base($home);
+is($base, "$home/rpmbuild/sources", 'the base directory is under the rpmbuild tree');
 
-# The key is the package AND the target. Either one alone leaves a pair sharing a directory.
-my $a = build_sources_dir('xCAT',   'openeuler-24.03sp4-x86_64', 'ci', $home);
-my $b = build_sources_dir('xCATsn', 'openeuler-24.03sp4-x86_64', 'ci', $home);
-my $c = build_sources_dir('xCAT',   'openeuler-22.03sp4-x86_64', 'ci', $home);
-isnt($a, $b, 'two packages of one target stage into different directories');
-isnt($a, $c, 'two targets of one package stage into different directories');
+# Two live processes on one host never have the same pid. The host name keeps two
+# hosts apart when $HOME is on NFS.
+my $a = build_sources_dir($base, 101, 'hosta');
+my $b = build_sources_dir($base, 102, 'hosta');
+my $c = build_sources_dir($base, 101, 'hostb');
+isnt($a, $b, 'two pids stage into different directories');
+isnt($a, $c, 'one pid on two hosts stages into different directories');
+is($a, "$base/SOURCES.hosta.101", 'the directory name carries the host and the pid');
+is(build_sources_dir($base, undef, 'hosta'), "$base/SOURCES.hosta.$$",
+    'the pid defaults to the calling process');
 
-# Keyed like the mock chroot, so the staging directory and the chroot cannot disagree about
-# which build owns which files.
-is($a, "$home/rpmbuild/xCAT-openeuler-24.03sp4-x86_64-ci/SOURCES",
-    'the directory is keyed like the mock chroot: package, target, uniqueext');
-is(build_sources_dir('xCAT', 'openeuler-24.03sp4-x86_64', '', $home),
-    "$home/rpmbuild/xCAT-openeuler-24.03sp4-x86_64/SOURCES",
-    'an empty uniqueext adds no trailing separator');
-isnt(build_sources_dir('xCAT', 'el9-x86_64', 'ci', $home),
-     build_sources_dir('xCAT', 'el9-x86_64', 'other', $home),
-    'two uniqueexts of one pair do not share, so two runs on one host cannot collide');
+ok(!eval { build_sources_dir($base, 'x1', 'hosta'); 1 }, 'a pid that is not a number is fatal');
+like($@, qr/build_sources_dir: pid 'x1' is not a number/, 'and the message names the pid');
+ok(!eval { build_sources_dir('', 101, 'hosta'); 1 }, 'an empty base is fatal');
+like($@, qr/build_sources_dir: no base directory/, 'and the message names the base');
 
-# A missing argument must not collapse the key back to the one shared directory.
-for my $case (['package', undef, 'el9-x86_64'], ['target', 'xCAT', undef],
-              ['empty package', '', 'el9-x86_64'], ['empty target', 'xCAT', '']) {
-    my ($what, $pkg, $tg) = @$case;
-    my $died = eval { build_sources_dir($pkg, $tg, 'ci', $home); 1 } ? 0 : 1;
-    ok($died, "a missing $what is fatal rather than a shared directory");
-    like($@, qr/build_sources_dir: (?:package|target) is required/,
-        "the message names the missing argument for $what");
+# A directory with the same name is left by a dead process that had this pid.
+make_path($a);
+write_text("$a/stale.tar.gz", "old\n");
+my $made = prepare_build_sources_dir($base, 101, 'hosta');
+is($made, $a, 'prepare returns the directory of that pid');
+ok(-d $made, 'the staging directory is created');
+opendir(my $dh, $made) or die "cannot read $made: $!\n";
+my @left = grep { !/^\.\.?$/ } readdir $dh;
+closedir $dh;
+is_deeply(\@left, [], 'the staging directory is created empty');
+remove_build_sources_dir($base, 101, 'hosta');
+
+# The parent calls remove_build_sources_dir when it reaps a child, whatever its exit.
+for my $how ('success', 'failure') {
+    my $dir = prepare_build_sources_dir($base, 201, 'hosta');
+    write_text("$dir/xCAT-test-2.20.0.tar.gz", "payload\n");
+    my $pid = fork // die "fork: $!\n";
+    if ($pid == 0) {
+        require POSIX;
+        POSIX::_exit($how eq 'success' ? 0 : 1);
+    }
+    waitpid($pid, 0);
+    is($? >> 8, $how eq 'success' ? 0 : 1, "the child exits for $how");
+    ok(remove_build_sources_dir($base, 201, 'hosta'), "remove reports the directory gone after $how");
+    ok(!-e $dir, "the directory is deleted after $how");
 }
+ok(remove_build_sources_dir($base, 202, 'hosta'), 'removing a directory that does not exist is not an error');
 
-# prepare_build_sources_dir is what buildall calls: it must CREATE the directory, because
-# staging into a path that does not exist is how the shared tree came to be created up front.
-my $made = prepare_build_sources_dir('xCAT-test', 'openeuler-24.03sp4-x86_64', 'ci', $home);
-ok(-d $made, 'the staging directory is created, not merely named');
-my $again = prepare_build_sources_dir('xCAT-test', 'openeuler-24.03sp4-x86_64', 'ci', $home);
-is($again, $made, 'a second call is idempotent and returns the same directory');
+# The sweep deletes the directories of dead pids on this host only.
+my %alive = (301 => 1);
+my $is_alive = sub { $alive{ $_[0] } };
+my $dead  = prepare_build_sources_dir($base, 300, 'hosta');
+my $live  = prepare_build_sources_dir($base, 301, 'hosta');
+my $other = prepare_build_sources_dir($base, 300, 'hostb');
+make_path("$base/unrelated");
+my @swept = sweep_build_sources_dirs($base, 'hosta', $is_alive);
+is_deeply(\@swept, [$dead], 'the sweep reports the directory of the dead pid');
+ok(!-e $dead, 'the directory of a dead pid is deleted');
+ok(-d $live, 'the directory of a live pid is kept');
+ok(-d $other, 'the directory of another host is kept');
+ok(-d "$base/unrelated", 'a directory with another name is kept');
 
-# Nothing writes beside the per-pair directories: a path that escaped $home/rpmbuild would put
-# one build's sources where another build reads them.
-my @dirs = map { build_sources_dir($_, 'el9-x86_64', 'ci', $home) } qw(xCAT xCATsn xCAT-test);
-for my $d (@dirs) {
-    like($d, qr{^\Q$home/rpmbuild/\E[^/]+/SOURCES$}, "$d is one level under rpmbuild");
-}
+# The default liveness check: this process is alive, a reaped child is not.
+my $own = prepare_build_sources_dir($base, $$, 'hosta');
+my $gone = fork // die "fork: $!\n";
+if ($gone == 0) { require POSIX; POSIX::_exit(0) }
+waitpid($gone, 0);
+my $reaped = prepare_build_sources_dir($base, $gone, 'hosta');
+sweep_build_sources_dirs($base, 'hosta');
+ok(-d $own, 'the default check keeps the directory of a running process');
+ok(!-e $reaped, 'the default check deletes the directory of a reaped process');
+
+is_deeply([sweep_build_sources_dirs("$home/absent", 'hosta', $is_alive)], [],
+    'a missing base directory is not an error');
 
 done_testing;