fix: restart HA daemons on install/remove; add regression test (#179)

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 <noreply@anthropic.com>

.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 `<daemon> 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/<node> root@<node>.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:<vmid> --state stopped
+   # wait for TASK OK
+   ha-manager set vm:<vmid> --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.

.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

packaging/DEBIAN/postinst

@@ -39,15 +39,24 @@ 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
+# 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
@@ -60,6 +69,7 @@ case "$1" in
             echo "$0: called with unknown argument '$1'" >&2
             exit 0
             ;;
-esac
+    esac
 
-exit 0
+    exit 0
+fi

packaging/DEBIAN/postrm

@@ -23,14 +23,23 @@ 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
+# 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}"
@@ -49,6 +58,7 @@ case "$1" in
             echo "$0: called with unknown argument '$1'" >&2
             exit 0
             ;;
-esac
+    esac
 
-exit 0
+    exit 0
+fi

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 <theprofessor@ksatechnologies.com>  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

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 = {};

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

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

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

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

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

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

tests/stubs/systemctl

@@ -0,0 +1,25 @@
+#!/bin/bash
+# Stub for systemctl, scoped to what restart_pve_services() actually calls:
+# `systemctl restart <unit> [<unit>...]` 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

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."