From 002ee4a3bbb18a2fe70cf8b1c9f4d096d3c0afe5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 15:29:29 +0000 Subject: [PATCH] fix(actions): stop shuts guests down gracefully instead of pulling the plug The stop action called `qm stop` / `pct stop`, which kills the guest immediately; the `--overrule-shutdown 1` retry after the Bash timeout only aborts running shutdown tasks and forced nothing. Stop now runs `qm|pct shutdown --timeout STOP_TIMEOUT --forceStop 1`, so Proxmox shuts the guest down cleanly and hard-stops it only after the timeout. The qm/pct mocks now log their argv and can fail on demand; new tests cover stop (VM and CT), the timeout value, the failure path, a declined confirmation and restart. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01S8LmYGQA8hFstiLCCyzUYZ --- CHANGELOG.md | 12 ++++++++++ README.md | 8 +++---- completions/pman.zsh | 2 +- proxmox-manager.sh | 39 +++++++++--------------------- tests/mock-bin/pct | 9 +++++++ tests/mock-bin/qm | 9 +++++++ tests/run.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 102 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c78c80..5b5ec75 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,18 @@ Versions follow [Semantic Versioning](https://semver.org/). ## [Unreleased] +### Fixed + +- Stop action (menu key 2) now runs `qm shutdown` / `pct shutdown` with + `--timeout STOP_TIMEOUT --forceStop 1`: the guest is shut down cleanly and + only hard-stopped by Proxmox once the timeout expires. Previously it called + `qm stop` / `pct stop`, which kills the guest immediately (like pulling the + power plug); the `--overrule-shutdown 1` retry did not force anything either, + it only aborts running shutdown tasks. +- Tests: `qm`/`pct` mocks log their calls (`PMAN_MOCK_GUEST_LOG`) and can fail + on demand (`PMAN_MOCK_GUEST_FAIL`); new tests cover stop, its failure path, + a declined confirmation and restart for VM and CT. + ## [2.13.0] - 2026-09-19 ### Added diff --git a/README.md b/README.md index 7aded30..59408ad 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ Check out how others are using `proxmox-manager`: | ๐Ÿฉบ | **Health Monitoring** | CPU / RAM / disk per guest, uptime and OK / WARN / CRIT โ€” `pman --health`, key `h` in the TUI, `Health:` line in the status action | | ๐Ÿ”” | **Alerts** | `pman --check` for cron: thresholds, stopped `onboot` guests, unexpected stops and failed tasks; ntfy / e-mail only when something changes (opt-in) | | ๐Ÿ“‹ | **List & Status** | All VMs and containers with live status โ€” `[+]` running ยท `[-]` stopped ยท `[~]` paused ยท `[?]` unknown | -| โšก | **Start / Stop / Restart** | Confirmation prompt for destructive actions. Proxmox error details on failure. Configurable timeout with force-stop fallback | +| โšก | **Start / Stop / Restart** | Confirmation prompt for destructive actions. Proxmox error details on failure. Stop is a graceful shutdown (ACPI / guest agent); Proxmox force-stops the guest after the configurable timeout | | ๐Ÿ–ฅ๏ธ | **Console Access** | LXC shell via `pct enter` or QEMU terminal via `qm terminal`. Verifies running state before entering | | ๐ŸŒ | **IP Address Lookup** | Shows current IPv4 addresses for running VMs and CTs โ€” VM via QEMU Guest Agent, CT via `pct exec` | | ๐Ÿ“ฆ | **Snapshot Management** | List, create, rollback, delete โ€” with name validation and snapshot preview before destructive actions | @@ -214,7 +214,7 @@ the checkout cannot change the privileged executable. | `pman --filter running` | Filter output by status: `running` \| `stopped` \| `paused` | | `pman --name web` | Filter by VM/CT name (ERE substring-match; combinable with `--filter`) | | `pman --force` | Skip all confirmation prompts (for unattended scripts) | -| `pman --timeout 30` | Custom stop timeout in seconds (default: 60) | +| `pman --timeout 30` | Graceful shutdown timeout before force stop, in seconds (default: 60) | | `pman --no-clear` | Don't clear screen in interactive mode | | `pman --once` | Run a single interactive refresh cycle (useful for TTY recording) | | `pman --health` | Health overview: CPU / MEM / DISK %, uptime and level per guest (`--list`, `--json` combinable) | @@ -451,7 +451,7 @@ CLI flags are applied last and always win over config file values. ```bash # ~/.pmanrc โ€” example -STOP_TIMEOUT=120 # stop timeout in seconds (default: 60) +STOP_TIMEOUT=120 # shutdown timeout before force stop (default: 60) LOG_FILE="/var/log/proxmox-manager.log" # structured log file (empty = disabled) PROXMOX_MANAGER_SPICE_ADDR="spice.example.invalid" # SPICE bind address HEALTH_MEM_WARN=80 # health thresholds, see "Health monitoring & alerts" @@ -547,7 +547,7 @@ shellcheck proxmox-manager.sh tests/run.sh ``` -131 tests covering `validate_vmid`, `validate_snapshot_name`, `ip_info`, +141 tests covering `validate_vmid`, `validate_snapshot_name`, `ip_info`, `--filter`, CLI flags, config parsing, the health view, the `--check` engine and notifications (mocked `pvesh`, `curl`, `sendmail`; no network, no root). diff --git a/completions/pman.zsh b/completions/pman.zsh index 73f5c12..f45633a 100644 --- a/completions/pman.zsh +++ b/completions/pman.zsh @@ -18,7 +18,7 @@ _pman() { '--filter[Filter output by status]:status:(running stopped paused)' \ '--no-clear[Do not clear the screen in interactive mode]' \ '--once[Run a single interactive refresh cycle]' \ - '--timeout[Timeout in seconds for stop operations]:seconds:(10 30 60 120 300)' \ + '--timeout[Shutdown timeout in seconds before force stop]:seconds:(10 30 60 120 300)' \ '--name[Filter by VM/CT name (ERE substring-match)]:pattern:' \ '--force[Skip all confirmation prompts]' \ '--health[Show CPU/memory/disk health of local VMs/CTs]' \ diff --git a/proxmox-manager.sh b/proxmox-manager.sh index a3e0c13..70dc480 100755 --- a/proxmox-manager.sh +++ b/proxmox-manager.sh @@ -22,7 +22,7 @@ LIST_FLAG=0 JSON_FLAG=0 LOG_FILE="${LOG_FILE:-}" # Set LOG_FILE=/path/to/file to enable file logging FILTER_STATUS="" # Filter output by status: running|stopped|paused (empty = no filter) -STOP_TIMEOUT="${STOP_TIMEOUT:-60}" # Timeout in seconds for stop operations; env-overridable +STOP_TIMEOUT="${STOP_TIMEOUT:-60}" # Graceful shutdown timeout before force stop; env-overridable PROXMOX_MANAGER_SPICE_ADDR="${PROXMOX_MANAGER_SPICE_ADDR:-}" FORCE_MODE=0 # Set to 1 via --force to skip all confirm() prompts FILTER_NAME="" # ERE substring-match against VM/CT name (empty = no filter) @@ -535,7 +535,7 @@ Options: --name PATTERN Filter by VM/CT name (ERE substring-match; combinable with --filter) --no-clear Do not clear the screen in interactive mode --once Run a single interactive refresh (useful for TTY recording) - --timeout SECS Timeout for stop operations in seconds (default: 60) + --timeout SECS Shutdown timeout before force stop in seconds (default: 60) --force Skip all confirmation prompts (use with care) --health Show CPU/memory/disk health of local VMs/CTs (combine with --list for plain text or --json; --filter/--name apply) @@ -2781,41 +2781,24 @@ do_action() { note "Aborted." return } - note "Stopping $ty $id ($name) (timeout: ${STOP_TIMEOUT}s)..." - local _timeout_out='' _timeout_exit=0 _force_out='' _force_exit=0 + # Graceful shutdown (ACPI / guest agent); Proxmox itself hard-stops the guest + # once STOP_TIMEOUT expires (--forceStop 1). A plain "qm/pct stop" would + # kill the guest immediately, like pulling the power plug. + note "Shutting down $ty $id ($name) (force stop after ${STOP_TIMEOUT}s)..." + local _pve_out if [[ "$ty" == "CT" ]]; then - _timeout_out=$(timeout "${STOP_TIMEOUT}" pct stop "$id" 2>&1) || _timeout_exit=$? - if ((_timeout_exit == 0)); then + if _pve_out=$(pct shutdown "$id" --timeout "$STOP_TIMEOUT" --forceStop 1 2>&1); then ok "$ty $id stopped." - elif ((_timeout_exit == 124)); then - note "Timeout after ${STOP_TIMEOUT}s. Forcing stop with --overrule-shutdown..." - _force_out=$(pct stop "$id" --overrule-shutdown 1 2>&1) || _force_exit=$? - if ((_force_exit == 0)); then - ok "$ty $id force-stopped." - else - err "Force stop failed for CT $id." - _log_pve_err "$_force_out" - fi else err "Failed to stop CT $id." - _log_pve_err "$_timeout_out" + _log_pve_err "$_pve_out" fi else - _timeout_out=$(timeout "${STOP_TIMEOUT}" qm stop "$id" 2>&1) || _timeout_exit=$? - if ((_timeout_exit == 0)); then + if _pve_out=$(qm shutdown "$id" --timeout "$STOP_TIMEOUT" --forceStop 1 2>&1); then ok "$ty $id stopped." - elif ((_timeout_exit == 124)); then - note "Timeout after ${STOP_TIMEOUT}s. Forcing stop with --overrule-shutdown..." - _force_out=$(qm stop "$id" --overrule-shutdown 1 2>&1) || _force_exit=$? - if ((_force_exit == 0)); then - ok "$ty $id force-stopped." - else - err "Force stop failed for VM $id." - _log_pve_err "$_force_out" - fi else err "Failed to stop VM $id." - _log_pve_err "$_timeout_out" + _log_pve_err "$_pve_out" fi fi ;; diff --git a/tests/mock-bin/pct b/tests/mock-bin/pct index 1742f97..2f71078 100755 --- a/tests/mock-bin/pct +++ b/tests/mock-bin/pct @@ -1,6 +1,15 @@ #!/usr/bin/env bash set -euo pipefail +# PMAN_MOCK_GUEST_LOG: append argv; PMAN_MOCK_GUEST_FAIL: exit 1 for that subcommand. +if [[ -n "${PMAN_MOCK_GUEST_LOG:-}" ]]; then + printf '%s %s\n' "$(basename "$0")" "$*" >>"$PMAN_MOCK_GUEST_LOG" +fi +if [[ -n "${PMAN_MOCK_GUEST_FAIL:-}" && "${1:-}" == "$PMAN_MOCK_GUEST_FAIL" ]]; then + echo "mock: $1 failed for ${2:-}" >&2 + exit 1 +fi + cmd="${1:-}" shift || true diff --git a/tests/mock-bin/qm b/tests/mock-bin/qm index 6a31caf..fbbd557 100755 --- a/tests/mock-bin/qm +++ b/tests/mock-bin/qm @@ -1,6 +1,15 @@ #!/usr/bin/env bash set -euo pipefail +# PMAN_MOCK_GUEST_LOG: append argv; PMAN_MOCK_GUEST_FAIL: exit 1 for that subcommand. +if [[ -n "${PMAN_MOCK_GUEST_LOG:-}" ]]; then + printf '%s %s\n' "$(basename "$0")" "$*" >>"$PMAN_MOCK_GUEST_LOG" +fi +if [[ -n "${PMAN_MOCK_GUEST_FAIL:-}" && "${1:-}" == "$PMAN_MOCK_GUEST_FAIL" ]]; then + echo "mock: $1 failed for ${2:-}" >&2 + exit 1 +fi + cmd="${1:-}" shift || true diff --git a/tests/run.sh b/tests/run.sh index 6fa0643..3735e5f 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -486,6 +486,62 @@ else _fail "status action: health line missing" fi +# --------------------------------------------------------------------------- +# Lifecycle actions: stop is a graceful shutdown with force-stop fallback +# --------------------------------------------------------------------------- +guest_log="$TEST_TMP/guest.log" +# _act INPUT ID TYPE ACTION [VAR=VAL...] โ€” run do_action in a subshell with a fresh guest log; sets act_out. +_act() { + local input="$1" id="$2" ty="$3" act="$4" + shift 4 + : >"$guest_log" + act_out="$( + export PMAN_MOCK_GUEST_LOG="$guest_log" + for kv in "$@"; do export "${kv?}"; done + STOP_TIMEOUT="${STOP_TIMEOUT:-60}" + printf '%s\n' "$input" | do_action "$id" "$ty" "$act" "name-$id" 2>&1 + )" || true +} + +for g in VM:qm:200 CT:pct:100; do + IFS=: read -r gty gcmd gid <<<"$g" + _act y "$gid" "$gty" stop + if grep -qx "$gcmd shutdown $gid --timeout 60 --forceStop 1" "$guest_log" && + ! grep -q "^$gcmd stop" "$guest_log" && grep -q "$gty $gid stopped." <<<"$act_out"; then + _pass "stop action ($gty): graceful shutdown with --forceStop, never '$gcmd stop'" + else + _fail "stop action ($gty): unexpected calls: $(tr '\n' ';' <"$guest_log")" + fi + + _act y "$gid" "$gty" stop STOP_TIMEOUT=17 + if grep -qx "$gcmd shutdown $gid --timeout 17 --forceStop 1" "$guest_log"; then + _pass "stop action ($gty): STOP_TIMEOUT is passed to $gcmd shutdown" + else + _fail "stop action ($gty): STOP_TIMEOUT not passed: $(tr '\n' ';' <"$guest_log")" + fi + + _act y "$gid" "$gty" stop PMAN_MOCK_GUEST_FAIL=shutdown + if grep -q "Failed to stop $gty $gid." <<<"$act_out" && grep -q "mock: shutdown failed for $gid" <<<"$act_out"; then + _pass "stop action ($gty): shutdown failure shows the Proxmox error" + else + _fail "stop action ($gty): shutdown failure not reported" + fi + + _act n "$gid" "$gty" stop + if ! grep -qE "^$gcmd (shutdown|stop) " "$guest_log" && grep -q "Aborted." <<<"$act_out"; then + _pass "stop action ($gty): declined confirmation sends nothing" + else + _fail "stop action ($gty): declined confirmation still acted" + fi + + _act y "$gid" "$gty" restart + if grep -qx "$gcmd reboot $gid" "$guest_log" && ! grep -qE "^$gcmd (stop|shutdown) " "$guest_log"; then + _pass "restart action ($gty): uses $gcmd reboot" + else + _fail "restart action ($gty): unexpected calls: $(tr '\n' ';' <"$guest_log")" + fi +done + # --------------------------------------------------------------------------- # Health check engine: --check / --dry-run (notifications are not sent here) # ---------------------------------------------------------------------------