From d60adfaaf79770b1dcdc07602b6b3cafc376a2fc Mon Sep 17 00:00:00 2001 From: Kevin Adams Date: Sat, 1 Aug 2026 17:57:46 -0400 Subject: [PATCH] fix: restart HA daemons on install/remove; add regression test (#179) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pvedaemon/pveproxy picked up the patched PVE::Storage state, but pve-ha-lrm/pve-ha-crm kept running against the pre-install state, so HA-managed VMs on TrueNAS storage failed to start. They don't support a `restart` subcommand (only start/stop/status), so they're restarted via systemctl instead. Also switched every restart from `cmd && log ...` to bare statements — set -e does not fire on the left side of &&, so a genuine restart failure was previously swallowed silently instead of aborting the script. Adds tests/test-restart-services.sh: sources the real postinst/postrm against stub PVE binaries in CI to catch both classes of regression (wrong subcommand, silently-swallowed failure) without needing a live PVE cluster. Real HA-cluster validation is documented as a manual runbook (RUNBOOK-001) since GitHub Actions can't run pve-ha-lrm/crm. Bumps $VERSION to 3.2.4. Fixes #179 Co-Authored-By: Claude Sonnet 5 --- .../RUNBOOK-001-ha-restart-validation.md | 90 +++++++++++++++++++ .github/workflows/build.yml | 5 ++ packaging/DEBIAN/postinst | 48 ++++++---- packaging/DEBIAN/postrm | 60 +++++++------ packaging/changelog.Debian | 13 +++ perl5/PVE/Storage/Custom/TrueNAS.pm | 2 +- tests/fail-stubs/pvestatd | 7 ++ tests/stubs/pve-ha-crm | 9 ++ tests/stubs/pve-ha-lrm | 9 ++ tests/stubs/pvedaemon | 7 ++ tests/stubs/pveproxy | 7 ++ tests/stubs/pvestatd | 7 ++ tests/stubs/systemctl | 25 ++++++ tests/test-restart-services.sh | 84 +++++++++++++++++ 14 files changed, 328 insertions(+), 45 deletions(-) create mode 100644 .claude/cos/runbooks/RUNBOOK-001-ha-restart-validation.md create mode 100644 tests/fail-stubs/pvestatd create mode 100644 tests/stubs/pve-ha-crm create mode 100644 tests/stubs/pve-ha-lrm create mode 100644 tests/stubs/pvedaemon create mode 100644 tests/stubs/pveproxy create mode 100644 tests/stubs/pvestatd create mode 100644 tests/stubs/systemctl create mode 100644 tests/test-restart-services.sh diff --git a/.claude/cos/runbooks/RUNBOOK-001-ha-restart-validation.md b/.claude/cos/runbooks/RUNBOOK-001-ha-restart-validation.md new file mode 100644 index 0000000..c9c514b --- /dev/null +++ b/.claude/cos/runbooks/RUNBOOK-001-ha-restart-validation.md @@ -0,0 +1,90 @@ +# RUNBOOK-001: HA Daemon Restart Validation + +## Purpose + +Validate that `truenas-proxmox` install/upgrade/remove correctly restarts the +Proxmox HA daemons (`pve-ha-lrm`, `pve-ha-crm`), so HA-managed VMs on TrueNAS +storage can start after a plugin install. Ref: #179. + +This is manual because it needs a real PVE cluster with HA configured — the +stub-based CI test (`tests/test-restart-services.sh`) covers the shell-level +regression (right subcommands, correct `set -e` failure propagation) but +cannot exercise the actual `pve-ha-lrm`/`pve-ha-crm` HA resource-manager +behavior or a real HA-managed VM start/stop cycle. + +## Background + +`postinst`/`postrm` patch `PVE::Storage` and must restart every PVE daemon +that has that module loaded in memory: `pvedaemon`, `pveproxy`, `pvestatd`, +`pve-ha-lrm`, `pve-ha-crm`. Missing the HA daemons leaves them running +against the pre-install state, so HA-managed VMs on TrueNAS/multipath +storage fail to start with errors like: + +``` +pve-ha-lrm: storage 'TrueNAS' does not exist +TASK ERROR: freenas: unknown iscsi provider. Available [comstar, istgt, iet, LIO] +``` + +`pve-ha-lrm`/`pve-ha-crm` binaries only support `start`/`stop`/`status`/`help` +— no `restart` subcommand — so they're restarted via `systemctl restart` +instead of the ` restart` form used for `pvedaemon`/`pveproxy`/`pvestatd`. + +## Prerequisites + +- At least one lab PVE node with HA enabled (an HA group + one HA-managed + resource is enough; a full multi-node failover isn't required for this check). +- TrueNAS storage configured and reachable from that node. +- SSH access (see `reference_ssh` memory for node FQDNs/keys). + +## Procedure + +1. **Install/upgrade the package** on the target node and confirm all five + services restart in the log: + ``` + ssh -i ~/.ssh/ root@.ksatechnologies.com \ + "apt install --reinstall truenas-proxmox && tail -20 /var/log/truenas-proxmox-install.log" + ``` + Expect log lines for `pvedaemon`, `pveproxy`, `pvestatd`, and + `pve-ha-lrm, pve-ha-crm restarted`. + +2. **Confirm the HA daemons are actually active post-restart:** + ``` + systemctl is-active pvedaemon pveproxy pvestatd pve-ha-lrm pve-ha-crm + ``` + All five must report `active`. + +3. **Create (or reuse) an HA-managed VM on TrueNAS storage:** + - VM disk on the `truenas:` (or `truenas-multipath:`) storage. + - Add the VM to an HA group (`max-started=1`, `requested state=started`). + +4. **Stop the VM via HA, then start it via HA** (not a manual `qm start`): + ``` + ha-manager set vm: --state stopped + # wait for TASK OK + ha-manager set vm: --state started + ``` + +5. **Confirm the HA-initiated start succeeds** — no + `storage 'TrueNAS' does not exist` or `unknown iscsi provider` errors in + the task log, and the VM reaches `running` state. + +6. **Repeat step 1–5 for `apt remove`/`purge`** if validating the `postrm` + path (less critical — removal restarts should be a no-op for HA behavior + since the plugin file is gone either way, but confirms the restarts + themselves don't error). + +## Pass/Fail + +- **Pass:** HA-initiated start succeeds with no storage-provider errors. +- **Fail:** Any HA-initiated start error referencing the TrueNAS storage — + re-check `packaging/DEBIAN/postinst` `restart_pve_services()` for a missed + daemon or a regression back to the `cmd && log ...` pattern (see + `tests/test-restart-services.sh` for why that pattern is unsafe under + `set -e`). + +## Known gaps + +- `packaging/DEBIAN-multipath/postinst`/`postrm` do not yet restart + `pvestatd`/`pve-ha-lrm`/`pve-ha-crm` at all (still only `pvedaemon`/ + `pveproxy`). Same root cause as #179, not yet fixed there — file as a + follow-up if multipath + HA is in active use. diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 74f0d75..03b9c5e 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -68,6 +68,11 @@ jobs: packaging/DEBIAN-multipath/postrm echo "Shell scripts OK" + - name: Test postinst/postrm service restarts + run: | + echo "==> Running tests/test-restart-services.sh..." + bash tests/test-restart-services.sh + # ── Job 2: Build .deb ──────────────────────────────────────────────────────── build: name: Build Package diff --git a/packaging/DEBIAN/postinst b/packaging/DEBIAN/postinst index 9ca9301..df2e68b 100644 --- a/packaging/DEBIAN/postinst +++ b/packaging/DEBIAN/postinst @@ -39,27 +39,37 @@ install_ui() { } restart_pve_services() { + # Bare statements (not `cmd && log ...`) so `set -e` actually aborts on + # failure instead of silently skipping past it — see tests/test-restart-services.sh. log "Restarting Proxmox VE services ..." - pvedaemon restart && log "pvedaemon restarted" - pveproxy restart && log "pveproxy restarted" - pvestatd restart && log "pvestatd restarted" - systemctl restart pve-ha-lrm pve-ha-crm && log "pve-ha-lrm, pve-ha-crm restarted" + pvedaemon restart + log "pvedaemon restarted" + pveproxy restart + log "pveproxy restarted" + pvestatd restart + log "pvestatd restarted" + systemctl restart pve-ha-lrm pve-ha-crm + log "pve-ha-lrm, pve-ha-crm restarted" log "Done. Refresh your Proxmox browser tab." } -case "$1" in - configure) - log "Configuring truenas-proxmox (previous version: ${2:-none})" - install_plugin - install_ui - restart_pve_services - ;; - abort-upgrade|abort-remove|abort-deconfigure) - ;; - *) - echo "$0: called with unknown argument '$1'" >&2 - exit 0 - ;; -esac +# Guard so tests can `source` this file (to exercise its functions) without +# running dpkg's maintainer-script logic below. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + case "$1" in + configure) + log "Configuring truenas-proxmox (previous version: ${2:-none})" + install_plugin + install_ui + restart_pve_services + ;; + abort-upgrade|abort-remove|abort-deconfigure) + ;; + *) + echo "$0: called with unknown argument '$1'" >&2 + exit 0 + ;; + esac -exit 0 + exit 0 +fi diff --git a/packaging/DEBIAN/postrm b/packaging/DEBIAN/postrm index d666430..09d062d 100644 --- a/packaging/DEBIAN/postrm +++ b/packaging/DEBIAN/postrm @@ -23,32 +23,42 @@ remove_ui() { } restart_pve_services() { + # Bare statements (not `cmd && log ...`) so `set -e` actually aborts on + # failure instead of silently skipping past it — see tests/test-restart-services.sh. log "Restarting Proxmox VE services ..." - pvedaemon restart && log "pvedaemon restarted" - pveproxy restart && log "pveproxy restarted" - pvestatd restart && log "pvestatd restarted" - systemctl restart pve-ha-lrm pve-ha-crm && log "pve-ha-lrm, pve-ha-crm restarted" + pvedaemon restart + log "pvedaemon restarted" + pveproxy restart + log "pveproxy restarted" + pvestatd restart + log "pvestatd restarted" + systemctl restart pve-ha-lrm pve-ha-crm + log "pve-ha-lrm, pve-ha-crm restarted" } -case "$1" in - remove) - log "Removing truenas-proxmox ..." - rm -f "$PLUGIN_DST" && log "Removed ${PLUGIN_DST}" - remove_ui - restart_pve_services - log "truenas-proxmox removed. Refresh your Proxmox browser tab." - ;; - purge) - log "Purging truenas-proxmox ..." - rm -f "$PLUGIN_DST" "$JS_DST" "$HELP_DST" "$LOG_FILE" - [ -f "$TPL" ] && sed -i '/truenas-storage\.js/d' "$TPL" - ;; - upgrade|failed-upgrade|disappear|abort-upgrade|abort-remove|abort-deconfigure) - ;; - *) - echo "$0: called with unknown argument '$1'" >&2 - exit 0 - ;; -esac +# Guard so tests can `source` this file (to exercise its functions) without +# running dpkg's maintainer-script logic below. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + case "$1" in + remove) + log "Removing truenas-proxmox ..." + rm -f "$PLUGIN_DST" && log "Removed ${PLUGIN_DST}" + remove_ui + restart_pve_services + log "truenas-proxmox removed. Refresh your Proxmox browser tab." + ;; + purge) + log "Purging truenas-proxmox ..." + rm -f "$PLUGIN_DST" "$JS_DST" "$HELP_DST" "$LOG_FILE" + [ -f "$TPL" ] && sed -i '/truenas-storage\.js/d' "$TPL" + ;; + upgrade|failed-upgrade|disappear|abort-upgrade|abort-remove|abort-deconfigure) + ;; + *) + echo "$0: called with unknown argument '$1'" >&2 + exit 0 + ;; + esac -exit 0 + exit 0 +fi diff --git a/packaging/changelog.Debian b/packaging/changelog.Debian index 29710f9..98ad8cb 100644 --- a/packaging/changelog.Debian +++ b/packaging/changelog.Debian @@ -1,3 +1,16 @@ +truenas-proxmox (3.2.4-1) bookworm; urgency=medium + + * Fix postinst/postrm to also restart pvestatd, pve-ha-lrm, pve-ha-crm on + install/upgrade/remove — pve-ha-lrm/pve-ha-crm previously kept running + against the pre-install PVE::Storage state, so HA-managed VMs on TrueNAS + storage failed to start (#179) + * pve-ha-lrm/pve-ha-crm restarted via systemctl (no restart subcommand); + all restarts changed from `cmd && log ...` to bare statements so a + genuine failure aborts the script instead of being silently swallowed + by set -e's && exemption + + -- KSA Technologies, LLC Sat, 01 Aug 2026 00:00:00 +0000 + truenas-proxmox (3.2.2-1) bookworm; urgency=low * Bump api() return value from 14 to 15 to match PVE 9 APIVER — silences diff --git a/perl5/PVE/Storage/Custom/TrueNAS.pm b/perl5/PVE/Storage/Custom/TrueNAS.pm index 13e0a7f..b69e7d5 100644 --- a/perl5/PVE/Storage/Custom/TrueNAS.pm +++ b/perl5/PVE/Storage/Custom/TrueNAS.pm @@ -29,7 +29,7 @@ use PVE::Storage::Plugin; use base qw(PVE::Storage::Plugin); -our $VERSION = '3.2.3'; +our $VERSION = '3.2.4'; # Per-host runtime state cache my $state = {}; diff --git a/tests/fail-stubs/pvestatd b/tests/fail-stubs/pvestatd new file mode 100644 index 0000000..745817d --- /dev/null +++ b/tests/fail-stubs/pvestatd @@ -0,0 +1,7 @@ +#!/bin/bash +# Always-fail stub used only by the failure-propagation test case, to prove +# a genuine restart failure aborts restart_pve_services() instead of being +# swallowed by the `cmd && log ...` pitfall (set -e does not fire on the +# left side of &&). +echo "pvestatd: simulated failure (test stub)" >&2 +exit 1 diff --git a/tests/stubs/pve-ha-crm b/tests/stubs/pve-ha-crm new file mode 100644 index 0000000..70c0147 --- /dev/null +++ b/tests/stubs/pve-ha-crm @@ -0,0 +1,9 @@ +#!/bin/bash +# Stub for the real /usr/sbin/pve-ha-crm PVE::HA daemon binary. +# Unlike pvedaemon/pveproxy/pvestatd, the real binary does NOT support +# "restart" — only start, stop, status, help. This must be restarted via +# systemctl instead (see systemctl stub). Ref: #179. +case "$1" in + start|stop|status|help) exit 0 ;; + *) echo "pve-ha-crm: unknown command '$1'" >&2; exit 1 ;; +esac diff --git a/tests/stubs/pve-ha-lrm b/tests/stubs/pve-ha-lrm new file mode 100644 index 0000000..e7bf39d --- /dev/null +++ b/tests/stubs/pve-ha-lrm @@ -0,0 +1,9 @@ +#!/bin/bash +# Stub for the real /usr/sbin/pve-ha-lrm PVE::HA daemon binary. +# Unlike pvedaemon/pveproxy/pvestatd, the real binary does NOT support +# "restart" — only start, stop, status, help. This must be restarted via +# systemctl instead (see systemctl stub). Ref: #179. +case "$1" in + start|stop|status|help) exit 0 ;; + *) echo "pve-ha-lrm: unknown command '$1'" >&2; exit 1 ;; +esac diff --git a/tests/stubs/pvedaemon b/tests/stubs/pvedaemon new file mode 100644 index 0000000..f617bf6 --- /dev/null +++ b/tests/stubs/pvedaemon @@ -0,0 +1,7 @@ +#!/bin/bash +# Stub for the real /usr/bin/pvedaemon PVE::Daemon wrapper. +# Real binary supports: start, stop, restart, reload, status, help. +case "$1" in + start|stop|restart|reload|status) exit 0 ;; + *) echo "pvedaemon: unknown command '$1'" >&2; exit 1 ;; +esac diff --git a/tests/stubs/pveproxy b/tests/stubs/pveproxy new file mode 100644 index 0000000..9727a87 --- /dev/null +++ b/tests/stubs/pveproxy @@ -0,0 +1,7 @@ +#!/bin/bash +# Stub for the real /usr/bin/pveproxy PVE::Daemon wrapper. +# Real binary supports: start, stop, restart, reload, status, help. +case "$1" in + start|stop|restart|reload|status) exit 0 ;; + *) echo "pveproxy: unknown command '$1'" >&2; exit 1 ;; +esac diff --git a/tests/stubs/pvestatd b/tests/stubs/pvestatd new file mode 100644 index 0000000..c564e55 --- /dev/null +++ b/tests/stubs/pvestatd @@ -0,0 +1,7 @@ +#!/bin/bash +# Stub for the real /usr/bin/pvestatd PVE::Daemon wrapper. +# Real binary supports: start, stop, restart, reload, status, help. +case "$1" in + start|stop|restart|reload|status) exit 0 ;; + *) echo "pvestatd: unknown command '$1'" >&2; exit 1 ;; +esac diff --git a/tests/stubs/systemctl b/tests/stubs/systemctl new file mode 100644 index 0000000..9151c22 --- /dev/null +++ b/tests/stubs/systemctl @@ -0,0 +1,25 @@ +#!/bin/bash +# Stub for systemctl, scoped to what restart_pve_services() actually calls: +# `systemctl restart [...]` against known PVE service units. +KNOWN_UNITS="pvedaemon pveproxy pvestatd pve-ha-lrm pve-ha-crm" + +case "$1" in + restart) + shift + if [[ $# -eq 0 ]]; then + echo "systemctl: restart requires at least one unit" >&2 + exit 1 + fi + for unit in "$@"; do + if [[ ! " $KNOWN_UNITS " =~ \ ${unit}\ ]]; then + echo "systemctl: unknown unit '${unit}'" >&2 + exit 1 + fi + done + exit 0 + ;; + *) + echo "systemctl: unsupported command '$1' in test stub" >&2 + exit 1 + ;; +esac diff --git a/tests/test-restart-services.sh b/tests/test-restart-services.sh new file mode 100644 index 0000000..2c7a497 --- /dev/null +++ b/tests/test-restart-services.sh @@ -0,0 +1,84 @@ +#!/bin/bash +# Regression test for restart_pve_services() in packaging/DEBIAN/postinst +# and postrm. Ref: #179 — pve-ha-lrm/pve-ha-crm don't support a `restart` +# subcommand (only pvedaemon/pveproxy/pvestatd do), so calling it directly +# aborts the maintainer script under `set -e` and breaks every future +# install/upgrade/remove. The stubs in tests/stubs/ mimic the real PVE +# binaries closely enough to catch that class of regression without needing +# a live PVE cluster (see .claude/cos/runbooks for the real-cluster check). +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(dirname "$SCRIPT_DIR")" +STUB_DIR="${SCRIPT_DIR}/stubs" + +export PATH="${STUB_DIR}:${PATH}" + +run_case() { + local script="$1" + local label="$2" + + echo "==> Testing restart_pve_services() in ${label}" + + local log_out + log_out="$(bash -c " + set -euo pipefail + source '${script}' + # postinst/postrm's own log() writes to /var/log via tee — override + # after sourcing so the test doesn't need root. + log() { echo \"[log] \$*\"; } + restart_pve_services + ")" + + printf ' %s\n' "${log_out//$'\n'/$'\n' }" + + for expected in "pvedaemon restarted" "pveproxy restarted" "pvestatd restarted" "pve-ha-lrm, pve-ha-crm restarted"; do + if ! grep -qF "${expected}" <<<"${log_out}"; then + echo "FAIL: ${label} — expected log line containing '${expected}' not found" >&2 + exit 1 + fi + done + + echo "PASS: ${label}" +} + +run_case "${REPO_ROOT}/packaging/DEBIAN/postinst" "postinst" +run_case "${REPO_ROOT}/packaging/DEBIAN/postrm" "postrm" + +# Failure-propagation check: a failing restart must abort the function (and +# thus the maintainer script under `set -e`), not be silently swallowed. +# This is what `cmd && log "..."` got wrong — set -e does not fire on the +# left side of `&&` — and what the current bare-statement form fixes. +run_failure_case() { + local script="$1" + local label="$2" + + echo "==> Testing failure propagation in ${label}" + + local rc=0 + local log_out + log_out="$(PATH="${SCRIPT_DIR}/fail-stubs:${STUB_DIR}:${PATH}" bash -c " + set -euo pipefail + source '${script}' + log() { echo \"[log] \$*\"; } + restart_pve_services + " 2>&1)" || rc=$? + + if [[ "${rc}" -eq 0 ]]; then + echo "FAIL: ${label} — restart_pve_services() returned 0 despite a failing restart:" >&2 + printf ' %s\n' "${log_out//$'\n'/$'\n' }" >&2 + exit 1 + fi + + if grep -qF "Done. Refresh" <<<"${log_out}"; then + echo "FAIL: ${label} — function ran to completion after a failing restart" >&2 + exit 1 + fi + + echo "PASS: ${label} (aborted with rc=${rc}, as expected)" +} + +run_failure_case "${REPO_ROOT}/packaging/DEBIAN/postinst" "postinst" +run_failure_case "${REPO_ROOT}/packaging/DEBIAN/postrm" "postrm" + +echo "All restart_pve_services() tests passed."