From 377a483c01ce3bc546c6dcee45902207d145be2e Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Wed, 26 Aug 2026 17:34:18 -0300 Subject: [PATCH] test(xcat-core): run setupntp instead of matching its source The test added with this fix matched regexes against the text of setupntp and makentp.pm. A source match cannot tell whether the code it found ever runs, and it describes the fix rather than the behaviour: unlike(qr/check_exec_or_exit[^\n]*hwclock/) says "this line does not mention hwclock", where what matters is that a management node without hwclock still gets its clock set. Run the script instead. setupntp cannot simply be executed: it forces its own PATH, so its commands cannot be stubbed from outside, and it exits unless UID is 0, so it cannot run as an ordinary user. The test takes the script's own helper functions and the section that configures the daemon and drives them with shell functions, which bash resolves ahead of PATH and which both `type` and `command -v` report as present -- the two probes the script uses. A node without hwclock is simulated by hiding it from both, rather than by asserting on the shape of the check. Every assertion now fails when the behaviour it describes is removed: hwclock required by check_exec_or_exit again 5 assertions fail the hwclock guard removed 2 assertions fail systemd-timesyncd left running 2 assertions fail systemd-timesyncd left enabled 2 assertions fail The debian/control and xCAT.spec checks are kept as they were. Those are manifest contents, not behaviour -- there is nothing to execute, and the assertion is on a package name that survives reformatting. The match on makentp.pm for xCAT::NTP::Backend->choose is dropped. It asserted that a call site exists; ntp_backend_selection.t already drives the selector itself across 33 assertions, which is the decision that matters. Signed-off-by: Daniel Hilst <392820+dhilst@users.noreply.github.com> --- xCAT-test/unit/makentp_ntp_deps.t | 141 +++++++++++++++++++++++------- 1 file changed, 107 insertions(+), 34 deletions(-) diff --git a/xCAT-test/unit/makentp_ntp_deps.t b/xCAT-test/unit/makentp_ntp_deps.t index 68180ee34..3716d852e 100644 --- a/xCAT-test/unit/makentp_ntp_deps.t +++ b/xCAT-test/unit/makentp_ntp_deps.t @@ -1,58 +1,131 @@ #!/usr/bin/env perl use strict; use warnings; + +use FindBin; +use File::Temp qw(tempdir); use Test::More; -# Regression: makentp/setupntp configure a server-capable NTP daemon (chronyd/ntpd) on the MN. -# On a minimal Ubuntu 24.04 MN chrony was absent (Ubuntu ships only the client-only -# systemd-timesyncd), hwclock moved to util-linux-extra (absent) so setupntp aborted its whole NTP -# setup, and timesyncd was left fighting the NTP daemon -- reddening reg_linux_diskfull_installation_flat. +# setupntp configures a server-capable NTP daemon on the management node. On a stock Ubuntu MN +# three things went wrong: hwclock had moved to util-linux-extra and was absent, so the fatal +# executable check aborted the ENTIRE NTP setup including the clock step that does not use it; +# systemd-timesyncd was left running and kept disciplining the clock against the daemon being +# configured; and the xcat package did not pull in a daemon that can serve time at all. +# +# The first three are behaviours of the script, so run it. The last is a property of the +# packaging manifests, so read those. -use File::Spec; -use FindBin; -my $repo_root = File::Spec->rel2abs( File::Spec->catdir( $FindBin::Bin, '..', '..' ) ); -sub slurp { - my ($rel) = @_; - my $path = File::Spec->catfile( $repo_root, split m{/}, $rel ); - local $/; - open my $fh, '<', $path or return undef; - <$fh>; +my $repo = "$FindBin::Bin/../.."; +my $setupntp_path = "$repo/xCAT/postscripts/setupntp"; +plan skip_all => 'setupntp not found' unless -r $setupntp_path; +plan skip_all => 'setupntp targets Linux management nodes' unless $^O eq 'linux'; + +open(my $fh, '<', $setupntp_path) or die "open $setupntp_path: $!"; +my @lines = <$fh>; +close $fh; +my $source = join '', @lines; + +# setupntp forces its own PATH and refuses to run unless UID is 0, so its commands cannot be +# stubbed from outside and it cannot run as an ordinary user. Take the script's own helper +# functions and the section that configures the daemon, and drive them with shell functions -- +# which bash resolves ahead of PATH, and which `type` and `command -v` both report as present. +my ($helpers) = $source =~ /\A(.*?)^\[ "\$\{UID\}" -eq "0" \]/ms; +my ($body) = $source =~ /^(check_exec_or_exit cp cat logger grep\n.*?)^CHRONY_CONF=/ms; +BAIL_OUT('could not take the helper functions from setupntp') unless $helpers; +BAIL_OUT('could not take the daemon setup section from setupntp') unless $body; + +sub run_setupntp { + my (%opt) = @_; + my $root = tempdir(CLEANUP => 1); + + # Record every call the section makes, and answer as the case requires. + my $doubles = <<"BASH"; +log() { printf '%s\\n' "\$*" >>"$root/calls"; } +systemctl() { log "systemctl \$*"; return 0; } +timedatectl() { log "timedatectl \$*"; return 0; } +chronyd() { log "chronyd \$*"; return 0; } +logger() { log "logger \$*"; return 0; } +rm() { log "rm \$*"; return 0; } +BASH + $doubles .= $opt{hwclock} + ? qq{hwclock() { log "hwclock \$*"; return 0; }\n} + # hwclock genuinely absent: hide it from both probes the script can use + : qq{command() { if [ "\$1" = "-v" ] && [ "\$2" = "hwclock" ]; then return 1; fi; builtin command "\$@"; }\n} + . qq{type() { if [ "\$1" = "hwclock" ]; then return 1; fi; builtin type "\$@"; }\n}; + + $doubles .= "declare -a NTP_SERVERS=(" . ($opt{server} ? qq{"$opt{server}"} : '') . ")\n"; + $doubles .= "log_label=xcat\n"; + + my $rc = system('bash', '-c', $doubles . $helpers . $body); + + my $calls = ''; + if (open my $ch, '<', "$root/calls") { local $/; $calls = <$ch>; close $ch } + return { rc => $rc >> 8, calls => $calls }; } -my $setupntp = slurp('xCAT/postscripts/setupntp'); -SKIP: { - skip 'setupntp not found', 4 unless defined $setupntp; - unlike($setupntp, qr/check_exec_or_exit[^\n]*\bhwclock\b/, - 'setupntp does NOT hard-require hwclock in check_exec_or_exit'); - like($setupntp, qr/command -v hwclock/, - 'setupntp guards its hwclock use so a missing hwclock is non-fatal'); - like($setupntp, qr/systemctl\s+(?:stop|disable)\s+systemd-timesyncd/, - 'setupntp stops/disables systemd-timesyncd so it does not fight the NTP daemon'); - like($setupntp, qr/chronyd\s+-f\s+\S*\s+-q/, - 'setupntp still steps the system clock via a one-shot chronyd -q'); +# --- a management node with no hwclock: the bug this fix exists for -------- +{ + my $r = run_setupntp(hwclock => 0); + + is($r->{rc}, 0, 'setupntp completes on a node with no hwclock instead of aborting'); + like($r->{calls}, qr/^chronyd .*-q/m, + 'the system clock is still stepped, which is the part that never needed hwclock'); + unlike($r->{calls}, qr/^hwclock/m, 'and no attempt is made to use the missing hwclock'); + like($r->{calls}, qr/hwclock not present/, + 'the skipped RTC persist is reported rather than passing silently'); } +# --- a management node that has hwclock ------------------------------------ +{ + my $r = run_setupntp(hwclock => 1); + + is($r->{rc}, 0, 'setupntp completes on a node that has hwclock'); + like($r->{calls}, qr/^hwclock --systohc --utc$/m, + 'the stepped system clock is persisted to the RTC when hwclock is available'); + like($r->{calls}, qr/^chronyd .*-q/m, 'the clock is stepped in this case too'); +} + +# --- systemd-timesyncd must yield to the NTP daemon ------------------------ +foreach my $case ([ 'with hwclock', 1 ], [ 'without hwclock', 0 ]) { + my ($name, $hwclock) = @$case; + my $r = run_setupntp(hwclock => $hwclock); + + like($r->{calls}, qr/^systemctl stop systemd-timesyncd\.service$/m, + "$name: systemd-timesyncd is stopped so it stops disciplining the clock"); + like($r->{calls}, qr/^systemctl disable systemd-timesyncd\.service$/m, + "$name: systemd-timesyncd is disabled so it does not come back on the next boot"); +} + +# --- the configured NTP server reaches the clock step ---------------------- +{ + my $r = run_setupntp(hwclock => 1, server => 'ntp.example.com'); + like($r->{calls}, qr/^chronyd .*server ntp\.example\.com iburst/m, + 'the clock is stepped against the NTP server the node was given'); +} +{ + my $r = run_setupntp(hwclock => 1); + like($r->{calls}, qr/^chronyd .*pool pool\.ntp\.org iburst/m, + 'a node given no NTP server falls back to the public pool'); +} + +# --- the packaging must supply a daemon that can serve time ---------------- +# These are manifest contents, not behaviour: there is nothing to execute. +sub slurp { my $p = shift; open my $h, '<', "$repo/$p" or return undef; local $/; <$h> } + my $ctrl = slurp('xCAT/debian/control'); SKIP: { skip 'debian/control not found', 2 unless defined $ctrl; like($ctrl, qr/^Depends:.*\bchrony \| ntp\b/m, - 'xcat debian package Depends on chrony | ntp (server-capable NTP daemon)'); + 'the xcat debian package depends on a server-capable NTP daemon'); like($ctrl, qr/^Recommends:.*\butil-linux-extra\b/m, - 'xcat debian package Recommends util-linux-extra (provides hwclock on noble+)'); + 'and recommends the package that carries hwclock on noble and later'); } my $spec = slurp('xCAT/xCAT.spec'); SKIP: { skip 'xCAT.spec not found', 1 unless defined $spec; like($spec, qr/^Requires:\s*\(chrony or ntp\)/m, - 'xCAT rpm Requires (chrony or ntp) for makentp'); -} - -my $makentp = slurp('xCAT-server/lib/xcat/plugins/makentp.pm'); -SKIP: { - skip 'makentp.pm not found', 1 unless defined $makentp; - like($makentp, qr/xCAT::NTP::Backend->choose/, - 'makentp selects the NTP daemon through the xCAT::NTP::Backend selector'); + 'the xCAT rpm requires a server-capable NTP daemon'); } done_testing();