From ecbdd53e83b1e8f31a08e62a560cf83fd3aea653 Mon Sep 17 00:00:00 2001 From: Daniel Hilst <392820+dhilst@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:13:01 -0300 Subject: [PATCH] fix(provtest,dhcptest): make a failed teardown safe to fail Teardown removed nodes, restored tables and unpacked the saved zone tree without checking any of it, then deleted the state directory regardless. A restore that half-failed left the machine wrong and the record of what to put back gone. Every removal and restore is checked now. The zone archive is verified when written and listed again before the live tree is removed, so an unreadable archive leaves the tree alone. Anything that fails keeps the state directory and returns non-zero. The kickstarts mkinstall renders under autoinst are removed by name, and the loader moved aside during the absent-loader stage stays in tftpdir rather than under /tmp. --- .../autotest/testcase/dhcptest/dhcpfixture.sh | 28 ++++-- .../autotest/testcase/provtest/provfixture.sh | 97 +++++++++++++++---- 2 files changed, 99 insertions(+), 26 deletions(-) diff --git a/xCAT-test/autotest/testcase/dhcptest/dhcpfixture.sh b/xCAT-test/autotest/testcase/dhcptest/dhcpfixture.sh index 47f400b51..79dca86a6 100755 --- a/xCAT-test/autotest/testcase/dhcptest/dhcpfixture.sh +++ b/xCAT-test/autotest/testcase/dhcptest/dhcpfixture.sh @@ -817,7 +817,10 @@ do_run_loader_absent() { tftp=$(tftpdir) path="$tftp/xcat/xnba.kpxe" [ -f "$path" ] || die "$path is not there to remove" - mv -f "$path" "$STATE/xnba.kpxe.away" || die "cannot move $path aside" + # Moved aside within $tftpdir and not into $STATE: $STATE is under /tmp, and + # this is the machine's only copy of a loader xCAT does not rebuild. A + # reboot between here and restore_absent_loader would lose it. + mv -f "$path" "$path.provtest-away" || die "cannot move $path aside" echo "$path" > "$STATE/loader-away" if do_generate; then @@ -837,7 +840,7 @@ restore_absent_loader() { local path [ -f "$STATE/loader-away" ] || return 0 path=$(cat "$STATE/loader-away") - mv -f "$STATE/xnba.kpxe.away" "$path" 2>/dev/null + [ -n "$path" ] && mv -f "$path.provtest-away" "$path" 2>/dev/null rm -f "$STATE/loader-away" } @@ -922,11 +925,14 @@ do_run_hierarchy() { } do_teardown() { - local unit + local unit failed=0 [ -d "$STATE" ] || return 0 - [ -f "$STATE/node" ] && { makedhcp -d "$NODE" >/dev/null 2>&1; makehosts -d "$NODE" >/dev/null 2>&1; rmdef "$NODE" >/dev/null 2>&1; } - [ -f "$STATE/adopt" ] && { makedhcp -d "$ADOPT_NODE" >/dev/null 2>&1; makehosts -d "$ADOPT_NODE" >/dev/null 2>&1; rmdef "$ADOPT_NODE" >/dev/null 2>&1; } + # Each removal is checked. A definition left behind makes the next run on + # this machine refuse, and deleting $STATE anyway would throw away the + # record of what still has to be put back. + [ -f "$STATE/node" ] && { makedhcp -d "$NODE" >/dev/null 2>&1; makehosts -d "$NODE" >/dev/null 2>&1; rmdef "$NODE" >/dev/null 2>&1 || { say "FAILED: cannot remove the node $NODE"; failed=1; }; } + [ -f "$STATE/adopt" ] && { makedhcp -d "$ADOPT_NODE" >/dev/null 2>&1; makehosts -d "$ADOPT_NODE" >/dev/null 2>&1; rmdef "$ADOPT_NODE" >/dev/null 2>&1 || { say "FAILED: cannot remove the node $ADOPT_NODE"; failed=1; }; } netboot_undefine extra_undefine [ -f "$STATE/iscsi" ] && chtab -d node="$ISCSI_NODE" iscsi >/dev/null 2>&1 @@ -939,11 +945,15 @@ do_teardown() { # tabrestore replaces the table wholesale, which is what is wanted here: # dhcpinterfaces and dhcpbackend go back to what they were, unset included. - [ -f "$STATE/site.csv" ] && tabrestore "$STATE/site.csv" >/dev/null 2>&1 + if [ -f "$STATE/site.csv" ]; then + tabrestore "$STATE/site.csv" >/dev/null 2>&1 \ + || { say "FAILED: cannot restore the site table from $STATE/site.csv"; failed=1; } + fi for f in /etc/dhcp/dhcpd.conf /etc/dhcpd.conf /etc/kea/kea-dhcp4.conf; do local saved="$STATE/$(echo "$f" | tr / _)" - [ -f "$saved" ] && cp -f "$saved" "$f" + [ -f "$saved" ] || continue + cp -f "$saved" "$f" || { say "FAILED: cannot restore $f"; failed=1; } done if [ -f "$STATE/backend" ]; then @@ -958,6 +968,10 @@ do_teardown() { [ -n "$unit" ] && systemctl restart "$unit" >/dev/null 2>&1 fi + if [ "$failed" != 0 ]; then + say "$STATE was kept; it holds what this machine has to be put back to" + return 1 + fi rm -rf "$STATE" say "fixture removed" } diff --git a/xCAT-test/autotest/testcase/provtest/provfixture.sh b/xCAT-test/autotest/testcase/provtest/provfixture.sh index 418efe31a..c77bafcbe 100755 --- a/xCAT-test/autotest/testcase/provtest/provfixture.sh +++ b/xCAT-test/autotest/testcase/provtest/provfixture.sh @@ -467,9 +467,16 @@ define_node() { mn_hosts_entry() { local short addr short=$(hostname -s) || die "cannot read this machine's hostname" - addr=$(getent ahostsv4 "$(hostname)" 2>/dev/null | awk '{print $1; exit}') - [ -n "$addr" ] || addr=$(hostname -I 2>/dev/null | awk '{print $1}') - [ -n "$addr" ] || die "cannot find an address for $short" + # Not a loopback address: Debian and Ubuntu map the hostname to 127.0.1.1 in + # /etc/hosts, and makedns would write that as the address of the zone's + # server -- a nameserver every node is told to ask and none can reach. + addr=$(getent ahostsv4 "$(hostname)" 2>/dev/null \ + | awk '$1 !~ /^127\./ {print $1; exit}') + [ -n "$addr" ] || addr=$(hostname -I 2>/dev/null | tr ' ' '\n' \ + | awk '$1 != "" && $1 !~ /^127\./ {print; exit}') + # Last resort, and a true one: $IF_SRV holds it, and it is the address the + # node reaches this machine on. + [ -n "$addr" ] || addr=$SRV_IP printf '%s %s.%s\n' "$addr" "$short" "$DOMAIN" >> /etc/hosts \ || die "cannot add $short.$DOMAIN to /etc/hosts" } @@ -606,27 +613,46 @@ do_setup() { } save_dns_config() { - local f + local f saved for f in /etc/named.conf /etc/bind/named.conf /etc/bind/named.conf.local; do - [ -f "$f" ] && cp -f "$f" "$STATE/$(echo "$f" | tr / _)" + [ -f "$f" ] || continue + cp -f "$f" "$STATE/$(echo "$f" | tr / _)" || die "cannot save $f" done # The zone tree as a whole: makedns removes inside it, and a file-by-file - # restore would miss that. + # restore would miss that. Both written and read back here, because a tar + # that ran out of room still leaves a file behind -- and teardown deletes a + # live zone tree on the strength of this archive. for f in /var/named /var/lib/bind /etc/bind; do - [ -d "$f" ] && tar -C / -czf "$STATE/$(echo "$f" | tr / _).tgz" "${f#/}" 2>/dev/null + [ -d "$f" ] || continue + saved="$STATE/$(echo "$f" | tr / _).tgz" + tar -C / -czf "$saved" "${f#/}" 2>/dev/null || die "cannot archive $f to $saved" + tar -tzf "$saved" >/dev/null 2>&1 || die "the archive of $f is unreadable" done + return 0 } +# Non-zero if anything could not be put back. Teardown needs to know: makedns +# reads the site table and rewrites the zones, so regenerating over a restore +# that half-failed is what turns a recoverable mess into a permanent one. restore_dns_config() { - local f saved + local f saved rc=0 for f in /var/named /var/lib/bind /etc/bind; do saved="$STATE/$(echo "$f" | tr / _).tgz" - [ -f "$saved" ] && { rm -rf "$f"; tar -C / -xzf "$saved" 2>/dev/null; } + [ -f "$saved" ] || continue + # Listed before the live tree is removed: an archive that cannot be read + # is one that cannot be extracted either, and then $f would simply be + # gone. + tar -tzf "$saved" >/dev/null 2>&1 \ + || { say "FAILED: $saved is unreadable, so $f was left as it is"; rc=1; continue; } + rm -rf "$f" + tar -C / -xzf "$saved" 2>/dev/null || { say "FAILED: cannot restore $f from $saved"; rc=1; } done for f in /etc/named.conf /etc/bind/named.conf /etc/bind/named.conf.local; do saved="$STATE/$(echo "$f" | tr / _)" - [ -f "$saved" ] && cp -f "$saved" "$f" + [ -f "$saved" ] || continue + cp -f "$saved" "$f" || { say "FAILED: cannot restore $f"; rc=1; } done + return $rc } # The file a node of each netboot type is given, by the name the loader asks for @@ -1008,28 +1034,43 @@ do_run_dns_removal() { # --- teardown ------------------------------------------------------------- do_teardown() { - local name path dir + local name path dir failed=0 [ -d "$STATE" ] || return 0 # Every loop reads its list on file descriptor 3, because the xCAT clients # inside them read standard input themselves: on the first iteration the # command swallows the rest of the file and teardown stops after one name, # leaving nodes and a rewritten site table behind. + # Each removal is checked. A definition left behind is what makes the next + # run on this machine refuse -- and if $STATE were deleted anyway, the + # record of what to put back would be gone with it. if [ -f "$STATE/nodes" ]; then while read -r name <&3; do [ -n "$name" ] || continue nodeset "$name" offline >/dev/null 2>&1 makedns -d "$name" >/dev/null 2>&1 makehosts -d "$name" >/dev/null 2>&1 - rmdef "$name" >/dev/null 2>&1 + rmdef "$name" >/dev/null 2>&1 \ + || { say "FAILED: cannot remove the node $name"; failed=1; } done 3< "$STATE/nodes" fi if [ -f "$STATE/osimages" ]; then while read -r name <&3; do - [ -n "$name" ] && rmdef -t osimage -o "$name" >/dev/null 2>&1 + [ -n "$name" ] || continue + rmdef -t osimage -o "$name" >/dev/null 2>&1 \ + || { say "FAILED: cannot remove the osimage $name"; failed=1; } done 3< "$STATE/osimages" fi - [ -f "$STATE/network" ] && rmdef -t network -o "$NETOBJ" >/dev/null 2>&1 + if [ -f "$STATE/network" ]; then + rmdef -t network -o "$NETOBJ" >/dev/null 2>&1 \ + || { say "FAILED: cannot remove the network $NETOBJ"; failed=1; } + fi + + # mkinstall renders the node's kickstart here and nothing records it, so it + # is removed by name. Guarded on a non-empty name: this is a glob. + for name in "$NODE" "$PXE_NODE" "$BOOT_NODE" "$XNBA_NODE" "$PTB_NODE"; do + [ -n "$name" ] && rm -rf "$(installdir)/autoinst/$name" "$(installdir)/autoinst/$name".* + done # What nodeset offline did not take with it. It is not reliable here: it # exits as soon as it cannot reach the DHCP backend, and leaves the kernel @@ -1083,20 +1124,38 @@ do_teardown() { # tabrestore replaces the table wholesale, which is what is wanted here: # site.master and site.domain go back to what they were, unset included. - [ -f "$STATE/site.csv" ] && tabrestore "$STATE/site.csv" >/dev/null 2>&1 - [ -f "$STATE/hosts" ] && cp -f "$STATE/hosts" /etc/hosts + if [ -f "$STATE/site.csv" ]; then + tabrestore "$STATE/site.csv" >/dev/null 2>&1 \ + || { say "FAILED: cannot restore the site table from $STATE/site.csv"; failed=1; } + fi + if [ -f "$STATE/hosts" ]; then + cp -f "$STATE/hosts" /etc/hosts \ + || { say "FAILED: cannot restore /etc/hosts from $STATE/hosts"; failed=1; } + fi - restore_dns_config + restore_dns_config || failed=1 # Regenerated from the restored configuration, so a machine that was serving - # its own zones is serving them again. - makedns -n >/dev/null 2>&1 + # its own zones is serving them again -- but only once everything is back. + # makedns reads site.domain and site.master: run over a site table that did + # not restore, it would write this fixture's domain into the zones of a real + # cluster, and teardown would be the thing that broke it. + if [ "$failed" = 0 ]; then + [ -f "$STATE/dns" ] && { makedns -n >/dev/null 2>&1 || say "makedns -n reported an error"; } + else + say "the zones were left alone: regenerating them from a configuration that did not fully restore would make the damage permanent" + fi restore_services + if [ "$failed" != 0 ]; then + say "$STATE was kept; it holds what this machine has to be put back to" + return 1 + fi rm -rf "$STATE" say "fixture removed" } dispatch() { + STAGE=${1:-fixture} case "${1:-}" in check) do_check ;; setup) do_setup ;;