mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-03 18:55:33 +00:00
docs: migrate DECISIONS.md to house-format ADRs, and retire the review folder (#131)
D001-D021 become ADR 0001-0021 in docs/decisions/, one decision per file. D017's PESTER_VERSION amendment was a second decision in one entry and becomes ADR 0022. ADR 0023 records the migration and reverses the lane2-change-plan ruling that deliberately kept DECISIONS.md until the CI lane work landed. DECISIONS.md is reduced to a stub with a D-to-ADR redirect table, so the four released CHANGELOG entries and older issue bodies that cite it degrade to a redirect rather than a dead reference. docs/review/ and docs/lane2-change-plan.md are deleted (ADR 0024). Of 91 findings, 83 were resolved and six of the seven still open were already GitHub issues; F021 was the exception and is now #130. CLAUDE.md's Key Conventions list gains the two rules it was missing and becomes the checklist, with the ADRs carrying rationale.
This commit is contained in:
goodolclint-claude[bot]
committed by
GitHub
GitHub
parent
c3f051bba5
commit
b90791e2bf
@@ -78,6 +78,5 @@ pve force-cleanup
|
||||
|
||||
Before writing new code, read these files to understand established patterns and open issues:
|
||||
|
||||
- [`DECISIONS.md`](../DECISIONS.md) — architectural decisions and anti-patterns that must not be reintroduced.
|
||||
- [`docs/review/findings.json`](../docs/review/findings.json) — stable findings database (IDs F001… are permanent; resolved findings are marked, never deleted).
|
||||
- [`docs/review/REVIEW_REPORT.md`](../docs/review/REVIEW_REPORT.md) — latest full review report.
|
||||
- [`docs/decisions/`](../docs/decisions/) — architectural decision records, one per file: what was chosen, what was rejected, and why. The rules themselves are the "Key Conventions" list in [`CLAUDE.md`](../CLAUDE.md).
|
||||
- The repository's open GitHub issues — all outstanding work is tracked there.
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
name: PVE Package Currency
|
||||
|
||||
# Lane 2 of the two-lane CI split (see docs/lane2-change-plan.md).
|
||||
# Lane 2 of the two-lane CI split (see docs/decisions/0017-ci-runs-two-lanes-a-pinned-gating-lane-and-a-report-only-currency-lane.md).
|
||||
#
|
||||
# Lane 1 (integration-tests.yml) pins the nested nodes to what the ISO ships
|
||||
# and never upgrades — that pin is what makes it a stable merge gate.
|
||||
|
||||
@@ -191,7 +191,7 @@ jobs:
|
||||
run: bash tests/infrastructure/scripts/preflight-cleanup.test.sh
|
||||
|
||||
# The pin lives in two files; split-brain is exactly the unexplained
|
||||
# breakage D017 exists to prevent, so assert they agree.
|
||||
# breakage ADR 0022 exists to prevent, so assert they agree.
|
||||
- name: Pester pin agrees across workflow and image
|
||||
run: |
|
||||
wf=$(sed -n 's/^ PESTER_VERSION: *//p' .github/workflows/unit-tests.yml)
|
||||
|
||||
@@ -178,28 +178,40 @@ headroom guard.
|
||||
- URL paths: `Uri.EscapeDataString()` on all dynamic path segments
|
||||
- No bare `catch {}` blocks — use specific or filtered exceptions
|
||||
- Verb class constants required (`VerbsCommon.Get`, not `"Get"`)
|
||||
- Repeated protocol literals (auth header names, token prefixes) are `const string` fields
|
||||
- Public output types and model properties are native .NET or `Pve*` types — never Newtonsoft
|
||||
`JObject`/`JArray`/`JToken`. Internal parsing may use them freely.
|
||||
|
||||
## Review System
|
||||
This list is the checklist. `docs/decisions/` carries the rationale behind each rule, and is
|
||||
where a decision goes when one is made — see [ADR 0023](docs/decisions/0023-decisions-live-in-docs-decisions-in-house-adr-format.md).
|
||||
|
||||
This repo uses a structured review system to track findings and prevent regressions.
|
||||
## Decisions and open work
|
||||
|
||||
### Key files
|
||||
- `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...).
|
||||
Never renumber. Read this before any coding session to understand open issues.
|
||||
- `docs/review/REVIEW_REPORT.md` — latest full review report (scan-9, 2026-03-26, F001–F085)
|
||||
- `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing
|
||||
any new code.** It documents patterns that were deliberately chosen or changed and must
|
||||
not be reintroduced.
|
||||
- **`docs/decisions/`** — architectural decision records, one decision per file, in house ADR
|
||||
format. Each records what was chosen, what was rejected and why. `DECISIONS.md` is a stub
|
||||
with a D-number redirect table for references written before 2026-09-02.
|
||||
- **GitHub issues** — all open work. There is no findings file; the ledger that used to live
|
||||
in `docs/review/` was retired on 2026-09-02, see
|
||||
[ADR 0024](docs/decisions/0024-the-findings-ledger-is-retired-open-work-lives-in-github-issues.md).
|
||||
- **`CLAUDE.md` § "Key Conventions"** — the rules themselves.
|
||||
|
||||
A planning document that needs to be public is an issue, not a file in `docs/`.
|
||||
|
||||
### Before starting a coding session
|
||||
1. Read `DECISIONS.md` to understand established patterns
|
||||
2. Check `docs/review/findings.json` for open findings relevant to the area you're working in
|
||||
3. Do not introduce patterns listed as anti-patterns in DECISIONS.md
|
||||
1. Read the "Key Conventions" list above — it is the complete set of rules for new code
|
||||
2. Read the ADRs in `docs/decisions/` covering the area you're working in, for the reasoning
|
||||
behind those rules and for the alternatives already rejected
|
||||
3. Check the open issues for the area you're working in
|
||||
|
||||
### Finding ID stability
|
||||
Finding IDs (F001, F002...) are permanent. A resolved finding is never deleted from
|
||||
findings.json — it is marked `resolved` with evidence of the fix. If a finding reappears,
|
||||
it is marked `regressed` and retains its original ID.
|
||||
### Recording a decision
|
||||
Generate the file — do not hand-write the skeleton:
|
||||
|
||||
```bash
|
||||
~/.claude/templates/new-adr.sh "Decision title in plain words"
|
||||
```
|
||||
|
||||
One decision per file. A decision that supersedes an earlier ADR links to it and edits that
|
||||
ADR's **Status** line in the same commit.
|
||||
|
||||
## Releasing to PSGallery
|
||||
|
||||
|
||||
+34
-942
@@ -1,944 +1,36 @@
|
||||
# Architectural Decisions
|
||||
|
||||
This file documents patterns that were deliberately chosen or changed, anti-patterns that
|
||||
were explicitly removed, and constraints that must be maintained. **Read this before writing
|
||||
any new code.**
|
||||
|
||||
---
|
||||
|
||||
## D001 — Task polling must use TaskService.WaitForTask
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F032, F033, F036, F058
|
||||
**Resolved in scan**: 2026-03-22 (for VM/network cmdlets); container snapshot + storage cmdlets still open
|
||||
|
||||
### Decision
|
||||
All task-polling loops must use `TaskService.WaitForTask(upid, session, timeout, progress)`.
|
||||
Never implement inline `while(true)` or `do/while` polling loops in cmdlet files.
|
||||
|
||||
### Rationale
|
||||
Four VM/network cmdlets (InvokePveNetworkApply, NewPveSnapshot, RestorePveSnapshot,
|
||||
RemovePveSnapshot) and one guest exec cmdlet had copy-pasted polling loops with no timeout,
|
||||
causing cmdlets to hang indefinitely if a PVE task stalled. TaskService.WaitForTask has
|
||||
timeout enforcement, failure detection, and WriteProgress support.
|
||||
|
||||
Five additional cmdlets (3 container snapshot + 2 storage) still have this anti-pattern as
|
||||
of scan 2026-03-22 (F058).
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER do this in a cmdlet
|
||||
while (true)
|
||||
{
|
||||
var status = taskService.GetTask(upid, session);
|
||||
if (status.IsFinished) break;
|
||||
Thread.Sleep(1000);
|
||||
}
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
// Always use this
|
||||
TaskService.WaitForTask(upid, session, TimeoutSeconds, this);
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D002 — Password parameters must use SecureString
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F051
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
All cmdlet parameters that accept passwords must use `SecureString` type with
|
||||
`Marshal.SecureStringToGlobalAllocUnicode` + `ZeroFreeGlobalAllocUnicode` in a
|
||||
try/finally block for extraction.
|
||||
|
||||
### Rationale
|
||||
Set-PveVmGuestPassword originally accepted a plain `string` password parameter, leaving
|
||||
the credential in managed memory indefinitely. SecureString minimizes the window of
|
||||
exposure and is consistent with Connect-PveServer's PSCredential handling.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER accept passwords as plain strings
|
||||
[Parameter(Mandatory = true)]
|
||||
public string Password { get; set; }
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
[Parameter(Mandatory = true)]
|
||||
public SecureString Password { get; set; }
|
||||
|
||||
// In ProcessRecord:
|
||||
IntPtr ptr = IntPtr.Zero;
|
||||
try
|
||||
{
|
||||
ptr = Marshal.SecureStringToGlobalAllocUnicode(Password);
|
||||
string plainText = Marshal.PtrToStringUni(ptr);
|
||||
// Use plainText for API call
|
||||
}
|
||||
finally
|
||||
{
|
||||
if (ptr != IntPtr.Zero)
|
||||
Marshal.ZeroFreeGlobalAllocUnicode(ptr);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D003 — URL encoding required for all path parameters
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F050
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
All user-supplied or dynamic values interpolated into API URL paths must be wrapped in
|
||||
`Uri.EscapeDataString()`. This applies to all service classes.
|
||||
|
||||
### Rationale
|
||||
Snapshot names, node names, user IDs, and other identifiers could theoretically contain
|
||||
characters that break URL path segments. While most values come from validated sources,
|
||||
defense-in-depth requires consistent encoding. Applied across all 14 service classes.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER interpolate raw strings into URL paths
|
||||
var resource = $"nodes/{node}/qemu/{vmid}/snapshot/{snapshotName}";
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
var resource = $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/snapshot/{Uri.EscapeDataString(snapshotName)}";
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D004 — No bare catch blocks
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F039
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
No bare `catch { }` or `catch (Exception) { }` blocks. All catch blocks must either:
|
||||
1. Use a specific exception type (`catch (PveApiException ex)`), or
|
||||
2. Use a filtered catch with `when` clause that excludes fatal exceptions
|
||||
(`catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)`)
|
||||
|
||||
### Rationale
|
||||
Bare catches in PveHttpClient, PveCmdletBase, VmService, ContainerService, and
|
||||
GetPveVmCmdlet silently swallowed errors, making debugging impossible. Replacing with
|
||||
filtered or specific catches preserves error visibility while still handling expected
|
||||
transient failures.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER use bare catches
|
||||
try { ... }
|
||||
catch { }
|
||||
|
||||
// NEVER catch all exceptions unfiltered
|
||||
try { ... }
|
||||
catch (Exception) { /* ignore */ }
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
// Catch specific exceptions
|
||||
try { ... }
|
||||
catch (PveApiException ex) { WriteWarning(ex.Message); }
|
||||
|
||||
// Or use filtered catch for status polling
|
||||
try { ... }
|
||||
catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)
|
||||
{
|
||||
WriteVerbose($"Status poll failed: {ex.Message}");
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D005 — OutputType required on all cmdlets
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F037
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
Every cmdlet must have an `[OutputType(typeof(...))]` attribute declaring its return type.
|
||||
|
||||
### Rationale
|
||||
~54 cmdlets were missing OutputType, degrading IntelliSense, pipeline type inference, and
|
||||
`Get-Command -OutputType` queries. All 169 cmdlets now have the attribute.
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
[Cmdlet(VerbsCommon.Get, "PveVm")]
|
||||
[OutputType(typeof(VmInfo))]
|
||||
public sealed class GetPveVmCmdlet : PveCmdletBase
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D006 — ConfirmImpact.High required for destructive operations
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F011, F034, F042, F043, F062, F063
|
||||
**Resolved in scan**: 2026-03-22 (for VM cmdlets); container Restart/Suspend still open
|
||||
|
||||
### Decision
|
||||
All cmdlets that perform destructive or disruptive operations must set
|
||||
`ConfirmImpact = ConfirmImpact.High` in the `[Cmdlet]` attribute. This includes:
|
||||
- All `Remove-*` cmdlets
|
||||
- All `Stop-*` cmdlets
|
||||
- All `Reset-*` cmdlets
|
||||
- All `Restart-*` cmdlets
|
||||
- All `Suspend-*` cmdlets
|
||||
- `Restore-PveSnapshot` and `Restore-PveContainerSnapshot`
|
||||
- `New-PveTemplate` (irreversible conversion)
|
||||
|
||||
### Rationale
|
||||
Stop-PveVm, Reset-PveVm, Suspend-PveVm, Restart-PveVm, and Remove-PveRole were missing
|
||||
ConfirmImpact.High, meaning users could accidentally perform disruptive operations without
|
||||
being prompted. Container counterparts (Restart/Suspend) remain inconsistent as of F062/F063.
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
[Cmdlet(VerbsLifecycle.Stop, "PveVm", SupportsShouldProcess = true,
|
||||
ConfirmImpact = ConfirmImpact.High)]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D007 — All cmdlet classes must be sealed
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F041
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
All cmdlet classes must be declared `sealed`. Cmdlets are not designed for inheritance
|
||||
and sealing prevents unintended extension.
|
||||
|
||||
### Rationale
|
||||
~95 cmdlets were not sealed. Sealing all 169 cmdlets makes the design intent explicit
|
||||
and enables potential JIT optimizations.
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
public sealed class GetPveVmCmdlet : PveCmdletBase
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D008 — JSON serialization: Newtonsoft.Json only
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F044
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
Use only `Newtonsoft.Json` (`[JsonProperty]`) for JSON serialization attributes on model
|
||||
classes. Do not add `System.Text.Json` (`[JsonPropertyName]`) attributes.
|
||||
|
||||
### Rationale
|
||||
The module uses Newtonsoft.Json for all API response deserialization. Having both
|
||||
`[JsonProperty]` and `[JsonPropertyName]` attributes was redundant and confusing —
|
||||
System.Text.Json is not used at runtime. All `[JsonPropertyName]` attributes were removed.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER add System.Text.Json attributes alongside Newtonsoft
|
||||
[JsonProperty("status")]
|
||||
[JsonPropertyName("status")] // Don't add this
|
||||
public string Status { get; set; }
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
[JsonProperty("status")]
|
||||
public string Status { get; set; }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D009 — Framework targeting: netstandard2.0 for publishable, net10.0+net48 for tests
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F047, F064
|
||||
**Status note**: net9.0 → net10.0 migration still pending
|
||||
|
||||
### Decision
|
||||
- Publishable projects (`PSProxmoxVE`, `PSProxmoxVE.Core`): Target `netstandard2.0` for
|
||||
maximum compatibility (PS 5.1 Desktop + PS 7.x Core).
|
||||
- Test projects: Target `net10.0` (LTS) and `net48` (Windows PowerShell 5.1 validation).
|
||||
|
||||
### Rationale
|
||||
.NET 9.0 reached EOL in May 2025. The test projects should use the current LTS release
|
||||
(net10.0). The publishable module must remain on netstandard2.0 to support both Desktop
|
||||
and Core editions.
|
||||
|
||||
---
|
||||
|
||||
## D010 — VmId parameters: nullable int with ValidateRange
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F012, F038
|
||||
**Resolved in scan**: 2026-03-21 (ValidateRange), 2026-03-22 (nullable)
|
||||
|
||||
### Decision
|
||||
VmId parameters must:
|
||||
1. Use `int?` (nullable) when the parameter is optional (e.g., firewall cmdlets that
|
||||
operate at cluster, node, or VM level)
|
||||
2. Include `[ValidateRange(100, 999999999)]` to match PVE's VMID constraints
|
||||
3. Use `int` (non-nullable) only when VmId is mandatory
|
||||
|
||||
### Rationale
|
||||
PVE requires VMIDs in range 100-999999999. Without ValidateRange, invalid IDs reach the
|
||||
API and return confusing errors. Using non-nullable int with default 0 for optional VmId
|
||||
made it impossible to distinguish "not specified" from "VM 0" in firewall cmdlets.
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
// Mandatory VmId
|
||||
[Parameter(Mandatory = true)]
|
||||
[ValidateRange(100, 999999999)]
|
||||
public int VmId { get; set; }
|
||||
|
||||
// Optional VmId (e.g., firewall cmdlets)
|
||||
[Parameter()]
|
||||
[ValidateRange(100, 999999999)]
|
||||
public int? VmId { get; set; }
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D011 — Verb class constants required for cmdlet attributes
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F009
|
||||
**Resolved in scan**: 2026-03-21
|
||||
|
||||
### Decision
|
||||
All `[Cmdlet]` attributes must use verb class constants (`VerbsCommon.Get`,
|
||||
`VerbsLifecycle.Start`, etc.) instead of hardcoded string literals.
|
||||
|
||||
### Rationale
|
||||
Reset-PveVm used `[Cmdlet("Reset", ...)]` instead of `VerbsCommon.Reset`. While "Reset"
|
||||
is an approved verb, using the constant ensures compile-time verification and consistency
|
||||
with all other cmdlets.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
[Cmdlet("Reset", "PveVm")] // Don't use string literals
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
[Cmdlet(VerbsCommon.Reset, "PveVm")]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D012 — Magic strings: extract to named constants
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F049
|
||||
**Resolved in scan**: 2026-03-22
|
||||
|
||||
### Decision
|
||||
Frequently used string literals (auth header names, token prefixes, etc.) should be
|
||||
extracted to `const string` fields for maintainability.
|
||||
|
||||
### Rationale
|
||||
Auth header names (`PVEAPIToken=`, `CSRFPreventionToken`) were inline string literals
|
||||
used in multiple places. Extracting to `const string ApiTokenPrefix` and
|
||||
`CsrfHeaderName` fields improves maintainability and reduces typo risk.
|
||||
|
||||
---
|
||||
|
||||
## D013 — Cmdlets must emit only native or module-defined types
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: F085
|
||||
**Resolved in scan**: 2026-03-25
|
||||
|
||||
### Decision
|
||||
All cmdlet output types and public model properties must be native .NET types (`string`,
|
||||
`int`, `bool`, `Dictionary<string, object?>`, `List<T>`, `PSObject`, `void`) or types
|
||||
defined within the module itself (`Pve*` classes). Never expose third-party types like
|
||||
Newtonsoft's `JObject`, `JArray`, or `JToken` in public APIs.
|
||||
|
||||
### Rationale
|
||||
PowerShell enumerates `JArray` unexpectedly and `JObject` properties are not discoverable
|
||||
via `Get-Member` or tab completion. Users piping module output into `Format-Table`,
|
||||
`Select-Object`, or `Where-Object` get confusing behavior when the underlying type is a
|
||||
Newtonsoft container. Native dictionaries and lists work naturally in PowerShell pipelines.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// Service returning Newtonsoft type
|
||||
public JObject GetNodeConfig(...) { ... }
|
||||
|
||||
// Model exposing Newtonsoft type
|
||||
[JsonProperty("members")]
|
||||
public JArray? Members { get; set; }
|
||||
|
||||
// Cmdlet OutputType referencing Newtonsoft type
|
||||
[OutputType(typeof(JObject))]
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
// Service returns native dictionary
|
||||
public Dictionary<string, object?> GetNodeConfig(...) { ... }
|
||||
|
||||
// Model uses native type with converter for deserialization
|
||||
[JsonProperty("members")]
|
||||
[JsonConverter(typeof(NativeListConverter))]
|
||||
public List<Dictionary<string, object?>>? Members { get; set; }
|
||||
|
||||
// Cmdlet OutputType uses native or module type
|
||||
[OutputType(typeof(Dictionary<string, object>))]
|
||||
// or
|
||||
[OutputType(typeof(PSObject))]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D014 — New-PveCluster -Wait blocks until the cluster is quorate
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — found via integration run 172, 2026-09-01)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
`New-PveCluster -Wait` returns only after the cluster reports quorum, not merely when the
|
||||
create task completes. `ClusterConfigService.WaitForQuorum` implements the wait; it polls
|
||||
`GET /cluster/status` for the `cluster` entry with `quorate = 1` and tolerates transient API
|
||||
errors while corosync and pmxcfs restart.
|
||||
|
||||
The wait is bounded and throws `TimeoutException` on expiry. It follows the `-Wait` timeout
|
||||
convention already used by `Stop-PveContainer`, `Reset-PveVm` and `New-PveBackup`:
|
||||
`[ValidateRange(1, 3600)] public int Timeout` with a default (60 here), **no `0 = infinite`**.
|
||||
|
||||
Note there are two distinct timeout conventions in this module; do not mix them:
|
||||
- **`-Wait` waits** (`Timeout`, `int` with a default, range 1-3600, no infinite) — task/state waits.
|
||||
- **HTTP client timeouts** (`TimeoutSeconds`, `int?`, range 0-int.MaxValue, `0 = infinite`) —
|
||||
`Connect-PveServer`, `Send-PveFile`, `Invoke-PveStorageDownload`, which set `HttpClient.Timeout`.
|
||||
|
||||
A single-node cluster reaches quorum in seconds (~6 s observed), so a node still not quorate
|
||||
after 60 s is broken rather than slow.
|
||||
|
||||
`-Wait` on every other cmdlet still means "wait for the task". Cluster creation is the
|
||||
exception because the task completing does not make the cluster usable.
|
||||
|
||||
### Rationale
|
||||
PVE's cluster-create task returns before corosync converges. Until the node is quorate it
|
||||
rejects a join with `cluster not ready - no quorum?`, so the natural sequence
|
||||
`New-PveCluster -Wait` → `Add-PveClusterMember` fails intermittently for every caller.
|
||||
|
||||
Observed on node A in integration run 172: the create task returned, corosync started ~1 s
|
||||
later, and `node has quorum` appeared ~6 s after that. The integration test had guarded this
|
||||
with `Start-Sleep -Seconds 5` — a fixed sleep against a longer, variable convergence — which
|
||||
is why the cluster tests had never passed.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```powershell
|
||||
# NEVER guard cluster convergence with a fixed sleep
|
||||
New-PveCluster -ClusterName 'c1' -Wait
|
||||
Start-Sleep -Seconds 5
|
||||
Add-PveClusterMember ...
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```powershell
|
||||
# -Wait already guarantees quorum; join immediately
|
||||
New-PveCluster -ClusterName 'c1' -Wait
|
||||
Add-PveClusterMember ...
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D015 — Lifecycle -Wait blocks until the guest config lock clears
|
||||
|
||||
**Status**: Active, rescoped 2026-09-01 — it guards the config lock only, never the flock
|
||||
**Finding refs**: (none — found via integration runs 183/184, 2026-09-01; rescoped for #113)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
> **This entry was written for a failure it does not prevent.** Two different things in PVE are
|
||||
> called "lock": the **config lock** (the `lock:` property — `migrate`, `backup`, `clone`,
|
||||
> `snapshot` — a persisted config field, exposed as `lock` in `status/current`) and the
|
||||
> **flock** on `/var/lock/qemu-server/lock-<vmid>.conf` taken by `PVE::QemuConfig->lock_config`,
|
||||
> which is not exposed through the API at all. The integration failures were the flock; this
|
||||
> entry guards the config lock, which an ordinary start/stop never sets. Run 186 confirmed the
|
||||
> check never fired — `Restart-PveVm` took 4.11 s, unchanged. The flock is handled by **D016**
|
||||
> (serialise server-side where an endpoint exists) and **D020** (retry where none does). The
|
||||
> guard below is correct for the config lock and stays, but must never be described as covering
|
||||
> the flock.
|
||||
|
||||
### Decision
|
||||
`WaitForStatusTransition` returns only when the guest reports the expected status **and**
|
||||
its config lock has cleared. Reaching the status is not enough: PVE publishes the new
|
||||
status while the operation still holds `/var/lock/qemu-server/lock-<vmid>.conf`, and the
|
||||
next API call against that guest fails with `got timeout` trying to take the same lock.
|
||||
|
||||
`lock` is read from the `status/current` response the poll already fetches — it is present
|
||||
on both `qemu` and `lxc` status/current and has been since PVE 5.4, well below this
|
||||
module's 7.0 floor, so this costs no extra request.
|
||||
|
||||
If the guest still reports the expected status on the final poll but the lock outlasts
|
||||
`-Timeout`, the cmdlet returns success rather than throwing. The waited-for operation did
|
||||
complete; only the settling ran long. This keeps a call that succeeded before the change
|
||||
from becoming an exception after it.
|
||||
|
||||
That fallback tests the **most recent** observation, not "matched at some point during the
|
||||
wait". A guest that reached the expected status and then drifted away from it has not
|
||||
satisfied the wait and still raises `PveTaskTimeoutException`. A poll that fails outright
|
||||
leaves the previous observation standing, so a single API blip is not read as divergence.
|
||||
|
||||
This is the same family as D014: a PVE task completing does not mean the resource is ready
|
||||
for the next operation. D014 is the cluster-quorum instance, D015 the guest-lock instance.
|
||||
|
||||
### Rationale
|
||||
Integration run 183 failed four tests from one cause. `Restart-PveVm -Wait` returned after
|
||||
4.1 s having observed `running`; the following `Stop-PveVm` spent exactly 10.0 s failing to
|
||||
acquire the lock, which cascaded into the template convert, clone, and remove tests. Run 184,
|
||||
the same commit re-run, passed: its status poll happened to take 10.1 s, by which point the
|
||||
lock had cleared. The same settling happens either way — the only variable is whether the
|
||||
wait absorbs it or the next caller does.
|
||||
|
||||
The check lives in `WaitForStatusTransition` rather than in each cmdlet because all nine
|
||||
lifecycle call sites route through it.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER treat the status transition alone as "ready for the next operation"
|
||||
if (string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase))
|
||||
return task;
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus);
|
||||
if (snapshot.StatusMatched && !snapshot.Locked)
|
||||
return task;
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D016 — Restart-PveVm uses PVE's native reboot endpoint
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — found via integration runs 183/185/186, root-caused on a live PVE 9.2.2 node 2026-09-01)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
`Restart-PveVm` calls `POST /nodes/{node}/qemu/{vmid}/status/reboot` (`VmService.RebootVm`).
|
||||
It must **not** compose a restart client-side as `status/shutdown` followed by `status/start`.
|
||||
|
||||
### Rationale
|
||||
Composing the restart races Proxmox's own post-stop cleanup for the guest's config flock:
|
||||
|
||||
1. The shutdown completes and the QEMU process exits.
|
||||
2. `qmeventd` forks `/usr/sbin/qm cleanup <vmid> ...`.
|
||||
3. The client sees `status == stopped` and immediately posts `status/start`. `vm_start` takes
|
||||
the flock, wins the race, starts a **new** QEMU, releases.
|
||||
4. `qm cleanup` then takes the flock with a **60 s** timeout and polls `vm_running_locally`
|
||||
for up to **30 s**, holding it the whole time, because it sees the new PID as the old one
|
||||
failing to exit. PVE's own warning names this: `"QEMU process $pid for VM $vmid still
|
||||
running (or newly started)"`.
|
||||
5. Every subsequent call fails: `lock_config` defaults to **10 s**, so the client gets
|
||||
`can't lock file '/var/lock/qemu-server/lock-<vmid>.conf' - got timeout`.
|
||||
|
||||
Measured on a reproduction (integration run 187), three distinct source constants matching:
|
||||
|
||||
```
|
||||
qmstart ends t+3 <- qm cleanup takes the flock, sees the NEW pid
|
||||
qmstop #1 FAIL t+14 10 s = lock_config default
|
||||
qmstop #2 FAIL t+24 10 s = lock_config default
|
||||
qmclone FAIL t+25 1 s = qmclone's separate source-VM lock timeout
|
||||
qmstop #3 OK t+33 <- released; hold was t+3..t+33 = 30 s = cleanup's wait loop
|
||||
```
|
||||
|
||||
`vm_reboot` avoids all of it by holding the config lock across the entire shutdown and letting
|
||||
`qm cleanup` perform the restart while it already holds that same lock — there is no window for
|
||||
a client call to interleave.
|
||||
|
||||
This surfaced on PVE 9.2 and not 9.1 because of two May 2026 qemu-server changes (cleanup
|
||||
deduplication, shipped for 9.1.13, and the 30 s cleanup wait). Neither touches the REST surface,
|
||||
so the API changelog showed nothing — "the API did not change, therefore behaviour did not" is
|
||||
not a valid inference for this class of bug.
|
||||
|
||||
**Containers are not affected by this decision**: `/nodes/{node}/lxc/{vmid}/status/reboot` does
|
||||
not exist, so `Restart-PveContainer` necessarily keeps shutdown + start.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER compose a VM restart from two client calls — it races qmeventd's cleanup
|
||||
WaitForStatusTransition(session, node, () => vmService.ShutdownVm(session, node, vmid, timeout),
|
||||
vmid, "stopped", timeout);
|
||||
WaitForStatusTransition(session, node, () => vmService.StartVm(session, node, vmid),
|
||||
vmid, "running", timeout);
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
PveTask Issue() => vmService.RebootVm(session, node, vmid, timeout);
|
||||
|
||||
var task = Wait.IsPresent
|
||||
? WaitForStatusTransition(session, node, Issue, vmid, "running", timeout)
|
||||
: Issue();
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D017 — CI runs two lanes: a pinned gating lane and a report-only currency lane
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — arose from integration runs 176–180, root-caused 2026-09-01)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
CI provisions nested PVE nodes in two distinct modes, and they must not be merged into one:
|
||||
|
||||
- **Lane 1, `integration-tests.yml`** — nodes stay pinned to what the ISO ships. `first-boot.sh`
|
||||
must never run `apt-get upgrade` or `dist-upgrade`. This lane gates merges.
|
||||
- **Lane 2, `package-currency.yml`** — nodes are `dist-upgrade`d to current PVE and the suite runs
|
||||
against them. **Report-only**: test failures do not fail the job.
|
||||
|
||||
Both declare `concurrency: group: integration-tests`. They drive the same nested VMIDs on the same
|
||||
parent node, so they must never run at once.
|
||||
|
||||
### Rationale
|
||||
`apt-get upgrade` holds back packages that need new dependencies. On the nested nodes that produced
|
||||
`pve-cluster` 9.1.6 against `libpve-cluster-api-perl` 9.1.0 — a combination no real install ever
|
||||
has — and its symptom was not a package error. The node's pmxcfs came back in local mode after a
|
||||
cluster join, `/etc/pve/corosync.conf` never appeared, and the node reported `online=0` while
|
||||
corosync itself had healthy 2-node membership. Three CI runs went into diagnosing that, and removing
|
||||
the upgrade was the entire fix: run 180 was the first fully green integration run.
|
||||
|
||||
So the pin is what makes lane 1 a trustworthy merge gate. But a permanently pinned CI never
|
||||
exercises the module against a current PVE, and that gap is exactly where an upstream regression
|
||||
would hide. Lane 2 closes it without putting the instability back into the gating path.
|
||||
|
||||
Report-only is deliberate. A scheduled job that goes red on an upstream change nobody has chosen to
|
||||
chase becomes noise, and a noisy cron gets ignored — which is the failure mode that makes a canary
|
||||
worthless. The signal is the rolling issue and the recorded package set, not the check colour.
|
||||
|
||||
**Consequence accepted**: a module genuinely broken against current PVE shows a green weekly check
|
||||
plus an updated issue. Operator ruling 2026-09-01, to be revisited after a few releases.
|
||||
|
||||
A failure of the lane's own machinery — provisioning, the upgrade, the reboot, an unreachable node —
|
||||
still fails the job. `run-integration.sh` returns 3 for a genuine test failure and 4 when it cannot
|
||||
reach or authenticate to a node; only 3 is suppressed. Suppressing both would let a botched reboot
|
||||
report success while the lane learned nothing.
|
||||
|
||||
### Amendment 2026-09-01 — the pin covers the test tooling, not just the nested PVE packages
|
||||
|
||||
The gating lane's own tooling is pinned by exact version for the same reason its nested PVE packages
|
||||
are: `Pester` in `tests/Dockerfile.test` (`ARG PESTER_VERSION`) and in `.github/workflows/unit-tests.yml`
|
||||
(`env.PESTER_VERSION`), installed and imported with `-RequiredVersion` at every site, including the
|
||||
suite's own import inside the container. The Dockerfile promotes the ARG to `ENV` so the version is
|
||||
discoverable at runtime. Both files must name the same version, and `shell-selfchecks` asserts it.
|
||||
|
||||
Before this, both sites used `-MinimumVersion 5.0` with no ceiling. The image is rebuilt on every CI
|
||||
run and Pester is installed fresh on every unit-test run, so PSGallery decided the version — a new
|
||||
major could reach the merge gate with no commit to this repository, surfacing as unexplained test
|
||||
breakage on whichever PR happened to run next. That is the same class of moving input the lane split
|
||||
exists to eliminate; the difference is only that it moves in the test runner rather than in PVE.
|
||||
|
||||
It had already happened silently: steps named "Install Pester 5" were resolving 6.1.0 on both the
|
||||
PowerShell 5.1 and 7.x legs, because Pester 6 declares `PowerShellVersion 5.1` and so installs on
|
||||
Windows PowerShell too. Nothing broke — the suite uses only constructs common to 5 and 6 — but
|
||||
nobody chose it.
|
||||
|
||||
Bumping is a deliberate commit that changes both files together.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```bash
|
||||
# NEVER in first-boot.sh — this is the mismatch that left a node unclustered
|
||||
apt-get update -qq
|
||||
apt-get -y upgrade
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```bash
|
||||
# first-boot.sh installs only what provisioning needs; the ISO is the pin
|
||||
apt-get update -qq
|
||||
apt-get install -y -qq --no-install-recommends qemu-guest-agent open-iscsi
|
||||
```
|
||||
```yaml
|
||||
# package-currency.yml opts in explicitly; lane 1 never sets this
|
||||
env:
|
||||
PVE_DIST_UPGRADE: '1'
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D018 — The currency lane reboots after dist-upgrade, and proves it rebooted
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — found in pre-push review of PR #106, 2026-09-01)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
After `dist-upgrade`, `prepare-test-environment.sh` reboots the node **unconditionally** and then
|
||||
**verifies the reboot happened** by comparing `/proc/sys/kernel/random/boot_id` before and after.
|
||||
An unchanged boot id is fatal. The reboot lives here, not in `first-boot.sh`.
|
||||
|
||||
### Rationale
|
||||
A PVE `dist-upgrade` pulls `proxmox-kernel-*`. Without a reboot the node runs new userspace on the
|
||||
old kernel, so the lane records a package set it never actually ran and is blind to kernel
|
||||
regressions — it would report "current PVE" while testing something that never booted.
|
||||
|
||||
The reboot cannot live in `first-boot.sh`: that runs `ordering = "fully-up"` while the parent is
|
||||
still polling, so `wait-for-pve.sh` can discover the IP, see the API, pass auth, and then have the
|
||||
node reboot out from under provisioning. That presents as an intermittent network fault.
|
||||
|
||||
It is unconditional rather than gated on `/var/run/reboot-required`, because that file comes from
|
||||
`update-notifier-common`, which is not guaranteed present on a PVE node.
|
||||
|
||||
Verification is the part that is easy to omit and was omitted in the first draft. `ssh … reboot`
|
||||
returns non-zero when the connection dies, so it needs `|| true` — which swallows *every* ssh
|
||||
failure, including the reboot never being issued. `wait-for-api.sh` then matches the
|
||||
**still-running pre-reboot** pveproxy on its first poll and returns `responsive after 0s`. The
|
||||
script exits 0 having proved nothing. A blind `sleep` before polling does not fix this; it is wrong
|
||||
in both directions and verifies nothing either way.
|
||||
|
||||
Order matters: prove the boot id changed first (ssh returns before pveproxy does), then wait for the
|
||||
API, then wait for pmxcfs — `pvesm set` writes `/etc/pve/storage.cfg`, which needs `/etc/pve`
|
||||
mounted, and on a fresh boot that lags the API by seconds.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```bash
|
||||
# NEVER — `|| true` hides a reboot that never happened, and the poll then
|
||||
# matches the pre-reboot node and returns immediately
|
||||
${SSH_CMD} "systemctl reboot" || true
|
||||
sleep 30
|
||||
bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```bash
|
||||
boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")"
|
||||
${SSH_CMD} "systemctl reboot" || true
|
||||
boot_after=""
|
||||
for _ in $(seq 1 60); do
|
||||
boot_after="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id" 2>/dev/null || true)"
|
||||
[[ -n "${boot_after}" && "${boot_after}" != "${boot_before}" ]] && break
|
||||
sleep 5
|
||||
done
|
||||
if [[ -z "${boot_after}" || "${boot_after}" == "${boot_before}" ]]; then
|
||||
echo "ERROR: ${NESTED_IP} did not reboot (boot_id unchanged)" >&2
|
||||
exit 1
|
||||
fi
|
||||
bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D019 — Local dev calls run-integration.sh directly; there is no wrapper script
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — found auditing the local dev path against the post-ARC CI, 2026-09-01)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
`tests/infrastructure/scripts/run-integration.sh` is the only entry point to the
|
||||
provision → test → cleanup lifecycle, for CI and for local development alike. Local runs
|
||||
invoke it inside the `dev-infra` container — the same image CI runs its jobs in. Do not add
|
||||
a convenience wrapper around it.
|
||||
|
||||
Build and unit tests need no container at all; they run natively against the solution.
|
||||
|
||||
### Rationale
|
||||
`tests/dev.ps1` was a 291-line PowerShell wrapper over roughly six `docker compose` and
|
||||
`docker exec` calls. Every capability it had was already available elsewhere: build and unit
|
||||
tests are plain `dotnet` and `Invoke-Pester` invocations, and the module build it performed
|
||||
is duplicated inside `run-integration.sh` itself, which publishes and installs the module
|
||||
before running the suite.
|
||||
|
||||
Being a second entry point, it drifted from the script it wrapped and from the CI it claimed
|
||||
to replicate. By the time it was removed it still offered a `-Version 8` leg retired in #88,
|
||||
mounted the Docker socket for storage containers replaced by the storage VM in #87, and
|
||||
defaulted its remote-host examples to a runner decommissioned in the ARC migration.
|
||||
|
||||
Four documentation files described a positional calling convention (`./tests/dev.ps1 test`)
|
||||
that did not do what it read as. The script took its actions from switches (`-Test`), but
|
||||
also declared `[string[]] $Tests`, so the bare word bound to `-Tests` — the integration-area
|
||||
filter. With no action switch set, the script then fell through to its `-Shell` default and
|
||||
silently opened an interactive container shell. Every documented command was wrong, and
|
||||
wrong in the quietest possible way: it succeeded at something nobody asked for.
|
||||
|
||||
A wrapper that must be kept in sync with the thing it wraps earns its place only when it
|
||||
removes real friction. This one removed none.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
A `dev.ps1`, `Makefile` target, or shell function that re-implements provisioning steps,
|
||||
module installation, or test invocation. If a local flow is awkward, fix it in
|
||||
`run-integration.sh` so CI gets the fix too.
|
||||
|
||||
---
|
||||
|
||||
## D020 — The qemu-server flock is retried, never predicted
|
||||
|
||||
**Status**: Active
|
||||
**Finding refs**: (none — issue #113, reproduced 2026-09-01 on a Rosetta-emulated client)
|
||||
**Resolved in scan**: n/a
|
||||
|
||||
### Decision
|
||||
An operation PVE rejects with `can't lock file '<guest lock path>' - got timeout` is reissued
|
||||
for a bounded window (`GuestLockRetry.DefaultWindow`, 45 s). Nothing in the module may attempt
|
||||
to *detect* that the flock is held before acting.
|
||||
|
||||
Two seams implement it, and both are required:
|
||||
|
||||
- **`PveHttpClient.SendAsync`** — retries the request itself. This covers every operation PVE
|
||||
serialises inside the API handler, where the failure arrives as a 500: `Set-PveVmConfig`,
|
||||
`Resize-PveVmDisk`'s config writes, and every future call that goes through the client.
|
||||
The private send takes a `Func<HttpRequestMessage>` rather than a request because an
|
||||
`HttpRequestMessage` cannot be sent twice.
|
||||
- **`PveCmdletBase.InvokeGuestTask`** — reissues the API call *and* re-waits its task. PVE takes
|
||||
the flock inside the forked worker for most guest operations (`qmreset`, `qmclone`), so the
|
||||
POST returns 200 with a UPID and the failure only appears in the task's exit status. The HTTP
|
||||
layer cannot see it and cannot retry it. `WaitForStatusTransition` routes through this helper,
|
||||
which is why it takes a `Func<PveTask>` instead of an already-issued `PveTask`.
|
||||
|
||||
Reissuing is safe **only** for a failure to *enter* `lock_config`, which PVE raises before the
|
||||
operation does any work. `GuestLockRetry.IsLockTimeout` must keep both properties that establish
|
||||
this, and no failure may be added to it without them:
|
||||
|
||||
- **Path-specific.** `PVE::Tools::lock_file` emits the identical wording for storage, LVM, HA,
|
||||
backup and firewall locks. Those are taken mid-worker and carry no such guarantee, so the match
|
||||
names the two guest config paths (`/var/lock/qemu-server/lock-<vmid>.conf`,
|
||||
`/run/lock/lxc/pve-config-<vmid>.lock`) rather than the generic phrasing.
|
||||
- **Anchored at the start of what PVE said.** `qmclone` is the operation that makes this matter:
|
||||
its worker creates and locks the target config, allocates disks, then re-locks. A timeout at one
|
||||
of those later points reads the same as one at entry, and reissuing it would hit
|
||||
`check_vmid_unused` — "VM <newid> already exists" — leaving an orphaned guest behind. PVE
|
||||
prefixes the late form with its own context (`clone failed: ...`), so anchoring rejects it.
|
||||
`Resize-PveVmDisk -Size '+1G'` is the case where getting this wrong is irreversible rather than
|
||||
merely messy.
|
||||
|
||||
The anchor only works against the raw text, so the predicate reads
|
||||
`PveTaskFailedException.ExitStatus` and `PveApiException.ApiMessage` — never `Exception.Message`,
|
||||
which both types prefix with their own context. `ApiMessage` exists for this.
|
||||
|
||||
### Rationale
|
||||
D015 tried to predict the lock and guarded the wrong one (see its note). D016 removed the race
|
||||
for `Restart-PveVm` by handing the ordering to PVE, but that only works where a server-side
|
||||
serialised endpoint exists. `Set-PveVmConfig`, `Resize-PveVmDisk` and clone have none, so for
|
||||
them the choice is retry or nothing.
|
||||
|
||||
The window is 45 s because `qm cleanup` holds the flock while polling `vm_running_locally` for
|
||||
up to 30 s, and each rejected attempt first burns PVE's own 10 s `lock_config` timeout. It bounds
|
||||
when a *new* attempt may start, not total wall clock: an attempt beginning just inside the window
|
||||
still runs to its own conclusion, so the real ceiling is roughly one attempt longer.
|
||||
|
||||
Two consequences are deliberate, and both are load-bearing enough to state rather than discover:
|
||||
|
||||
- **`-Timeout` does not bound the retry.** It is documented as the budget for the status
|
||||
transition, and `WaitForStatusTransition` starts counting it only after the operation's task
|
||||
completes. Binding the retry to it would defeat the fix at exactly the values that need it —
|
||||
`Reset-PveVm -Wait -Timeout 30` needed ~31 s of retrying in the run that verified this change.
|
||||
- **The two seams nest.** A cmdlet operation rejected synchronously burns the HTTP layer's window
|
||||
inside `InvokeGuestTask`'s. The overlap costs a longer wait before the same failure, never a
|
||||
different outcome, so it is not worth threading a shared budget through both layers.
|
||||
|
||||
CI never showed this. Runs 189–200 were green because the CI client is fast enough to win the
|
||||
race. It reproduces on a client roughly 40% slower — the CI container image run under Docker
|
||||
Desktop's Rosetta emulation on Apple Silicon — which failed `Should hard-reset a running VM`,
|
||||
`Should clone a VM` and `Should resize a VM disk (Resize-PveVmDisk)` on the same commit CI
|
||||
passed. **A green CI run is not evidence about this class of bug.**
|
||||
|
||||
### Not yet adopted
|
||||
`InvokeGuestTask` is the correct seam for every cmdlet that issues a guest operation and waits
|
||||
on its task. `Remove-PveVm`, `Move-PveVm`, the snapshot and template cmdlets, and the container
|
||||
equivalents still call `TaskService.WaitForTask` directly and remain exposed to the same race.
|
||||
They adopt the helper as they are next touched.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// NEVER try to observe the flock — PVE does not expose it in status/current or anywhere else
|
||||
if (!snapshot.Locked)
|
||||
return task; // reads the config `lock:` property; says nothing about the flock
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
```csharp
|
||||
PveTask Issue() => vmService.CloneVm(session, sourceNode, vmid, newid, name, targetNode, full);
|
||||
|
||||
var task = Wait.IsPresent
|
||||
? InvokeGuestTask(session, sourceNode, Issue)
|
||||
: Issue();
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## D021 — Integration tests prove server semantics; payloads are proven offline
|
||||
|
||||
**Status**: Active
|
||||
**Issue refs**: #120 (coverage), #92/#118 (the case that motivated it)
|
||||
**Decided**: 2026-09-02
|
||||
|
||||
### Decision
|
||||
An integration test must earn its place by testing something only a live PVE can answer.
|
||||
Request-payload correctness — which keys a cmdlet sends, and with what values — is verified
|
||||
offline against the mock `IPveHttpClient` harness.
|
||||
|
||||
Concretely:
|
||||
- New cmdlets route through a `*Service` that accepts `IPveHttpClient`, so their payload is
|
||||
reachable from `PSProxmoxVE.Core.Tests` without a cluster.
|
||||
- The 37 cmdlets that construct `PveHttpClient` directly are converted to that seam before the
|
||||
next large coverage push, and opportunistically when otherwise touched. Measured against 194
|
||||
concrete cmdlet files (`src/PSProxmoxVE/Cmdlets/**/*.cs` less the `PveCmdletBase` base class):
|
||||
155 reach the API only through a `*Service`, 25 only through their own client, 12 do both, and
|
||||
2 do neither. The service and direct-client sets overlap, so they do not sum to 194.
|
||||
- The integration suite is tiered: a PR exercises smoke plus the areas its diff touches
|
||||
(`run-integration.sh test <ver> <Area>` already supports this); the full suite runs on merge
|
||||
to `main`.
|
||||
- Areas whose dependencies cannot exist in CI (ACME needs a CA plus DNS or HTTP reachability)
|
||||
are covered by mock/contract tests asserting request shape, not by a live lane.
|
||||
- Areas needing a differently-shaped cluster (Ceph needs dedicated block devices per node and
|
||||
wants three monitors) live behind an opt-in provisioning profile, so ordinary runs do not pay
|
||||
for them.
|
||||
|
||||
### Rationale
|
||||
`Set-PveNetwork` sent `bridge_vlan_aware=0` to clear a VLAN-aware bridge. The API schema
|
||||
advertises a plain optional boolean, so the request is valid and returns success — and PVE
|
||||
merges supplied keys onto the stored stanza and ignores the `0`. The flag never cleared. Only
|
||||
`delete=bridge_vlan_aware` works, and only a run against a real PVE 9 revealed it.
|
||||
|
||||
That is what a live cluster is for: server behaviour the schema misdescribes. It is not for
|
||||
checking that a dictionary has the right keys, which a mock proves in milliseconds.
|
||||
|
||||
The distinction matters because it decides whether the suite scales. Coverage is ~36% of 678
|
||||
endpoints; the remaining surface is large enough that "every new endpoint gets a live test" puts
|
||||
the integration suite on a growth curve the CI budget cannot absorb. Growth is linear in
|
||||
*live-only* surface, and how much surface is live-only is a design choice, not a given.
|
||||
|
||||
### Anti-pattern (do not reintroduce)
|
||||
```csharp
|
||||
// A cmdlet that builds its own form and owns its own client has no offline seam:
|
||||
// every field below is verifiable only by provisioning a cluster.
|
||||
using var client = new PveHttpClient(session);
|
||||
var data = new Dictionary<string, string> { ["type"] = Type };
|
||||
if (!string.IsNullOrEmpty(Address)) data["address"] = Address!;
|
||||
client.PutAsync($"nodes/{node}/network/{iface}", data).GetAwaiter().GetResult();
|
||||
```
|
||||
|
||||
### Correct pattern
|
||||
This is the target shape, not a form the tree already takes. `Set-PveNetwork` is one of the 37,
|
||||
and `NetworkService.SetNetwork` has no callers anywhere in the repository.
|
||||
|
||||
```csharp
|
||||
// Service takes IPveHttpClient, so PSProxmoxVE.Core.Tests can assert the emitted form
|
||||
// without a cluster; the integration test then covers only what PVE alone can tell us.
|
||||
var service = new NetworkService();
|
||||
service.SetNetwork(session, Node, Iface, config);
|
||||
```
|
||||
**This file has moved.** Architectural decisions now live in [`docs/decisions/`](docs/decisions/) as house-format ADRs, one decision per file. See [ADR 0023](docs/decisions/0023-decisions-live-in-docs-decisions-in-house-adr-format.md) for why.
|
||||
|
||||
New decisions are generated from the repository root with `~/.claude/templates/new-adr.sh "Title in plain words"`, which stamps the next number, the filename and the section skeleton. Do not add entries to this file.
|
||||
|
||||
The convention checklist — the rules to follow when writing a cmdlet — is `CLAUDE.md` § "Key Conventions". The ADRs carry the rationale behind those rules.
|
||||
|
||||
## D-number redirects
|
||||
|
||||
The old `DNNN` identifiers are retired. They appear in released `CHANGELOG.md` entries and in GitHub issues written before 2026-09-02, neither of which is rewritten. This table maps them.
|
||||
|
||||
| Old | ADR |
|
||||
|---|---|
|
||||
| D001 | [0001 — Task polling must use TaskService.WaitForTask](docs/decisions/0001-task-polling-must-use-taskservice-waitfortask.md) |
|
||||
| D002 | [0002 — Password parameters must use SecureString](docs/decisions/0002-password-parameters-must-use-securestring.md) |
|
||||
| D003 | [0003 — URL encoding required for all path parameters](docs/decisions/0003-url-encoding-required-for-all-path-parameters.md) |
|
||||
| D004 | [0004 — No bare catch blocks](docs/decisions/0004-no-bare-catch-blocks.md) |
|
||||
| D005 | [0005 — OutputType required on all cmdlets](docs/decisions/0005-outputtype-required-on-all-cmdlets.md) |
|
||||
| D006 | [0006 — ConfirmImpact.High required for destructive operations](docs/decisions/0006-confirmimpact-high-required-for-destructive-operations.md) |
|
||||
| D007 | [0007 — All cmdlet classes must be sealed](docs/decisions/0007-all-cmdlet-classes-must-be-sealed.md) |
|
||||
| D008 | [0008 — JSON serialisation is Newtonsoft.Json only](docs/decisions/0008-json-serialisation-is-newtonsoft-json-only.md) |
|
||||
| D009 | [0009 — Framework targeting](docs/decisions/0009-framework-targeting-netstandard2-0-for-publishable-net10-0-and-net48-for-tests.md) |
|
||||
| D010 | [0010 — VmId parameters are nullable int with ValidateRange](docs/decisions/0010-vmid-parameters-are-nullable-int-with-validaterange.md) |
|
||||
| D011 | [0011 — Verb class constants required for cmdlet attributes](docs/decisions/0011-verb-class-constants-required-for-cmdlet-attributes.md) |
|
||||
| D012 | [0012 — Magic strings are extracted to named constants](docs/decisions/0012-magic-strings-are-extracted-to-named-constants.md) |
|
||||
| D013 | [0013 — Cmdlets must emit only native or module-defined types](docs/decisions/0013-cmdlets-must-emit-only-native-or-module-defined-types.md) |
|
||||
| D014 | [0014 — New-PveCluster -Wait blocks until the cluster is quorate](docs/decisions/0014-new-pvecluster-wait-blocks-until-the-cluster-is-quorate.md) |
|
||||
| D015 | [0015 — Lifecycle -Wait blocks until the guest config lock clears](docs/decisions/0015-lifecycle-wait-blocks-until-the-guest-config-lock-clears.md) |
|
||||
| D016 | [0016 — Restart-PveVm uses PVE's native reboot endpoint](docs/decisions/0016-restart-pvevm-uses-pve-s-native-reboot-endpoint.md) |
|
||||
| D017 | [0017 — CI runs two lanes: a pinned gating lane and a report-only currency lane](docs/decisions/0017-ci-runs-two-lanes-a-pinned-gating-lane-and-a-report-only-currency-lane.md) |
|
||||
| D017 (amendment) | [0022 — The gating lane pins its own test tooling by exact version](docs/decisions/0022-the-gating-lane-pins-its-own-test-tooling-by-exact-version.md) |
|
||||
| D018 | [0018 — The currency lane reboots after dist-upgrade, and proves it rebooted](docs/decisions/0018-the-currency-lane-reboots-after-dist-upgrade-and-proves-it-rebooted.md) |
|
||||
| D019 | [0019 — Local dev calls run-integration.sh directly](docs/decisions/0019-local-dev-calls-run-integration-sh-directly-there-is-no-wrapper-script.md) |
|
||||
| D020 | [0020 — The qemu-server flock is retried, never predicted](docs/decisions/0020-the-qemu-server-flock-is-retried-never-predicted.md) |
|
||||
| D021 | [0021 — Integration tests prove server semantics; payloads are proven offline](docs/decisions/0021-integration-tests-prove-server-semantics-payloads-are-proven-offline.md) |
|
||||
|
||||
@@ -0,0 +1,37 @@
|
||||
# ADR 0001 — Task polling must use TaskService.WaitForTask
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F032, F033, F036, F058
|
||||
|
||||
## Context
|
||||
|
||||
Four VM/network cmdlets (`Invoke-PveNetworkApply`, `New-PveSnapshot`, `Restore-PveSnapshot`, `Remove-PveSnapshot`) and one guest exec cmdlet had copy-pasted polling loops with no timeout, so a cmdlet hung indefinitely if a PVE task stalled.
|
||||
|
||||
`TaskService.WaitForTask` already has timeout enforcement, failure detection and `WriteProgress` support. Every inline loop was a worse reimplementation of it.
|
||||
|
||||
## Decision
|
||||
|
||||
All task-polling loops use `TaskService.WaitForTask(upid, session, timeout, progress)`. No cmdlet file implements its own `while(true)` or `do`/`while` polling.
|
||||
|
||||
```csharp
|
||||
TaskService.WaitForTask(upid, session, TimeoutSeconds, this);
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
An inline poll in the cmdlet. It carries no timeout, no failure detection and no progress reporting, and each copy drifts from the others:
|
||||
|
||||
```csharp
|
||||
while (true)
|
||||
{
|
||||
var status = taskService.GetTask(upid, session);
|
||||
if (status.IsFinished) break;
|
||||
Thread.Sleep(1000);
|
||||
}
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
Five cmdlets — three container snapshot, two storage — still carried the inline form at scan 2026-03-22, and were converted on 2026-03-23 (F058, resolved).
|
||||
@@ -0,0 +1,48 @@
|
||||
# ADR 0002 — Password parameters must use SecureString
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F051
|
||||
|
||||
## Context
|
||||
|
||||
`Set-PveVmGuestPassword` accepted a plain `string` password parameter, leaving the credential in managed memory indefinitely.
|
||||
|
||||
`Connect-PveServer` already took a `PSCredential`, so the module was inconsistent with itself about how sensitive input arrives.
|
||||
|
||||
## Decision
|
||||
|
||||
Every cmdlet parameter that accepts a password is a `SecureString`, extracted with `Marshal.SecureStringToGlobalAllocUnicode` and freed with `ZeroFreeGlobalAllocUnicode` in a `finally`.
|
||||
|
||||
```csharp
|
||||
[Parameter(Mandatory = true)]
|
||||
public SecureString Password { get; set; }
|
||||
|
||||
IntPtr ptr = IntPtr.Zero;
|
||||
try
|
||||
{
|
||||
ptr = Marshal.SecureStringToGlobalAllocUnicode(Password);
|
||||
string plainText = Marshal.PtrToStringUni(ptr);
|
||||
}
|
||||
finally
|
||||
{
|
||||
if (ptr != IntPtr.Zero)
|
||||
Marshal.ZeroFreeGlobalAllocUnicode(ptr);
|
||||
}
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
A plain `string` parameter. It is simpler to write and to test, and it leaves the credential recoverable from a memory dump for the lifetime of the process:
|
||||
|
||||
```csharp
|
||||
[Parameter(Mandatory = true)]
|
||||
public string Password { get; set; }
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
The service layer below the cmdlet still receives a plain `string` — the conversion happens at the cmdlet boundary, and `ClusterConfigService.JoinCluster` documents that it expects the converted value. The guarantee is about the module's public surface and the window of exposure, not about the credential never existing in managed memory.
|
||||
|
||||
A TLS private key is at least as sensitive as a password; anything accepting one is covered by the same rule.
|
||||
@@ -0,0 +1,30 @@
|
||||
# ADR 0003 — URL encoding required for all path parameters
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F050
|
||||
|
||||
## Context
|
||||
|
||||
Snapshot names, node names, user IDs and similar identifiers were interpolated raw into API URL paths. Most reach the module from validated sources, but nothing in the code enforced that, and a value carrying `/` or `?` would silently change which endpoint was called.
|
||||
|
||||
## Decision
|
||||
|
||||
Every user-supplied or dynamic value interpolated into an API URL path is wrapped in `Uri.EscapeDataString()`. This applies to all service classes without exception.
|
||||
|
||||
```csharp
|
||||
var resource = $"nodes/{Uri.EscapeDataString(node)}/qemu/{vmid}/snapshot/{Uri.EscapeDataString(snapshotName)}";
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Encoding only the parameters that can plausibly carry a separator, and trusting validation upstream for the rest. Rejected because the audit then has to be redone on every new call site, and the reader cannot tell a deliberate omission from an oversight:
|
||||
|
||||
```csharp
|
||||
var resource = $"nodes/{node}/qemu/{vmid}/snapshot/{snapshotName}";
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
Applied across all 14 service classes at the time of the decision. Form-encoded *bodies* are a separate matter — PVE does not URL-decode form values in some internal consumers, so the cluster-join path deliberately sends minimally encoded values.
|
||||
@@ -0,0 +1,41 @@
|
||||
# ADR 0004 — No bare catch blocks
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F039
|
||||
|
||||
## Context
|
||||
|
||||
Bare catches in `PveHttpClient`, `PveCmdletBase`, `VmService`, `ContainerService` and `GetPveVmCmdlet` swallowed every error, including ones that had nothing to do with the transient failure the catch was written for. A misconfigured endpoint and a stalled task presented identically: as silence.
|
||||
|
||||
## Decision
|
||||
|
||||
No `catch { }` and no unfiltered `catch (Exception) { }`. Every catch either names a specific exception type, or filters with a `when` clause that excludes fatal exceptions.
|
||||
|
||||
```csharp
|
||||
catch (PveApiException ex) { WriteWarning(ex.Message); }
|
||||
|
||||
catch (Exception ex) when (ex is not OutOfMemoryException and not StackOverflowException)
|
||||
{
|
||||
WriteVerbose($"Status poll failed: {ex.Message}");
|
||||
}
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Catching everything and continuing, on the theory that a status poll failing is never worth surfacing:
|
||||
|
||||
```csharp
|
||||
try { ... }
|
||||
catch { }
|
||||
|
||||
try { ... }
|
||||
catch (Exception) { /* ignore */ }
|
||||
```
|
||||
|
||||
Rejected because it also swallows `OutOfMemoryException` and `StackOverflowException`, and because "this particular call is allowed to fail quietly" is a claim that has to be re-checked whenever the body of the `try` grows.
|
||||
|
||||
## Consequences
|
||||
|
||||
Filtered catches still need somewhere for the message to go — `WriteVerbose` at minimum — or the filter merely moves the silence. This regressed once: F039 was reopened after bare catches reappeared in `VmService.PingGuestAgent` and `Import-PveOva`'s VM-retrieval fallback, and was fixed again.
|
||||
@@ -0,0 +1,28 @@
|
||||
# ADR 0005 — OutputType required on all cmdlets
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F037
|
||||
|
||||
## Context
|
||||
|
||||
Around 54 of the module's 169 cmdlets had no `[OutputType]` attribute. PowerShell uses it for IntelliSense, for pipeline type inference, and to answer `Get-Command -OutputType`; without it, tooling cannot tell what a cmdlet emits until it runs.
|
||||
|
||||
## Decision
|
||||
|
||||
Every cmdlet declares its return type with `[OutputType(typeof(...))]`.
|
||||
|
||||
```csharp
|
||||
[Cmdlet(VerbsCommon.Get, "PveVm")]
|
||||
[OutputType(typeof(VmInfo))]
|
||||
public sealed class GetPveVmCmdlet : PveCmdletBase
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
None recorded. This was adopted as a convention during review scan 2026-03-22 rather than chosen between competing options. All 169 cmdlets carry the attribute.
|
||||
|
||||
## Consequences
|
||||
|
||||
The attribute is only as useful as the type it names, which is what [ADR 0013](0013-cmdlets-must-emit-only-native-or-module-defined-types.md) constrains: an `[OutputType(typeof(JObject))]` satisfies this rule and still gives the user nothing discoverable.
|
||||
@@ -0,0 +1,29 @@
|
||||
# ADR 0006 — ConfirmImpact.High required for destructive operations
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F011, F034, F042, F043, F062, F063
|
||||
|
||||
## Context
|
||||
|
||||
`Stop-PveVm`, `Reset-PveVm`, `Suspend-PveVm`, `Restart-PveVm` and `Remove-PveRole` did not set `ConfirmImpact.High`, so a user could perform a disruptive operation without being prompted — including against the wrong guest.
|
||||
|
||||
## Decision
|
||||
|
||||
Every cmdlet performing a destructive or disruptive operation sets `ConfirmImpact = ConfirmImpact.High`. That covers all `Remove-*`, `Stop-*`, `Reset-*`, `Restart-*` and `Suspend-*` cmdlets, plus `Restore-PveSnapshot`, `Restore-PveContainerSnapshot`, and `New-PveTemplate` because the conversion is irreversible.
|
||||
|
||||
```csharp
|
||||
[Cmdlet(VerbsLifecycle.Stop, "PveVm", SupportsShouldProcess = true,
|
||||
ConfirmImpact = ConfirmImpact.High)]
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
None recorded. The rule states which verbs qualify rather than choosing between options; the open question at the time was only which cmdlets had been missed.
|
||||
|
||||
## Consequences
|
||||
|
||||
The container counterparts `Restart-PveContainer` and `Suspend-PveContainer` remained inconsistent with their VM equivalents at scan 2026-03-22 (F062, F063).
|
||||
|
||||
A cmdlet whose danger is not obvious from its verb needs the same treatment. The module has no cmdlet for HA `disarm-ha` yet; when one is added it will be a `Disable-`/`Invoke-` verb that this rule's verb list does not cover, while releasing every watchdog in the cluster.
|
||||
@@ -0,0 +1,26 @@
|
||||
# ADR 0007 — All cmdlet classes must be sealed
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F041
|
||||
|
||||
## Context
|
||||
|
||||
Around 95 of the module's 169 cmdlet classes were not `sealed`. Cmdlets in this module are leaves — they derive from `PveCmdletBase` and nothing derives from them — but the code did not say so.
|
||||
|
||||
## Decision
|
||||
|
||||
Every cmdlet class is declared `sealed`.
|
||||
|
||||
```csharp
|
||||
public sealed class GetPveVmCmdlet : PveCmdletBase
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
None recorded. Adopted as a convention during review scan 2026-03-22. Beyond making the design intent explicit, sealing enables potential JIT devirtualisation. All 169 cmdlets are now sealed.
|
||||
|
||||
## Consequences
|
||||
|
||||
Applies to cmdlets only. `PveCmdletBase` is the shared base and is deliberately not sealed; a rule stated as "all cmdlet classes" has to exclude it, and a mechanical check that does not will produce a false positive on every run.
|
||||
@@ -0,0 +1,35 @@
|
||||
# ADR 0008 — JSON serialisation is Newtonsoft.Json only
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F044
|
||||
|
||||
## Context
|
||||
|
||||
Model classes carried both `[JsonProperty]` (Newtonsoft) and `[JsonPropertyName]` (System.Text.Json) attributes. Only Newtonsoft runs at runtime, so the second set was inert — but a reader could not tell which one the deserialiser honoured, and changing one without the other would look correct and do nothing.
|
||||
|
||||
## Decision
|
||||
|
||||
Newtonsoft.Json is the only JSON library. Model classes carry `[JsonProperty]` and nothing else.
|
||||
|
||||
```csharp
|
||||
[JsonProperty("status")]
|
||||
public string Status { get; set; }
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Carrying both attribute sets so a future migration to System.Text.Json is already half-done:
|
||||
|
||||
```csharp
|
||||
[JsonProperty("status")]
|
||||
[JsonPropertyName("status")]
|
||||
public string Status { get; set; }
|
||||
```
|
||||
|
||||
Rejected because the unused set is never exercised, so it rots silently, and it makes every property read as though two serialisers are in play.
|
||||
|
||||
## Consequences
|
||||
|
||||
All `[JsonPropertyName]` attributes were removed. A `System.Text.Json` package reference survived the removal for the netstandard2.0 and net48 targets with no source using it — an unused dependency on the published surface, filed separately.
|
||||
+27
@@ -0,0 +1,27 @@
|
||||
# ADR 0009 — Framework targeting: netstandard2.0 for publishable, net10.0 and net48 for tests
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F047, F064. The date is the scan that recorded it; no decision date was captured.
|
||||
|
||||
## Context
|
||||
|
||||
.NET 9.0 reached end of life in May 2025, and the test projects still targeted it.
|
||||
|
||||
The publishable module has a harder constraint than the tests do: it must load in both Windows PowerShell 5.1 (Desktop) and PowerShell 7.x (Core), which only `netstandard2.0` satisfies.
|
||||
|
||||
## Decision
|
||||
|
||||
- Publishable projects (`PSProxmoxVE`, `PSProxmoxVE.Core`) target `netstandard2.0` and nothing else.
|
||||
- Test projects target `net10.0` (current LTS) and `net48` (to validate the Windows PowerShell 5.1 path).
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Multi-targeting the publishable projects as `netstandard2.0;net10.0;net48`. It was briefly in place and inflates the published module with framework-specific assemblies PowerShell will not use, for no compatibility gain over `netstandard2.0` alone.
|
||||
|
||||
## Consequences
|
||||
|
||||
The net9.0 → net10.0 move on the test projects was still outstanding when this was recorded, alongside two related dependency pins the same decision governs: the `System.Management.Automation` pin (F064) and the workflow SDK versions (F073, F079). All are now resolved.
|
||||
|
||||
Anything the module needs that `netstandard2.0` lacks has to be polyfilled or avoided; that constraint does not apply to test code, which is why the split exists.
|
||||
@@ -0,0 +1,38 @@
|
||||
# ADR 0010 — VmId parameters are nullable int with ValidateRange
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scans 2026-03-21 (ValidateRange) and 2026-03-22 (nullable)
|
||||
- **Context source:** `docs/review/findings.json` F012, F038
|
||||
|
||||
## Context
|
||||
|
||||
PVE accepts VMIDs in the range 100–999999999. Without a `ValidateRange`, an out-of-range value reached the API and came back as a confusing server-side error rather than a parameter-binding failure the user could act on.
|
||||
|
||||
Separately, the firewall cmdlets operate at cluster, node or VM level, and used a non-nullable `int` defaulting to 0 for the optional VmId. That made "not specified" and "VM 0" the same value, so the cmdlet could not tell which scope the caller meant.
|
||||
|
||||
## Decision
|
||||
|
||||
- `[ValidateRange(100, 999999999)]` on every VmId parameter, mandatory or optional.
|
||||
- `int?` when the parameter is optional, so absence is representable.
|
||||
- `int` only when VmId is mandatory.
|
||||
|
||||
```csharp
|
||||
[Parameter(Mandatory = true)]
|
||||
[ValidateRange(100, 999999999)]
|
||||
public int VmId { get; set; }
|
||||
|
||||
[Parameter()]
|
||||
[ValidateRange(100, 999999999)]
|
||||
public int? VmId { get; set; }
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
A non-nullable `int` for optional VmId, using 0 as the sentinel for "not supplied". Rejected because 0 is indistinguishable from a supplied value, and because it silently defeats `ValidateRange` — the default sits outside the valid range and never trips it.
|
||||
|
||||
## Consequences
|
||||
|
||||
The range check is on the parameter, not in the service, so a service called directly from another service is not covered by it.
|
||||
|
||||
`Get-PveTaskList` was found later still missing the attribute on its optional `int?` VmId, which is the failure mode this rule invites: adding `int?` is the visible half and it is easy to stop there.
|
||||
@@ -0,0 +1,30 @@
|
||||
# ADR 0011 — Verb class constants required for cmdlet attributes
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-21
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-21
|
||||
- **Context source:** `docs/review/findings.json` F009
|
||||
|
||||
## Context
|
||||
|
||||
`Reset-PveVm` declared `[Cmdlet("Reset", ...)]` with a string literal while every other cmdlet used the verb constants. "Reset" is an approved verb, so nothing was broken — but a typo in that position produces a cmdlet with an unapproved verb, which surfaces only as a module-load warning.
|
||||
|
||||
## Decision
|
||||
|
||||
Every `[Cmdlet]` attribute names its verb through the verb classes — `VerbsCommon`, `VerbsLifecycle` and the rest — never as a string literal.
|
||||
|
||||
```csharp
|
||||
[Cmdlet(VerbsCommon.Reset, "PveVm")]
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
The string literal. It compiles, reads identically, and moves verb validation from the compiler to a runtime warning nobody reads:
|
||||
|
||||
```csharp
|
||||
[Cmdlet("Reset", "PveVm")]
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
The noun half is still a string literal and gets no such protection; the `Pve` prefix convention is enforced by review, not by the compiler.
|
||||
@@ -0,0 +1,22 @@
|
||||
# ADR 0012 — Magic strings are extracted to named constants
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-22
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-22
|
||||
- **Context source:** `docs/review/findings.json` F049
|
||||
|
||||
## Context
|
||||
|
||||
The auth header names `PVEAPIToken=` and `CSRFPreventionToken` appeared as inline literals at several call sites. A typo in one of them fails at runtime as an authentication error, which is a long way from the cause.
|
||||
|
||||
## Decision
|
||||
|
||||
String literals used in more than one place — auth header names, token prefixes and the like — are `const string` fields with names, such as `ApiTokenPrefix` and `CsrfHeaderName`.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
None recorded. This is a maintainability convention adopted during review scan 2026-03-22, not a choice between competing designs.
|
||||
|
||||
## Consequences
|
||||
|
||||
The rule is about repeated literals with protocol meaning. It is not an instruction to hoist every string in the module into a constants class, and it has no mechanical test — a new inline literal is caught by review or not at all.
|
||||
@@ -0,0 +1,47 @@
|
||||
# ADR 0013 — Cmdlets must emit only native or module-defined types
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-03-25
|
||||
- **Deciders:** unrecorded; adopted during review scan 2026-03-25
|
||||
- **Context source:** `docs/review/findings.json` F085
|
||||
|
||||
## Context
|
||||
|
||||
PowerShell enumerates a Newtonsoft `JArray` in ways the user does not expect, and `JObject` properties are invisible to `Get-Member` and to tab completion. Piping module output into `Format-Table`, `Select-Object` or `Where-Object` therefore behaved differently depending on whether the underlying value happened to be a Newtonsoft container — a distinction the user has no way to see.
|
||||
|
||||
Native dictionaries and lists work naturally in the pipeline, so the fix is to stop the third-party types at the module boundary.
|
||||
|
||||
## Decision
|
||||
|
||||
Cmdlet output types and public model properties are native .NET types (`string`, `int`, `bool`, `Dictionary<string, object?>`, `List<T>`, `PSObject`, `void`) or types the module defines itself. No `JObject`, `JArray` or `JToken` on the public surface.
|
||||
|
||||
```csharp
|
||||
public Dictionary<string, object?> GetNodeConfig(...) { ... }
|
||||
|
||||
[JsonProperty("members")]
|
||||
[JsonConverter(typeof(NativeListConverter))]
|
||||
public List<Dictionary<string, object?>>? Members { get; set; }
|
||||
|
||||
[OutputType(typeof(Dictionary<string, object>))]
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Returning the parsed Newtonsoft object directly. It is the shortest path from response to output and it pushes the enumeration problem onto every user:
|
||||
|
||||
```csharp
|
||||
public JObject GetNodeConfig(...) { ... }
|
||||
|
||||
[JsonProperty("members")]
|
||||
public JArray? Members { get; set; }
|
||||
|
||||
[OutputType(typeof(JObject))]
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
The restriction is on the **public** surface. `JObject`, `JArray` and `JToken` are still used freely inside services and cmdlets for response parsing, and that is intended — a review that flags internal parsing use is reading this rule too broadly.
|
||||
|
||||
Conversion has to happen somewhere: models that deserialise a nested structure need a converter (`NativeListConverter`, `JsonHelper.ToNative`) rather than the default binding. A `[JsonExtensionData]` catch-all must land in a private field and be exposed as a native dictionary, or it reintroduces `JToken` through the back door.
|
||||
|
||||
The rule is most at risk on a large nested response, where a typed model is most work and passing the parsed object through is most tempting. If Ceph coverage is added ([ADR 0021](0021-integration-tests-prove-server-semantics-payloads-are-proven-offline.md)), `GET /nodes/{node}/ceph/status` returns raw `ceph status` output and is the shape most likely to leak one; the module has no Ceph surface today.
|
||||
@@ -0,0 +1,46 @@
|
||||
# ADR 0014 — New-PveCluster -Wait blocks until the cluster is quorate
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** integration run 172, 2026-09-01. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
PVE's cluster-create task returns before corosync converges. Until the node is quorate it rejects a join with `cluster not ready - no quorum?`, so the natural sequence `New-PveCluster -Wait` then `Add-PveClusterMember` failed intermittently for every caller.
|
||||
|
||||
Observed on node A in integration run 172: the create task returned, corosync started about a second later, and `node has quorum` appeared about six seconds after that. The integration test had guarded this with `Start-Sleep -Seconds 5` — a fixed sleep against a longer, variable convergence — which is why the cluster tests had never passed.
|
||||
|
||||
## Decision
|
||||
|
||||
`New-PveCluster -Wait` returns only once the cluster reports quorum, not when the create task completes. `ClusterConfigService.WaitForQuorum` polls `GET /cluster/status` for the `cluster` entry with `quorate = 1`, tolerating transient API errors while corosync and pmxcfs restart.
|
||||
|
||||
The wait is bounded and throws `TimeoutException` on expiry, following the `-Wait` timeout convention already used by `Stop-PveContainer`, `Reset-PveVm` and `New-PveBackup`: `[ValidateRange(1, 3600)] public int Timeout`, default 60, **no `0 = infinite`**. A single-node cluster reaches quorum in seconds, so a node still not quorate after 60 s is broken rather than slow.
|
||||
|
||||
`-Wait` on every other cmdlet still means "wait for the task". Cluster creation is the exception because the task completing does not make the cluster usable.
|
||||
|
||||
```powershell
|
||||
New-PveCluster -ClusterName 'c1' -Wait
|
||||
Add-PveClusterMember ...
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
A fixed sleep between create and join, which is what the integration test did:
|
||||
|
||||
```powershell
|
||||
New-PveCluster -ClusterName 'c1' -Wait
|
||||
Start-Sleep -Seconds 5
|
||||
Add-PveClusterMember ...
|
||||
```
|
||||
|
||||
It is wrong in both directions — too short for a slow convergence, wasted time on a fast one — and it puts the workaround in every caller instead of in the cmdlet.
|
||||
|
||||
## Consequences
|
||||
|
||||
There are now two distinct timeout conventions in the module and they must not be mixed:
|
||||
|
||||
- **`-Wait` waits** — `Timeout`, `int` with a default, range 1–3600, no infinite. Task and state waits.
|
||||
- **HTTP client timeouts** — `TimeoutSeconds`, `int?`, range 0–`int.MaxValue`, `0 = infinite`. `Connect-PveServer`, `Send-PveFile`, `Invoke-PveStorageDownload`, which set `HttpClient.Timeout`.
|
||||
|
||||
This is the first instance of a general problem: a PVE task completing does not mean the resource is ready for the next operation. [ADR 0015](0015-lifecycle-wait-blocks-until-the-guest-config-lock-clears.md) is the guest-lock instance of the same thing.
|
||||
@@ -0,0 +1,50 @@
|
||||
# ADR 0015 — Lifecycle -Wait blocks until the guest config lock clears
|
||||
|
||||
- **Status:** Accepted, rescoped 2026-09-01 — covers the config lock only. The flock this was written for is [ADR 0016](0016-restart-pvevm-uses-pve-s-native-reboot-endpoint.md) and [ADR 0020](0020-the-qemu-server-flock-is-retried-never-predicted.md).
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** integration runs 183/184, 2026-09-01; rescoped for issue #113. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
**This entry was written for a failure it does not prevent.** Two different things in PVE are called "lock":
|
||||
|
||||
- the **config lock** — the `lock:` property (`migrate`, `backup`, `clone`, `snapshot`), a persisted config field exposed as `lock` in `status/current`;
|
||||
- the **flock** on `/var/lock/qemu-server/lock-<vmid>.conf` taken by `PVE::QemuConfig->lock_config`, which is not exposed through the API at all.
|
||||
|
||||
The integration failures were the flock. This decision guards the config lock, which an ordinary start or stop never sets. Run 186 confirmed the check never fired: `Restart-PveVm` took 4.11 s, unchanged. The guard below is correct for the config lock and stays, but must never be described as covering the flock.
|
||||
|
||||
The original observation still stands as motivation. Integration run 183 failed four tests from one cause: `Restart-PveVm -Wait` returned after 4.1 s having observed `running`, and the following `Stop-PveVm` spent exactly 10.0 s failing to acquire the lock, cascading into the template convert, clone and remove tests. Run 184 — the same commit, re-run — passed, because its status poll happened to take 10.1 s by which point the lock had cleared. The same settling happens either way; the only variable is whether the wait absorbs it or the next caller does.
|
||||
|
||||
## Decision
|
||||
|
||||
`WaitForStatusTransition` returns only when the guest reports the expected status **and** its config lock has cleared.
|
||||
|
||||
`lock` is read from the `status/current` response the poll already fetches. It is present on both `qemu` and `lxc` and has been since PVE 5.4, well below this module's 7.0 floor, so this costs no extra request.
|
||||
|
||||
If the guest still reports the expected status on the final poll but the lock outlasts `-Timeout`, the cmdlet returns success rather than throwing: the waited-for operation did complete, and only the settling ran long. This keeps a call that succeeded before the change from becoming an exception after it. That fallback tests the **most recent** observation, not "matched at some point during the wait" — a guest that reached the expected status and then drifted away has not satisfied the wait and still raises `PveTaskTimeoutException`. A poll that fails outright leaves the previous observation standing, so a single API blip is not read as divergence.
|
||||
|
||||
```csharp
|
||||
var snapshot = GuestStatusSnapshot.Evaluate(json, expectedStatus);
|
||||
if (snapshot.StatusMatched && !snapshot.Locked)
|
||||
return task;
|
||||
```
|
||||
|
||||
The check lives in `WaitForStatusTransition` rather than in each cmdlet because all nine lifecycle call sites route through it.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Treating the status transition alone as "ready for the next operation":
|
||||
|
||||
```csharp
|
||||
if (string.Equals(effectiveStatus, expectedStatus, StringComparison.OrdinalIgnoreCase))
|
||||
return task;
|
||||
```
|
||||
|
||||
Also rejected, and this is the important one: **attempting to detect the flock before acting**. `snapshot.Locked` reads the config `lock:` property and says nothing about the flock. There is no API surface that does. See [ADR 0020](0020-the-qemu-server-flock-is-retried-never-predicted.md).
|
||||
|
||||
## Consequences
|
||||
|
||||
Same family as [ADR 0014](0014-new-pvecluster-wait-blocks-until-the-cluster-is-quorate.md): a PVE task completing does not mean the resource is ready for the next operation. 0014 is the cluster-quorum instance, this is the guest config-lock instance.
|
||||
|
||||
The flock race that motivated this entry was left unfixed by it, and needed two further decisions: serialise server-side where an endpoint exists (0016), and retry where none does (0020).
|
||||
@@ -0,0 +1,59 @@
|
||||
# ADR 0016 — Restart-PveVm uses PVE's native reboot endpoint
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** integration runs 183/185/186, root-caused on a live PVE 9.2.2 node 2026-09-01. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
Composing a restart client-side races Proxmox's own post-stop cleanup for the guest's config flock:
|
||||
|
||||
1. The shutdown completes and the QEMU process exits.
|
||||
2. `qmeventd` forks `/usr/sbin/qm cleanup <vmid> ...`.
|
||||
3. The client sees `status == stopped` and immediately posts `status/start`. `vm_start` takes the flock, wins the race, starts a **new** QEMU, releases.
|
||||
4. `qm cleanup` then takes the flock with a **60 s** timeout and polls `vm_running_locally` for up to **30 s**, holding it the whole time, because it sees the new PID as the old one failing to exit. PVE's own warning names this: `QEMU process $pid for VM $vmid still running (or newly started)`.
|
||||
5. Every subsequent call fails: `lock_config` defaults to **10 s**, so the client gets `can't lock file '/var/lock/qemu-server/lock-<vmid>.conf' - got timeout`.
|
||||
|
||||
Measured on a reproduction (integration run 187), three distinct source constants matching:
|
||||
|
||||
```
|
||||
qmstart ends t+3 <- qm cleanup takes the flock, sees the NEW pid
|
||||
qmstop #1 FAIL t+14 10 s = lock_config default
|
||||
qmstop #2 FAIL t+24 10 s = lock_config default
|
||||
qmclone FAIL t+25 1 s = qmclone's separate source-VM lock timeout
|
||||
qmstop #3 OK t+33 <- released; hold was t+3..t+33 = 30 s = cleanup's wait loop
|
||||
```
|
||||
|
||||
This surfaced on PVE 9.2 and not 9.1 because of two May 2026 qemu-server changes: cleanup deduplication, shipped for 9.1.13, and the 30 s cleanup wait. Neither touches the REST surface, so the API changelog showed nothing. **"The API did not change, therefore behaviour did not" is not a valid inference for this class of bug.**
|
||||
|
||||
## Decision
|
||||
|
||||
`Restart-PveVm` calls `POST /nodes/{node}/qemu/{vmid}/status/reboot` (`VmService.RebootVm`).
|
||||
|
||||
`vm_reboot` holds the config lock across the entire shutdown and lets `qm cleanup` perform the restart while it already holds that same lock, so there is no window for a client call to interleave.
|
||||
|
||||
```csharp
|
||||
PveTask Issue() => vmService.RebootVm(session, node, vmid, timeout);
|
||||
|
||||
var task = Wait.IsPresent
|
||||
? WaitForStatusTransition(session, node, Issue, vmid, "running", timeout)
|
||||
: Issue();
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Composing the restart from two client calls — `status/shutdown` then `status/start`. This is what the cmdlet did, and it is what races `qmeventd`'s cleanup:
|
||||
|
||||
```csharp
|
||||
WaitForStatusTransition(session, node, () => vmService.ShutdownVm(session, node, vmid, timeout),
|
||||
vmid, "stopped", timeout);
|
||||
WaitForStatusTransition(session, node, () => vmService.StartVm(session, node, vmid),
|
||||
vmid, "running", timeout);
|
||||
```
|
||||
|
||||
## Consequences
|
||||
|
||||
**Containers are not affected.** `/nodes/{node}/lxc/{vmid}/status/reboot` does not exist, so `Restart-PveContainer` necessarily keeps shutdown + start and keeps the exposure.
|
||||
|
||||
This removes the race only where PVE offers a server-side serialised endpoint. `Set-PveVmConfig`, `Resize-PveVmDisk` and clone have none, which is what [ADR 0020](0020-the-qemu-server-flock-is-retried-never-predicted.md) exists to handle.
|
||||
+58
@@ -0,0 +1,58 @@
|
||||
# ADR 0017 — CI runs two lanes: a pinned gating lane and a report-only currency lane
|
||||
|
||||
- **Status:** Accepted. Extended by [ADR 0022](0022-the-gating-lane-pins-its-own-test-tooling-by-exact-version.md), which applies the same pin to the lane's own test tooling.
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** integration runs 176–180, root-caused 2026-09-01. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
`apt-get upgrade` holds back packages that need new dependencies. On the nested nodes that produced `pve-cluster` 9.1.6 against `libpve-cluster-api-perl` 9.1.0 — a combination no real install ever has — and its symptom was not a package error. The node's pmxcfs came back in local mode after a cluster join, `/etc/pve/corosync.conf` never appeared, and the node reported `online=0` while corosync itself had healthy 2-node membership. Three CI runs went into diagnosing that, and removing the upgrade was the entire fix: run 180 was the first fully green integration run.
|
||||
|
||||
So the pin is what makes the gating lane trustworthy. But a permanently pinned CI never exercises the module against a current PVE, and that gap is exactly where an upstream regression would hide.
|
||||
|
||||
## Decision
|
||||
|
||||
CI provisions nested PVE nodes in two distinct modes, and they are not merged into one:
|
||||
|
||||
- **Lane 1, `integration-tests.yml`** — nodes stay pinned to what the ISO ships. `first-boot.sh` never runs `apt-get upgrade` or `dist-upgrade`. This lane gates merges.
|
||||
- **Lane 2, `package-currency.yml`** — nodes are `dist-upgrade`d to current PVE and the suite runs against them. **Report-only**: test failures do not fail the job.
|
||||
|
||||
Both declare `concurrency: group: integration-tests`. They drive the same nested VMIDs on the same parent node, so they must never run at once.
|
||||
|
||||
```bash
|
||||
# first-boot.sh installs only what provisioning needs; the ISO is the pin
|
||||
apt-get update -qq
|
||||
apt-get install -y -qq --no-install-recommends qemu-guest-agent open-iscsi
|
||||
```
|
||||
|
||||
```yaml
|
||||
# package-currency.yml opts in explicitly; lane 1 never sets this
|
||||
env:
|
||||
PVE_DIST_UPGRADE: '1'
|
||||
```
|
||||
|
||||
Report-only is deliberate. A scheduled job that goes red on an upstream change nobody has chosen to chase becomes noise, and a noisy cron gets ignored — the failure mode that makes a canary worthless. The signal is the rolling issue and the recorded package set, not the check colour.
|
||||
|
||||
A failure of the lane's own machinery — provisioning, the upgrade, the reboot, an unreachable node — still fails the job. `run-integration.sh` returns 3 for a genuine test failure and 4 when it cannot reach or authenticate to a node; only 3 is suppressed. Suppressing both would let a botched reboot report success while the lane learned nothing.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Upgrading packages in the gating lane's `first-boot.sh`, so one lane covers both currency and gating:
|
||||
|
||||
```bash
|
||||
apt-get update -qq
|
||||
apt-get -y upgrade
|
||||
```
|
||||
|
||||
This is the mismatch that left a node unclustered and cost three CI runs to diagnose. `upgrade` rather than `dist-upgrade` is what produces the impossible combination, but the deeper problem is that a moving input cannot sit in the merge gate at all.
|
||||
|
||||
Also rejected: making lane 2 fail the build. See the consequence below.
|
||||
|
||||
## Consequences
|
||||
|
||||
**Accepted risk:** a module genuinely broken against current PVE shows a green weekly check plus an updated issue. Operator ruling 2026-09-01, to be revisited after a few releases.
|
||||
|
||||
Any moving input to the gating lane is the same defect in a different place, which is what [ADR 0022](0022-the-gating-lane-pins-its-own-test-tooling-by-exact-version.md) addresses for the test tooling.
|
||||
|
||||
A node-versus-node package comparison is reported even when the set is otherwise unchanged, because a mismatch *between* the two nested nodes is the failure that cost those three runs.
|
||||
+54
@@ -0,0 +1,54 @@
|
||||
# ADR 0018 — The currency lane reboots after dist-upgrade, and proves it rebooted
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** pre-push review of PR #106, 2026-09-01. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
A PVE `dist-upgrade` pulls `proxmox-kernel-*`. Without a reboot the node runs new userspace on the old kernel, so the currency lane records a package set it never actually ran and is blind to kernel regressions — it would report "current PVE" while testing something that never booted.
|
||||
|
||||
The verification half is the part that is easy to omit, and it was omitted in the first draft. `ssh … reboot` returns non-zero when the connection dies, so it needs `|| true` — which swallows *every* ssh failure, including the reboot never being issued. `wait-for-api.sh` then matches the **still-running pre-reboot** pveproxy on its first poll and returns `responsive after 0s`. The script exits 0 having proved nothing. A blind `sleep` before polling does not fix this; it is wrong in both directions and verifies nothing either way.
|
||||
|
||||
## Decision
|
||||
|
||||
After `dist-upgrade`, `prepare-test-environment.sh` reboots the node **unconditionally** and then **verifies the reboot happened** by comparing `/proc/sys/kernel/random/boot_id` before and after. An unchanged boot id is fatal.
|
||||
|
||||
```bash
|
||||
boot_before="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id")"
|
||||
${SSH_CMD} "systemctl reboot" || true
|
||||
boot_after=""
|
||||
for _ in $(seq 1 60); do
|
||||
boot_after="$(${SSH_CMD} "cat /proc/sys/kernel/random/boot_id" 2>/dev/null || true)"
|
||||
[[ -n "${boot_after}" && "${boot_after}" != "${boot_before}" ]] && break
|
||||
sleep 5
|
||||
done
|
||||
if [[ -z "${boot_after}" || "${boot_after}" == "${boot_before}" ]]; then
|
||||
echo "ERROR: ${NESTED_IP} did not reboot (boot_id unchanged)" >&2
|
||||
exit 1
|
||||
fi
|
||||
bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
|
||||
```
|
||||
|
||||
Order matters: prove the boot id changed first (ssh returns before pveproxy does), then wait for the API, then wait for pmxcfs. `pvesm set` writes `/etc/pve/storage.cfg`, which needs `/etc/pve` mounted, and on a fresh boot that lags the API by seconds.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Reboot and sleep, without proving anything:
|
||||
|
||||
```bash
|
||||
${SSH_CMD} "systemctl reboot" || true
|
||||
sleep 30
|
||||
bash "${SCRIPT_DIR}/wait-for-api.sh" "${NESTED_IP}" 8006 600
|
||||
```
|
||||
|
||||
Gating the reboot on `/var/run/reboot-required`. That file comes from `update-notifier-common`, which is not guaranteed present on a PVE node, so the gate silently never fires.
|
||||
|
||||
Putting the reboot in `first-boot.sh`. That runs `ordering = "fully-up"` while the parent is still polling, so `wait-for-pve.sh` can discover the IP, see the API, pass auth, and then have the node reboot out from under provisioning — presenting as an intermittent network fault.
|
||||
|
||||
## Consequences
|
||||
|
||||
The lane costs one reboot per node on every currency run, plus three bounded waits: up to 300 s proving the boot id changed (60 polls at 5 s), then `wait-for-api.sh` at 600 s, then up to 150 s for pmxcfs. The worst case is therefore near 17 minutes per node, though a healthy node clears it in a fraction of that. That is the price of the recorded package set being the one actually tested.
|
||||
|
||||
This is the general shape of the [ADR 0017](0017-ci-runs-two-lanes-a-pinned-gating-lane-and-a-report-only-currency-lane.md) machinery rule: a failure of the lane's own plumbing fails the job, even though test failures in that lane do not.
|
||||
+32
@@ -0,0 +1,32 @@
|
||||
# ADR 0019 — Local dev calls run-integration.sh directly; there is no wrapper script
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** audit of the local dev path against the post-ARC CI, 2026-09-01. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
`tests/dev.ps1` was a 291-line PowerShell wrapper over roughly six `docker compose` and `docker exec` calls. Every capability it had was already available elsewhere: build and unit tests are plain `dotnet` and `Invoke-Pester` invocations, and the module build it performed is duplicated inside `run-integration.sh`, which publishes and installs the module before running the suite.
|
||||
|
||||
Being a second entry point, it drifted from the script it wrapped and from the CI it claimed to replicate. By the time it was removed it still offered a `-Version 8` leg retired in #88, mounted the Docker socket for storage containers replaced by the storage VM in #87, and defaulted its remote-host examples to a runner decommissioned in the ARC migration.
|
||||
|
||||
Four documentation files described a positional calling convention (`./tests/dev.ps1 test`) that did not do what it read as. The script took its actions from switches (`-Test`), but also declared `[string[]] $Tests`, so the bare word bound to `-Tests` — the integration-area filter. With no action switch set, the script fell through to its `-Shell` default and silently opened an interactive container shell. Every documented command was wrong, and wrong in the quietest possible way: it succeeded at something nobody asked for.
|
||||
|
||||
## Decision
|
||||
|
||||
`tests/infrastructure/scripts/run-integration.sh` is the only entry point to the provision → test → cleanup lifecycle, for CI and for local development alike. Local runs invoke it inside the `dev-infra` container — the same image CI runs its jobs in.
|
||||
|
||||
Build and unit tests need no container at all; they run natively against the solution.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
A `dev.ps1`, a `Makefile` target, or a shell function that re-implements provisioning steps, module installation or test invocation.
|
||||
|
||||
A wrapper that must be kept in sync with the thing it wraps earns its place only when it removes real friction. This one removed none, and its drift was invisible because a wrong invocation still exited zero.
|
||||
|
||||
## Consequences
|
||||
|
||||
If a local flow is awkward, the fix goes in `run-integration.sh` so CI gets it too.
|
||||
|
||||
Local runs on Apple Silicon pay for this in emulation: the image is amd64-only, and under Rosetta the suite runs roughly 40% slower. That is a documented consequence of using the CI image rather than a local shortcut — and it turned out to be load-bearing, because it is the client speed that reproduced the flock race in [ADR 0020](0020-the-qemu-server-flock-is-retried-never-predicted.md) that CI never showed.
|
||||
@@ -0,0 +1,59 @@
|
||||
# ADR 0020 — The qemu-server flock is retried, never predicted
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** issue #113, reproduced 2026-09-01 on a Rosetta-emulated client. No finding ID.
|
||||
|
||||
## Context
|
||||
|
||||
[ADR 0015](0015-lifecycle-wait-blocks-until-the-guest-config-lock-clears.md) tried to predict the lock and guarded the wrong one. [ADR 0016](0016-restart-pvevm-uses-pve-s-native-reboot-endpoint.md) removed the race for `Restart-PveVm` by handing the ordering to PVE, but that only works where a server-side serialised endpoint exists. `Set-PveVmConfig`, `Resize-PveVmDisk` and clone have none, so for them the choice is retry or nothing.
|
||||
|
||||
CI never showed this. Runs 189–200 were green because the CI client is fast enough to win the race. It reproduces on a client roughly 40% slower — the CI container image run under Docker Desktop's Rosetta emulation on Apple Silicon — which failed `Should hard-reset a running VM`, `Should clone a VM` and `Should resize a VM disk (Resize-PveVmDisk)` on the same commit CI passed. **A green CI run is not evidence about this class of bug.**
|
||||
|
||||
## Decision
|
||||
|
||||
An operation PVE rejects with `can't lock file '<guest lock path>' - got timeout` is reissued for a bounded window (`GuestLockRetry.DefaultWindow`, 45 s).
|
||||
|
||||
Two seams implement it, and both are required:
|
||||
|
||||
- **`PveHttpClient.SendAsync`** — retries the request itself. This covers every operation PVE serialises inside the API handler, where the failure arrives as a 500: `Set-PveVmConfig`, `Resize-PveVmDisk`'s config writes, and every future call through the client. The private send takes a `Func<HttpRequestMessage>` rather than a request, because an `HttpRequestMessage` cannot be sent twice.
|
||||
- **`PveCmdletBase.InvokeGuestTask`** — reissues the API call *and* re-waits its task. PVE takes the flock inside the forked worker for most guest operations (`qmreset`, `qmclone`), so the POST returns 200 with a UPID and the failure appears only in the task's exit status. The HTTP layer cannot see it and cannot retry it. `WaitForStatusTransition` routes through this helper, which is why it takes a `Func<PveTask>` rather than an already-issued `PveTask`.
|
||||
|
||||
```csharp
|
||||
PveTask Issue() => vmService.CloneVm(session, sourceNode, vmid, newid, name, targetNode, full);
|
||||
|
||||
var task = Wait.IsPresent
|
||||
? InvokeGuestTask(session, sourceNode, Issue)
|
||||
: Issue();
|
||||
```
|
||||
|
||||
Reissuing is safe **only** for a failure to *enter* `lock_config`, which PVE raises before the operation does any work. `GuestLockRetry.IsLockTimeout` must keep both properties that establish this, and no failure may be added to it without them:
|
||||
|
||||
- **Path-specific.** `PVE::Tools::lock_file` emits identical wording for storage, LVM, HA, backup and firewall locks. Those are taken mid-worker and carry no such guarantee, so the match names the two guest config paths (`/var/lock/qemu-server/lock-<vmid>.conf`, `/run/lock/lxc/pve-config-<vmid>.lock`) rather than the generic phrasing.
|
||||
- **Anchored at the start of what PVE said.** `qmclone` is the operation that makes this matter: its worker creates and locks the target config, allocates disks, then re-locks. A timeout at one of those later points reads the same as one at entry, and reissuing it would hit `check_vmid_unused` — "VM `<newid>` already exists" — leaving an orphaned guest behind. PVE prefixes the late form with its own context (`clone failed: ...`), so anchoring rejects it. `Resize-PveVmDisk -Size '+1G'` is the case where getting this wrong is irreversible rather than merely messy.
|
||||
|
||||
The anchor only works against the raw text, so the predicate reads `PveTaskFailedException.ExitStatus` and `PveApiException.ApiMessage` — never `Exception.Message`, which both types prefix with their own context. `ApiMessage` exists for this.
|
||||
|
||||
The window is 45 s because `qm cleanup` holds the flock while polling `vm_running_locally` for up to 30 s, and each rejected attempt first burns PVE's own 10 s `lock_config` timeout.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Observing the flock before acting. PVE does not expose it in `status/current` or anywhere else, so there is nothing to observe:
|
||||
|
||||
```csharp
|
||||
if (!snapshot.Locked)
|
||||
return task; // reads the config `lock:` property; says nothing about the flock
|
||||
```
|
||||
|
||||
Binding the retry to the cmdlet's `-Timeout`. See the consequence below — it defeats the fix at exactly the values that need it.
|
||||
|
||||
Threading a shared retry budget through both seams. See below: the overlap costs a longer wait before the same failure, never a different outcome.
|
||||
|
||||
## Consequences
|
||||
|
||||
- **`-Timeout` does not bound the retry.** It is documented as the budget for the status transition, and `WaitForStatusTransition` starts counting only after the operation's task completes. `Reset-PveVm -Wait -Timeout 30` needed about 31 s of retrying in the run that verified this change.
|
||||
- **The two seams nest.** A cmdlet operation rejected synchronously burns the HTTP layer's window inside `InvokeGuestTask`'s.
|
||||
- The window bounds when a *new* attempt may start, not total wall clock: an attempt beginning just inside the window still runs to its own conclusion, so the real ceiling is roughly one attempt longer.
|
||||
|
||||
**Not yet adopted.** `InvokeGuestTask` is the correct seam for every cmdlet that issues a guest operation and waits on its task. `Remove-PveVm`, `Move-PveVm`, the snapshot and template cmdlets, and the container equivalents still call `TaskService.WaitForTask` directly and remain exposed to the same race. They adopt the helper as they are next touched.
|
||||
+54
@@ -0,0 +1,54 @@
|
||||
# ADR 0021 — Integration tests prove server semantics; payloads are proven offline
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-02
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** issue #120 (coverage); #92/#118 (the case that motivated it)
|
||||
|
||||
## Context
|
||||
|
||||
`Set-PveNetwork` sent `bridge_vlan_aware=0` to clear a VLAN-aware bridge. The API schema advertises a plain optional boolean, so the request is valid and returns success — and PVE merges supplied keys onto the stored stanza and ignores the `0`. The flag never cleared. Only `delete=bridge_vlan_aware` works, and only a run against a real PVE 9 revealed it.
|
||||
|
||||
That is what a live cluster is for: server behaviour the schema misdescribes. It is not for checking that a dictionary has the right keys, which a mock proves in milliseconds.
|
||||
|
||||
The distinction decides whether the suite scales. Coverage is about 36% of 678 endpoints; the remaining surface is large enough that "every new endpoint gets a live test" puts the integration suite on a growth curve the CI budget cannot absorb. Growth is linear in *live-only* surface, and how much surface is live-only is a design choice, not a given.
|
||||
|
||||
## Decision
|
||||
|
||||
An integration test must earn its place by testing something only a live PVE can answer. Request-payload correctness — which keys a cmdlet sends, and with what values — is verified offline against the mock `IPveHttpClient` harness.
|
||||
|
||||
Concretely:
|
||||
|
||||
- New cmdlets route through a `*Service` that accepts `IPveHttpClient`, so their payload is reachable from `PSProxmoxVE.Core.Tests` without a cluster.
|
||||
- The 37 cmdlets that construct `PveHttpClient` directly are converted to that seam before the next large coverage push, and opportunistically when otherwise touched. Measured against 194 concrete cmdlet files (`src/PSProxmoxVE/Cmdlets/**/*.cs` less the `PveCmdletBase` base class): 155 reach the API only through a `*Service`, 25 only through their own client, 12 do both, and 2 do neither. The service and direct-client sets overlap, so they do not sum to 194.
|
||||
- The integration suite is tiered: a PR exercises smoke plus the areas its diff touches (`run-integration.sh test <ver> <Area>` already supports this); the full suite runs on merge to `main`.
|
||||
- Areas whose dependencies cannot exist in CI (ACME needs a CA plus DNS or HTTP reachability) are covered by mock/contract tests asserting request shape, not by a live lane.
|
||||
- Areas needing a differently-shaped cluster (Ceph needs dedicated block devices per node and wants three monitors) live behind an opt-in provisioning profile, so ordinary runs do not pay for them.
|
||||
|
||||
The target shape, which is not yet how the tree reads — `Set-PveNetwork` is one of the 37, and `NetworkService.SetNetwork` has no callers anywhere in the repository:
|
||||
|
||||
```csharp
|
||||
var service = new NetworkService();
|
||||
service.SetNetwork(session, Node, Iface, config);
|
||||
```
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
A cmdlet that builds its own form and owns its own client. Every field it emits is then verifiable only by provisioning a cluster:
|
||||
|
||||
```csharp
|
||||
using var client = new PveHttpClient(session);
|
||||
var data = new Dictionary<string, string> { ["type"] = Type };
|
||||
if (!string.IsNullOrEmpty(Address)) data["address"] = Address!;
|
||||
client.PutAsync($"nodes/{node}/network/{iface}", data).GetAwaiter().GetResult();
|
||||
```
|
||||
|
||||
Also rejected: a live integration test for every newly covered endpoint. It is the status quo and the reason the question arose — Ceph and certificates alone would add about 66.
|
||||
|
||||
## Consequences
|
||||
|
||||
The seam conversion (#126) is a precondition for the Ceph (#128) and certificates (#129) work, not a parallel task. Sequencing agreed with the operator 2026-09-02: quick wins (#121–#124), then the seam conversion, then Ceph and certificates. Suite tiering is #127.
|
||||
|
||||
`PSProxmoxVE.Core.Tests` references only `PSProxmoxVE.Core`, not the cmdlet assembly, so a payload that stays in the cmdlet has no offline path even in principle.
|
||||
|
||||
A tiered PR run is not full validation; merge to `main` remains the gate.
|
||||
@@ -0,0 +1,34 @@
|
||||
# ADR 0022 — The gating lane pins its own test tooling by exact version
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-01
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** recorded as an amendment to D017 on 2026-09-01; split into its own record during the ADR migration, [ADR 0023](0023-decisions-live-in-docs-decisions-in-house-adr-format.md).
|
||||
|
||||
## Context
|
||||
|
||||
[ADR 0017](0017-ci-runs-two-lanes-a-pinned-gating-lane-and-a-report-only-currency-lane.md) pins the nested PVE packages so the merge gate has no moving inputs. The lane's own tooling was not pinned, which is the same defect one layer up.
|
||||
|
||||
Both Pester install sites used `-MinimumVersion 5.0` with no ceiling. The image is rebuilt on every CI run and Pester is installed fresh on every unit-test run, so PSGallery decided the version: a new major could reach the merge gate with no commit to this repository, surfacing as unexplained test breakage on whichever PR happened to run next.
|
||||
|
||||
It had already happened silently. Steps named "Install Pester 5" were resolving 6.1.0 on both the PowerShell 5.1 and 7.x legs, because Pester 6 declares `PowerShellVersion 5.1` and so installs on Windows PowerShell too. Nothing broke — the suite uses only constructs common to 5 and 6 — but nobody chose it.
|
||||
|
||||
## Decision
|
||||
|
||||
The gating lane's test tooling is pinned by exact version: `Pester` in `tests/Dockerfile.test` (`ARG PESTER_VERSION`) and in `.github/workflows/unit-tests.yml` (`env.PESTER_VERSION`), installed and imported with `-RequiredVersion` at every site, including the suite's own import inside the container.
|
||||
|
||||
The Dockerfile promotes the ARG to `ENV` so the version is discoverable at runtime. Both files must name the same version, and `shell-selfchecks` asserts it.
|
||||
|
||||
Bumping is a deliberate commit that changes both files together.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
`-MinimumVersion 5.0`, or any floor without a ceiling. It reads as a pin and is not one: the resolved version is whatever PSGallery published most recently, so the merge gate changes without a commit.
|
||||
|
||||
Pinning only the Dockerfile. The unit-test workflow installs Pester independently, so the two would drift and the drift would be invisible — which is why `shell-selfchecks` asserts they agree rather than trusting convention.
|
||||
|
||||
## Consequences
|
||||
|
||||
Two files must change together for every bump, and a check exists solely to enforce that.
|
||||
|
||||
The pin is on the version, not on the gallery — a yanked or unavailable version fails the build loudly, which is the intended behaviour for a gate.
|
||||
@@ -0,0 +1,46 @@
|
||||
# ADR 0023 — Decisions live in docs/decisions in house ADR format
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-02
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** reverses the ruling in `docs/lane2-change-plan.md`, made 2026-09-01. That file was deleted the same day by [ADR 0024](0024-the-findings-ledger-is-retired-open-work-lives-in-github-issues.md); it is in git history.
|
||||
|
||||
## Context
|
||||
|
||||
This repository recorded architectural decisions in a single `DECISIONS.md` as D001–D021, in a bespoke format: a `Status` / `Finding refs` / `Resolved in scan` header, then `Decision`, `Rationale`, `Anti-pattern (do not reintroduce)` and `Correct pattern`.
|
||||
|
||||
On 2026-09-01 that was examined and deliberately kept. `docs/lane2-change-plan.md` recorded the reasoning: the repo already had a decision store its own `CLAUDE.md` named as the read-before-coding file, and introducing a parallel `docs/decisions/` tree mid-flight would have been the exact defect the change gate warns about. The decision was to migrate once the CI lane work landed rather than during it.
|
||||
|
||||
That work is complete, so the reason for the deferral is spent.
|
||||
|
||||
A survey of the wider ADR corpus settled the one open format question. Across 22 repositories there are roughly 290 house-format ADRs (independent counts landed between 285 and 294, depending on whether index files are included); fewer than 20 contain a fenced code block at all, and — the load-bearing part, which two independent passes confirmed — **none** uses a contrastive wrong-form/right-form pair. The `Anti-pattern` / `Correct pattern` sections are specific to this repository's genre — per-call-site coding idioms — not a gap in the house template.
|
||||
|
||||
The same survey found that repositories which keep a never-reintroduce checklist keep it in `CLAUDE.md` or `CONTRIBUTING.md` and cite ADRs for rationale, rather than treating the ADR as the checklist. This repository already did half of that: `CLAUDE.md` § "Key Conventions" carried ten of the thirteen convention rules as one-line bullets.
|
||||
|
||||
## Decision
|
||||
|
||||
Architectural decisions live in `docs/decisions/` in the house ADR format, generated by `~/.claude/templates/new-adr.sh`. `DECISIONS.md` is reduced to a stub pointing there, with a D-number to ADR-number redirect table.
|
||||
|
||||
D001–D021 map one-to-one onto ADR 0001–0021. The `Anti-pattern` block becomes `## Rejected alternatives`; the `Correct pattern` block folds into `## Decision`. D017's "Amendment 2026-09-01" was a second decision in one entry and is split into [ADR 0022](0022-the-gating-lane-pins-its-own-test-tooling-by-exact-version.md).
|
||||
|
||||
The convention checklist lives in `CLAUDE.md` § "Key Conventions", completed to cover every per-call-site convention rule. ADR 0009 (framework targeting) has no bullet, because it constrains project files rather than code a cmdlet author writes. The ADRs carry rationale.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
**Adding `## Anti-pattern` and `## Correct pattern` to the global house template.** It would mark every existing ADR in the corpus nonconforming to serve fewer than ten of that genre, and the generator does flat token substitution with no conditionals, so an "optional" section is two headings the author deletes by hand in most cases.
|
||||
|
||||
**Keeping the extra headings in this repository only.** Directly contradicts the ADR skill's instruction that the template is the single source of the format.
|
||||
|
||||
**Keeping `DECISIONS.md`.** The position `docs/lane2-change-plan.md` took, correct while the CI work was in flight and now spent.
|
||||
|
||||
**Deleting `DECISIONS.md` outright.** Four `CHANGELOG.md` entries and several issue bodies cite it by name and D-number; a stub degrades those to a redirect instead of a dead reference.
|
||||
|
||||
## Consequences
|
||||
|
||||
`docs/lane2-change-plan.md` was amended to point here so the repository would not carry two contradictory rulings, and was then deleted outright by [ADR 0024](0024-the-findings-ledger-is-retired-open-work-lives-in-github-issues.md) along with the rest of the superseded planning documents.
|
||||
|
||||
The D-numbers are retired as identifiers. `docs/review/findings.json` `decisions_ref` values were rewritten to ADR numbers so the repository would carry one scheme — 14 of them had to be repointed by topic, because they were already off by one under the old scheme and a literal substitution preserved that. The ledger was then retired entirely by [ADR 0024](0024-the-findings-ledger-is-retired-open-work-lives-in-github-issues.md). The redirect table in the `DECISIONS.md` stub covers released changelog entries and older issue bodies, which are not rewritten.
|
||||
|
||||
`.github/workflows/claude-code-review.yml` still names a hardcoded decision range, `D001-D016`. It had already drifted five entries behind while the file held D021 — a document-as-checklist referenced by number goes stale on every addition, which is part of why the checklist now lives in `CLAUDE.md`. Repointing it at the Key Conventions list lands in a separate pull request, because `claude-code-action`'s anti-tamper gate refuses to review any PR whose copy of that one file differs from the default branch: bundling the change here would have made this migration unreviewable by the bot.
|
||||
|
||||
Future decisions are generated with `new-adr.sh` from the repository root; the next number is 0024.
|
||||
+44
@@ -0,0 +1,44 @@
|
||||
# ADR 0024 — The findings ledger is retired; open work lives in GitHub issues
|
||||
|
||||
- **Status:** Accepted
|
||||
- **Date:** 2026-09-02
|
||||
- **Deciders:** operator + agent
|
||||
- **Context source:** audit of `docs/review/` during the ADR migration, [ADR 0023](0023-decisions-live-in-docs-decisions-in-house-adr-format.md)
|
||||
|
||||
## Context
|
||||
|
||||
`docs/review/` held a structured review system: `findings.json`, a ledger of 91 findings with permanent IDs, resolution evidence and regression history; `REVIEW_REPORT.md`, the last full scan report; and `PLAN-integration-refactor.md`, a planning document. `CLAUDE.md` instructed every session to read the ledger before starting work.
|
||||
|
||||
It had already stopped being used. The last substantive commit to `docs/review/` was 2026-05-22, more than three months before this decision, while the repository kept moving — decisions D014 through D021 were all recorded without a corresponding scan.
|
||||
|
||||
The parity check is what settled it. Of 91 findings, 83 were resolved and one was `wont_fix`. Of the seven still open, **six were already filed as GitHub issues**: F046 as #127 and #120, F054 as #128, F061, F067 and F068 as #120, F069 as #129. Only F021 — no `IconUri` in the manifest, severity low — existed solely in the ledger, and was filed as #130 before this decision took effect.
|
||||
|
||||
So the ledger was not a second source of truth. It was a stale copy of one.
|
||||
|
||||
The two supporting documents were worse than stale. `REVIEW_REPORT.md` is a dated snapshot whose decision-compliance table covered D001–D013 while the repository held D021. `PLAN-integration-refactor.md`, marked "Planned (not started)", proposed parallel provisioning, ISO caching and zero-touch runner setup — all since delivered by the Terraform and ARC work — and still planned around the PVE 8 leg retired in #88.
|
||||
|
||||
## Decision
|
||||
|
||||
`docs/review/` is deleted. `docs/lane2-change-plan.md` is deleted with it, on the same reasoning: its only live content was a ruling now superseded by [ADR 0023](0023-decisions-live-in-docs-decisions-in-house-adr-format.md), and the rest is a change plan for work that shipped.
|
||||
|
||||
Open work is tracked in GitHub issues. Decisions are recorded as ADRs in `docs/decisions/`. Conventions live in `CLAUDE.md` § "Key Conventions". There is no fourth store.
|
||||
|
||||
A planning document that needs to be public becomes an issue, not a file in `docs/`.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
**Keeping `findings.json` as a historical archive.** It would still be listed in `CLAUDE.md` and in `.github/copilot-instructions.md` as a thing to read before coding, so every future session would read a ledger that has been wrong since May. An archive nobody is told to ignore is not an archive.
|
||||
|
||||
**Keeping the ledger and retiring only the two stale documents.** This was the narrower option and it fails the same test: the ledger's remaining value was its seven open findings, six of which were already duplicated in issues. Maintaining both means reconciling them, and nothing had reconciled them for three months.
|
||||
|
||||
**Migrating the 83 resolved findings into issues as closed records.** High volume, no reader. The resolution evidence that mattered was already absorbed into the ADRs during the migration, and git history holds the rest.
|
||||
|
||||
## Consequences
|
||||
|
||||
Fourteen ADRs cite finding IDs in their **Context source** line, such as `docs/review/findings.json F032, F033, F036, F058`. Those citations stay. They record what prompted the decision, which remains true, and this ADR is where a reader learns the ledger was retired deliberately rather than lost. The file is recoverable from git history at any commit before this one.
|
||||
|
||||
`CLAUDE.md` loses its "Review System" and "Finding ID stability" sections. The session checklist no longer directs a reader to a findings file.
|
||||
|
||||
The F-numbers are retired as identifiers, the same way [ADR 0023](0023-decisions-live-in-docs-decisions-in-house-adr-format.md) retired the D-numbers. Unlike the D-numbers there is no redirect table, because there is nothing to redirect to.
|
||||
|
||||
`.github/workflows/package-currency.yml` carried a header comment pointing at the deleted change plan; it now points at [ADR 0017](0017-ci-runs-two-lanes-a-pinned-gating-lane-and-a-report-only-currency-lane.md), which is where that lane's reasoning lives.
|
||||
@@ -1,110 +0,0 @@
|
||||
# Lane 2 — PVE package-currency CI lane: change plan
|
||||
|
||||
Status: **approved by the operator 2026-09-01.** Report-only accepted for now — revisit once a few releases are out.
|
||||
|
||||
## 1. Problem
|
||||
|
||||
Lane 1 pins the nested PVE nodes to what the ISO ships and deliberately does not upgrade. `first-boot.sh` says so in a comment: `apt-get upgrade` held packages back and produced `pve-cluster` 9.1.6 against `libpve-cluster-api-perl` 9.1.0 — a combination no real install ever has, whose cluster join silently left the node unclustered. Removing it was the whole fix for integration run 180.
|
||||
|
||||
That pin buys a stable signal and costs currency: nothing in CI ever exercises the module against a *current* PVE. Lane 2 is the canary that does, without putting that instability back into the lane that gates merges.
|
||||
|
||||
## 2. What is already decided
|
||||
|
||||
From the cluster/CI workstream tracker, and not reopened here:
|
||||
|
||||
- Lane 2 does `dist-upgrade`, records the resulting package set, and is **report-only** — it never auto-bumps lane 1's pin.
|
||||
- Weekly cron plus manual dispatch.
|
||||
- It **must reboot** after the `dist-upgrade`. A PVE `dist-upgrade` pulls `proxmox-kernel-*`; with no reboot the node runs new userspace on the old kernel, so the lane records a package set it never actually ran and is blind to kernel regressions.
|
||||
- The reboot goes in `prepare-test-environment.sh`, **not** `first-boot.sh`. `first-boot.sh` runs `ordering = "fully-up"` while the parent is still polling, so `wait-for-pve.sh` can discover the IP, see the API, pass auth, and then have the node reboot out from under provisioning — intermittent, and looks like a network fault.
|
||||
- Wait for the node to come back with `wait-for-api.sh`, which already exists and is currently unused by `run-integration.sh`.
|
||||
- Reboot **unconditionally**, not gated on `/var/run/reboot-required` — that file comes from `update-notifier-common`, which is not guaranteed on a PVE node.
|
||||
- Failures raise **one** rolling GitHub issue, updated in place, carrying the `dpkg` diff against the last good run — not a new issue per tick.
|
||||
- Share the `integration-tests` concurrency group, or two runs fight over the nested VMIDs.
|
||||
|
||||
## 3. Decisions taken in this gate
|
||||
|
||||
| # | Fork | Choice |
|
||||
|---|---|---|
|
||||
| 1 | Lane home | New `.github/workflows/package-currency.yml` |
|
||||
| 2 | Scope | Full Pester suite; test failures reported, **not** fatal |
|
||||
| 3 | Baseline store | **Superseded during build** — see 3.3. An unprotected `ci/package-baseline` data branch, not a committed file in a PR |
|
||||
|
||||
### 3.1 Why a separate workflow
|
||||
|
||||
`integration-tests.yml` is already the most complex workflow in the repo and its trigger comment explicitly reasons about fork-PR safety. The currency lane needs `issues: write` (to maintain the rolling issue) and `contents: write` + `pull-requests: write` (to raise the baseline PR); `integration-tests.yml` needs none of those. Putting the lane in its own file keeps those permissions off every normal integration run. Both files declare `concurrency: group: integration-tests` so they serialise against each other.
|
||||
|
||||
### 3.2 Why the suite runs but does not fail the job
|
||||
|
||||
Package drift that doesn't break anything is not interesting. The thing worth knowing is *the current PVE breaks the module* — that only surfaces by running the tests. But a scheduled job that goes red on an upstream change we have not chosen to chase becomes noise, and a noisy cron gets ignored, which is the failure mode that makes canaries worthless. So the suite runs, results land in the issue, and the job stays green unless the lane's own machinery fails.
|
||||
|
||||
**Consequence to accept:** a genuinely broken module against current PVE is a green check with an updated issue. The issue is the signal, not the check colour.
|
||||
|
||||
### 3.3 Baseline: superseded during implementation
|
||||
|
||||
The original choice was a committed baseline file updated by an auto-merging PR. **That cannot work on this repo**, and the reason only surfaced in review:
|
||||
|
||||
- A PR opened with `GITHUB_TOKEN` never triggers `pull_request` workflows — GitHub suppresses them to prevent recursion. `build.yml`, `unit-tests.yml` and `claude-code-review.yml` are all `pull_request`-triggered, so the baseline PR would report **zero required checks** and be permanently unmergeable under branch protection. Not "waits for a human to merge it" — cannot be merged.
|
||||
- Auto-merge is enabled on the repo, but auto-merge waits for required checks that will never report, so it does not help.
|
||||
- `pve_api` solves this by pushing straight to `main` as `github-actions[bot]` with `contents: write`. That works because its `main` is unprotected. This repo's is (required checks, required review, admin enforced), so the same action is rejected.
|
||||
- Using a GitHub App installation token would fire the checks — but it puts output from a machine that just ran `dist-upgrade` against an upstream repo in front of the `claude-review` agent, which holds `pull-requests: write` and is instructed to end with `--approve`. That is a known [required-reviews bypass](https://medium.com/cider-sec/bypassing-required-reviews-using-github-actions-6e1b29135cc7) shape, and not worth opening for a generated data file.
|
||||
|
||||
**Adopted instead: an unprotected data branch**, `ci/package-baseline`, holding exactly one file. This is the established pattern for generated data against a protected main — [github-archive-action](https://github.com/githubocto/github-archive-action) writes to an orphan branch for the same reasons. Branch protection covers `main` only, so `contents: write` plus `GITHUB_TOKEN` is sufficient: no PR, no checks, no protection conflict, and no path from node output to the review agent.
|
||||
|
||||
It is written with git plumbing (`hash-object` → `mktree` → `commit-tree` → `push <sha>:refs/heads/…`) so the job's checkout is never touched and no local branch is created, which also makes a re-run unable to collide with itself.
|
||||
|
||||
Artifacts alone were considered — the operator's own suggestion — and rejected on the stated goal: artifacts expire (90 days by default), so a weekly cadence would retain roughly 13 data points and lose the history. `git log ci/package-baseline` keeps it indefinitely.
|
||||
|
||||
## 4. Change plan
|
||||
|
||||
Small, test-pinned commits; each is its own PR.
|
||||
|
||||
### Commit 1 — `prepare-test-environment.sh` learns to upgrade
|
||||
|
||||
Add an opt-in third argument (default off), so lane 1's behaviour is byte-identical when it isn't passed:
|
||||
|
||||
- `dist-upgrade` non-interactively.
|
||||
- Capture `dpkg-query -W -f='${binary:Package}\t${Version}\n'`, sorted, to a file the caller collects.
|
||||
- Reboot unconditionally.
|
||||
- Return; the caller waits.
|
||||
|
||||
`run-integration.sh` gains a `PVE_DIST_UPGRADE` env var that it forwards, and calls `wait-for-api.sh` after `prepare-test-environment.sh` when it is set. `wait-for-api.sh` is used for the first time here.
|
||||
|
||||
*Check:* `bash -n` on both scripts, plus a lane-1 run showing the provisioning path unchanged when the flag is absent.
|
||||
|
||||
### Commit 2 — `package-currency.yml`
|
||||
|
||||
Schedule + `workflow_dispatch`; `concurrency: group: integration-tests`; provisions with `PVE_DIST_UPGRADE=1`; runs the full suite with `continue-on-error` on the test step; uploads the package set as an artifact.
|
||||
|
||||
### Commit 3 — reporting *(landed as #108, revised)*
|
||||
|
||||
Diff the reference node against the baseline on `ci/package-baseline`. On difference: update the data branch, then upsert the rolling issue (`pve-currency`). On no difference: exit quietly.
|
||||
|
||||
Two additions beyond the plan:
|
||||
|
||||
- **Node-vs-node comparison.** A package mismatch *between* the two nested nodes is the failure that left a node unclustered and cost three CI runs to diagnose (see D017). It is reported even when the set is otherwise unchanged.
|
||||
- **Input validation.** The package files come from a machine that just installed from an upstream repo and their contents reach a GitHub issue body, so anything that is not a dpkg name/version pair fails the run.
|
||||
|
||||
### Commit 4 — `DECISIONS.md`
|
||||
|
||||
D017 (two-lane CI: pinned gating lane + report-only currency lane, and why `first-boot.sh` must never upgrade) and D018 (the currency lane reboots after `dist-upgrade` **and proves it rebooted** — the verification was missing from the first draft and is the part easiest to omit).
|
||||
|
||||
## 5. Convention conflict, surfaced
|
||||
|
||||
The global instruction routes architectural decisions to `docs/decisions/` in house ADR format. This repo has no `docs/decisions/` — it records decisions in `DECISIONS.md` as D001–D016, and its own `CLAUDE.md` names that file as the thing to read before writing code. **Repo convention wins:** Lane 2's decisions go in `DECISIONS.md` as D017/D018, not a new `docs/decisions/` tree. Introducing a parallel decision store in a repo that already has one is exactly the defect the gate warns about.
|
||||
|
||||
## 6. Definition of Done
|
||||
|
||||
Discriminating, end-to-end — fails before, passes after:
|
||||
|
||||
1. A manual `workflow_dispatch` of `package-currency.yml` provisions a nested PVE, and the run log shows `dist-upgrade` installing at least one package, a reboot, and `wait-for-api.sh` reporting the API responsive afterwards. *Before the change there is no such workflow.*
|
||||
2. `pveversion` captured after the reboot reports a kernel matching the upgraded `proxmox-kernel-*` package, not the ISO's. This is the check that proves the reboot is real; it fails if the reboot is dropped.
|
||||
3. The full Pester suite runs and its result is recorded. A deliberately failed test does **not** turn the job red.
|
||||
4. With the baseline file absent or stale, the run opens exactly one baseline PR and updates exactly one issue. Re-running with the baseline current opens neither.
|
||||
5. A lane-1 integration run on the same commit is unchanged — no `dist-upgrade`, no reboot — proving the opt-in default holds.
|
||||
|
||||
## 7. Risks
|
||||
|
||||
- **The lane's own instability.** Upgrading is what broke cluster join before. It is contained here: lane 2 never gates a merge, and lane 1 keeps the pin.
|
||||
- **Concurrency starvation.** Sharing the group means a long lane-2 run delays a post-merge lane-1 run. Weekly cadence makes collisions rare; `cancel-in-progress: false` is already the setting, so nothing is lost, only delayed.
|
||||
- **Auto-merge blast radius.** "Allow auto-merge" is repo-wide, not scoped to this lane — any PR can be set to auto-merge. Already enabled, operator-confirmed.
|
||||
- **Report-only hides a real break.** A module genuinely broken against current PVE shows a green weekly check plus an updated issue. Accepted deliberately; revisit after a few releases.
|
||||
@@ -1,124 +0,0 @@
|
||||
# Plan: Integration Test Workflow Refactoring
|
||||
|
||||
**Status**: Planned (not started)
|
||||
**Created**: 2026-03-23
|
||||
**Priority**: Medium — improves CI speed and reliability but not blocking
|
||||
|
||||
## Goals
|
||||
|
||||
1. **Parallel VM provisioning** — provision PVE 8 and PVE 9 VMs simultaneously instead of sequentially
|
||||
2. **File caching** — cache ISOs, cloud images, and OVA files on the self-hosted runner
|
||||
3. **Zero-touch runner setup** — runner no longer needs manual ISO provisioning
|
||||
4. **Extensibility** — easy to add cluster peers, storage VMs in the future
|
||||
|
||||
## Proposed Job Graph
|
||||
|
||||
```
|
||||
build ─────────────┐
|
||||
├──→ provision ──→ test-pve8 ──┐
|
||||
container-image ───┘ (all VMs test-pve9 ──┤──→ cleanup
|
||||
parallel) test-cluster─┘ (always)
|
||||
(future)
|
||||
```
|
||||
|
||||
## Changes Required
|
||||
|
||||
### 1. Terraform: for_each multi-VM provisioning
|
||||
- Refactor main.tf from single-VM to `pve_instances` map variable
|
||||
- Both ISOs upload and both VMs create in a single `terraform apply`
|
||||
- Update variables.tf and outputs.tf
|
||||
|
||||
### 2. New caching scripts
|
||||
- `ensure-base-iso.sh` — download PVE base ISO if not cached in /opt/pve-isos
|
||||
- `ensure-cloud-images.sh` — download cloud image + OVA with ETag-based 7-day TTL
|
||||
- Modify `prepare-auto-iso.sh` — add `--cache-dir` flag with hash-based skip
|
||||
|
||||
### 3. New `provision` job (self-hosted)
|
||||
- Preflight cleanup for ALL VM IDs
|
||||
- Prepare both auto-install ISOs (cached)
|
||||
- Download cloud images once (cached)
|
||||
- Single `terraform apply`
|
||||
- Wait for both PVE installs + create API tokens
|
||||
- Expose connection details as job outputs
|
||||
|
||||
### 4. Test jobs consume provision outputs
|
||||
- Matrix with `max-parallel: 2`
|
||||
- Tests get PVETEST_HOST, PVETEST_APITOKEN from `needs.provision.outputs.*`
|
||||
- No provisioning in test jobs
|
||||
|
||||
### 5. Cleanup job with `if: always()`
|
||||
- API-only cleanup via preflight-cleanup.sh (stateless)
|
||||
- Terraform state as artifact for belt-and-suspenders
|
||||
|
||||
## Estimated Time Savings
|
||||
|
||||
| Phase | Current | Proposed |
|
||||
|---|---|---|
|
||||
| Build + container | 5-10 min | 5-10 min |
|
||||
| Provision (sequential → parallel) | 20-30 min | 10-15 min |
|
||||
| Tests (sequential → parallel if runner allows) | 10-20 min | 5-10 min |
|
||||
| Teardown | 4-6 min | 3-5 min |
|
||||
| **Total** | **~40-60 min** | **~25-40 min** |
|
||||
|
||||
## Dependabot / CI Isolation Lessons (2026-03-23)
|
||||
|
||||
During scan-6 we discovered two issues that this refactoring must account for:
|
||||
|
||||
### 1. cleanup-images must be gated against dependabot
|
||||
|
||||
The `cleanup-images` job uses `actions/delete-package-versions` with `if: always()` to
|
||||
prune old GHCR container images. When a dependabot PR ran, the `integration` job was
|
||||
skipped (correctly), but `cleanup-images` still fired and **deleted the container image
|
||||
that the main branch integration run was actively using**, causing PVE 8 to fail with
|
||||
"image not found".
|
||||
|
||||
**Fix already applied**: All jobs now have `if: github.actor != 'dependabot[bot]'`.
|
||||
|
||||
**For the refactoring**: The new `cleanup` job (Terraform destroy + VM cleanup) must
|
||||
also be gated. Use `if: always() && github.actor != 'dependabot[bot]'` on all jobs
|
||||
that touch shared resources (self-hosted runner, GHCR, PVE host).
|
||||
|
||||
### 2. Container image tags must survive concurrent cleanup
|
||||
|
||||
The current `cleanup-images` job deletes all but 1 container image version. If two
|
||||
workflow runs overlap (e.g., a push to main while a prior run is still testing), the
|
||||
cleanup from the first run can delete the image needed by the second.
|
||||
|
||||
**For the refactoring**: Consider one of:
|
||||
- **Tag images by run ID** instead of commit SHA, and only delete images older than
|
||||
the current run
|
||||
- **Pin `min-versions-to-keep: 3`** to survive overlapping runs
|
||||
- **Move cleanup to a scheduled workflow** (weekly) instead of per-run
|
||||
|
||||
### 3. Self-hosted runner disk space
|
||||
|
||||
The runner ran out of disk space during an integration test run, leaving it in a broken
|
||||
state that required manual rebuilding. The caching strategy must account for this:
|
||||
- Set a maximum cache size or file count in `/opt/pve-isos`
|
||||
- Auto-prune ISOs not referenced by the current workflow matrix
|
||||
- The cleanup job should clean up uploaded ISOs from the PVE host, not just VMs
|
||||
- Consider a periodic runner maintenance script that frees disk space
|
||||
|
||||
## Implementation Order
|
||||
|
||||
1. Refactor Terraform (main.tf, variables.tf, outputs.tf) → for_each
|
||||
2. Create ensure-base-iso.sh and ensure-cloud-images.sh
|
||||
3. Add --cache-dir to prepare-auto-iso.sh
|
||||
4. Create parallel wait wrapper for create-api-token.sh
|
||||
5. Restructure workflow into provision → test → cleanup jobs
|
||||
6. Update prepare-test-environment.sh for cache dir
|
||||
7. Ensure all jobs gated with `github.actor != 'dependabot[bot]'`
|
||||
8. Address container image cleanup race condition
|
||||
9. Test on runner via workflow_dispatch
|
||||
|
||||
## Files to Modify
|
||||
|
||||
- .github/workflows/integration-tests.yml
|
||||
- tests/infrastructure/main.tf
|
||||
- tests/infrastructure/variables.tf
|
||||
- tests/infrastructure/outputs.tf
|
||||
- tests/infrastructure/scripts/prepare-auto-iso.sh
|
||||
- tests/infrastructure/scripts/prepare-test-environment.sh
|
||||
- tests/infrastructure/scripts/create-api-token.sh (wrapper)
|
||||
- New: tests/infrastructure/scripts/ensure-base-iso.sh
|
||||
- New: tests/infrastructure/scripts/ensure-cloud-images.sh
|
||||
@@ -1,307 +0,0 @@
|
||||
# PSProxmoxVE Review Report — Scan 9
|
||||
|
||||
```
|
||||
Scan date: 2026-03-26
|
||||
Prior report date: 2026-03-24
|
||||
PVE API spec date: 2026-03-21T15:04:50.641Z
|
||||
PVE API spec SHA256: 4af79be30166209a4714b771f65e1e9540c5b738f414ff30c98454402e29d030
|
||||
PVE version hint: (not set)
|
||||
Total API endpoints: 646
|
||||
Findings DB: docs/review/findings.json (F001–F085)
|
||||
Open findings: 11 (before scan) → 7 (after scan)
|
||||
New this scan: 0 Resolved this scan: 4 Regressed: 0
|
||||
Last CI run: integration-tests.yml | success | 2026-03-26T00:51:58Z | run 23571920298
|
||||
```
|
||||
|
||||
## Executive Summary
|
||||
|
||||
- **Delta**: 4 resolved | 0 new | 0 regressed | 7 still open
|
||||
- **Resolved this scan**: F039 (bare catch regression fixed), F053 (HA subsystem implemented), F060 (cluster config implemented), F084 (PveSession secret hiding)
|
||||
- **API drift**: 37 PVE 9.0 endpoints unimplemented (down from 42); 0 breaking changes detected in implemented endpoints
|
||||
- **CI**: Last integration run PASSED (both PVE 8 and PVE 9) on 2026-03-26; 0 test failures
|
||||
- **Coverage**: ~172 cmdlets across 16 functional areas; 7 PVE subsystems remain uncovered (Ceph, disks, notifications, ACME/certs, most PVE 9.0 endpoints)
|
||||
- **Code quality**: All 13 DECISIONS.md patterns verified — no regressions detected
|
||||
- **Security**: All password params use SecureString; URL encoding consistent; PveSession default output now hides secrets
|
||||
|
||||
---
|
||||
|
||||
## Phase 1 — Repository Inventory & Structure
|
||||
|
||||
| Item | Present | Notes |
|
||||
|---|---|---|
|
||||
| Solution file (.sln) | Yes | PSProxmoxVE.sln |
|
||||
| Source projects | Yes | src/PSProxmoxVE/ (cmdlets), src/PSProxmoxVE.Core/ (services/models) |
|
||||
| Test projects | Yes | xUnit (PSProxmoxVE.Core.Tests), Pester (PSProxmoxVE.Tests) |
|
||||
| CI/CD workflows | Yes | build.yml, unit-tests.yml, integration-tests.yml, publish.yml, claude.yml, claude-code-review.yml |
|
||||
| README.md | Yes | Badges, installation, usage, cmdlet list |
|
||||
| CHANGELOG.md | Yes | 0.1.0-preview entry |
|
||||
| CONTRIBUTING.md | Yes | .NET 10.0+ SDK, build/test instructions |
|
||||
| LICENSE | Yes | MIT |
|
||||
| CODE_OF_CONDUCT.md | Yes | Contributor Covenant v2.1 |
|
||||
| SECURITY.md | Yes | Vulnerability disclosure policy |
|
||||
| DECISIONS.md | Yes | 13 active decisions (D001–D013) |
|
||||
| CODEOWNERS | Yes | Single maintainer |
|
||||
| PSGallery manifest (.psd1) | Yes | 0.1.0-preview, 172 cmdlets exported |
|
||||
| .editorconfig | Yes | C# and PowerShell rules |
|
||||
| .gitignore | Yes | Standard .NET + PS patterns |
|
||||
| .gitattributes | Yes | Line ending normalization |
|
||||
| Issue/PR templates | Yes | Bug report, feature request YAML + PR template |
|
||||
| Dependabot | Yes | NuGet + GitHub Actions weekly |
|
||||
| docs/review/ | Yes | findings.json (F001–F085), REVIEW_REPORT.md |
|
||||
| docs/cmdlets/ | Yes | 170 markdown help docs |
|
||||
| MAML help | Yes | PSProxmoxVE.dll-Help.xml |
|
||||
| Format file | Yes | PSProxmoxVE.format.ps1xml (models + PveSession) |
|
||||
|
||||
**Missing**: IconUri in manifest (F021 — cosmetic).
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 — PVE API Coverage Audit
|
||||
|
||||
### Coverage by Functional Area
|
||||
|
||||
| Area | Total Endpoints | Covered (approx.) | % | Notable Gaps |
|
||||
|---|---|---|---|---|
|
||||
| vms | 97 | ~45 | 46% | dbus-vmstate (PVE 9.0), pending/current config |
|
||||
| containers | 62 | ~25 | 40% | LXC migrate GET (PVE 9.0), firewall per-CT |
|
||||
| firewall | 40 | ~21 | 53% | VM/CT-level firewall via Level param |
|
||||
| sdn | 60 | ~20 | 33% | SDN fabrics (14 new PVE 9.0), lock/rollback |
|
||||
| ha | 21 | ~14 | 67% | Status details, fencing config |
|
||||
| cluster | 77 | ~12 | 16% | Bulk actions (PVE 9.0), metrics, replication |
|
||||
| cluster_config | 10 | ~8 | 80% | — |
|
||||
| nodes | 75 | ~8 | 11% | Capabilities, hardware scan, syslog, journal |
|
||||
| storage | 19 | ~10 | 53% | OCI registry pull (PVE 9.0) |
|
||||
| storage_config | 5 | ~3 | 60% | — |
|
||||
| access | 15 | ~5 | 33% | VNC ticket (PVE 9.0), TFA |
|
||||
| users | 12 | ~8 | 67% | — |
|
||||
| access_groups | 5 | ~4 | 80% | — |
|
||||
| access_domains | 6 | ~4 | 67% | Sync endpoint |
|
||||
| roles | 5 | ~3 | 60% | — |
|
||||
| pools | 7 | ~4 | 57% | — |
|
||||
| acl | 2 | ~2 | 100% | — |
|
||||
| tasks | 5 | ~4 | 80% | — |
|
||||
| backup | 6 | ~5 | 83% | — |
|
||||
| ceph | 40 | 0 | 0% | F054 — entire subsystem |
|
||||
| disks | 18 | 0 | 0% | F067 — LVM, ZFS, SMART |
|
||||
| acme | 15 | 0 | 0% | F069 — certificate management |
|
||||
| certificates | 8 | 0 | 0% | F069 — TLS certs |
|
||||
| services | 7 | 0 | 0% | Node service management |
|
||||
| networking | 7 | ~5 | 71% | — |
|
||||
| apt | 8 | 0 | 0% | Package management |
|
||||
| metrics | 7 | 0 | 0% | External metric servers |
|
||||
| replication | 5 | 0 | 0% | Storage replication |
|
||||
| version | 1 | 0 | 0% | PVE version endpoint |
|
||||
|
||||
**Overall**: ~210/646 endpoints covered (~33%)
|
||||
|
||||
### PVE 9.0 New Endpoints (42 total)
|
||||
|
||||
| Status | Endpoint | Area |
|
||||
|---|---|---|
|
||||
| **Covered** | GET/POST/GET/{id}/PUT/{id}/DELETE/{id} /cluster/ha/rules | ha |
|
||||
| Missing | GET/POST /cluster/bulk-action/guest/* (6) | cluster |
|
||||
| Missing | /cluster/sdn/fabrics/* (14) | sdn |
|
||||
| Missing | /cluster/sdn/lock, /cluster/sdn/rollback (3) | sdn |
|
||||
| Missing | POST /nodes/{node}/qemu/{vmid}/dbus-vmstate | vms |
|
||||
| Missing | GET /nodes/{node}/lxc/{vmid}/migrate | containers |
|
||||
| Missing | GET /nodes/{node}/capabilities/qemu/* (2) | nodes |
|
||||
| Missing | POST /nodes/{node}/storage/{storage}/oci-registry-pull | storage |
|
||||
| Missing | /nodes/{node}/sdn/* (8) | nodes |
|
||||
| Missing | GET /nodes/{node}/query-oci-repo-tags | nodes |
|
||||
| Missing | POST /access/vncticket | access |
|
||||
|
||||
**PVE 9.0 coverage: 5/42 (12%)**
|
||||
|
||||
### API Drift — Breaking Changes
|
||||
|
||||
No breaking changes detected in PVE 9.0 for currently implemented endpoints. The 248 parameter changes in PVE 9.0 are predominantly additive (new optional parameters) and do not break existing cmdlet behavior.
|
||||
|
||||
---
|
||||
|
||||
## Phase 3 — Code Quality & Best Practices
|
||||
|
||||
### DECISIONS.md Compliance Check
|
||||
|
||||
| Decision | Status | Evidence |
|
||||
|---|---|---|
|
||||
| D001 — TaskService.WaitForTask | Compliant | `while(true)` only in TaskService.cs:114 and WaitPveTaskCmdlet.cs:69 (the implementations themselves, with timeout) |
|
||||
| D002 — SecureString passwords | Compliant | All 7 password cmdlet params use SecureString with Marshal try/finally |
|
||||
| D003 — Uri.EscapeDataString | Compliant | All service path parameters properly escaped |
|
||||
| D004 — No bare catches | **Resolved** | F039 regression fixed. All catches are specific or filtered. |
|
||||
| D005 — OutputType required | Compliant | All cmdlets have [OutputType] |
|
||||
| D006 — ConfirmImpact.High | Compliant | All destructive cmdlets have ConfirmImpact.High |
|
||||
| D007 — Sealed cmdlets | Compliant | All cmdlet classes are sealed |
|
||||
| D008 — Newtonsoft only | Compliant | No [JsonPropertyName] in source |
|
||||
| D009 — netstandard2.0 | Compliant | Both .csproj target netstandard2.0; test targets net10.0+net48 |
|
||||
| D010 — VmId ValidateRange | Compliant | All VmId params have [ValidateRange(100, 999999999)] |
|
||||
| D011 — Verb class constants | Compliant | No string literal verbs found |
|
||||
| D012 — Magic strings | Compliant | Auth constants extracted |
|
||||
| D013 — No Newtonsoft in public API | Compliant | F085 resolved; JObject/JArray only used internally for parsing |
|
||||
|
||||
### Code Quality Findings
|
||||
|
||||
| Finding ID | File | Severity | Status | Description |
|
||||
|---|---|---|---|---|
|
||||
| F039 | VmService.cs, ImportPveOvaCmdlet.cs | Medium | **Resolved** | Bare catch regression fixed. VmService:554 catches PveApiException; ImportPveOvaCmdlet:108 catches Exception+ThrowTerminatingError |
|
||||
| F048 | src/PSProxmoxVE.Core/ | Medium | Wont_fix | ~216 sync-over-async via GetAwaiter().GetResult() — accepted pattern for PS 5.1 compat |
|
||||
|
||||
**Note on JObject usage**: JObject/JArray/JToken are used extensively in services and cmdlets for internal JSON parsing (response deserialization). This is acceptable per D013 — the restriction is on **public API types** (model properties, OutputType), not internal implementation. No Newtonsoft types appear in model properties or OutputType attributes.
|
||||
|
||||
### HttpClient Lifecycle (F045)
|
||||
|
||||
Resolved in prior scan. IPveHttpClient interface extracted; all 14 services accept shared client via constructor injection.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4 — Testing Coverage Analysis
|
||||
|
||||
### Test Infrastructure
|
||||
|
||||
| Component | Framework | Target |
|
||||
|---|---|---|
|
||||
| PSProxmoxVE.Core.Tests | xUnit 2.9.3 | net10.0, net48 |
|
||||
| PSProxmoxVE.Tests | Pester 5 | PowerShell 7.x |
|
||||
| Integration tests | Pester 5 | Live PVE 8 + PVE 9 |
|
||||
|
||||
### Test Coverage by Area
|
||||
|
||||
| Area | xUnit (Models/Services) | Pester (Unit) | Integration | Notes |
|
||||
|---|---|---|---|---|
|
||||
| Connection | — | Yes | Yes (00_Connection) | |
|
||||
| Nodes | NodeModelTests | Yes | Yes (01_Nodes) | |
|
||||
| Users | UserModelTests, UserServiceTests | Yes | Yes (02_Users) | |
|
||||
| Storage | StorageModelTests, StorageServiceTests | Yes | Yes (03_Storage, 03a_Shared) | |
|
||||
| Network | NetworkModelTests | Yes | Yes (04_Network) | |
|
||||
| SDN | SdnModelTests | Yes | Yes (05_SDN) | |
|
||||
| VMs | VmModelTests | Yes | Yes (06_VMs) | |
|
||||
| Snapshots | SnapshotModelTests, SnapshotServiceTests | Yes | Yes (07_Snapshots) | |
|
||||
| Templates | TemplateServiceTests | Yes | Yes (08_Templates) | |
|
||||
| CloudInit | CloudInitServiceTests | Yes | Yes (09_CloudInit) | |
|
||||
| Containers | ContainerModelTests | Yes | Yes (10_Containers) | |
|
||||
| Firewall | FirewallModelTests | Yes | Yes (13_Firewall) | |
|
||||
| Backup | BackupModelTests, BackupServiceTests | Yes | Yes (14_Backup) | |
|
||||
| Tasks | TaskModelTests, TaskServiceTests | Yes | Yes (15_Tasks) | |
|
||||
| Cluster | ClusterModelTests, ClusterServiceTests, ClusterConfigServiceTests | Yes | Yes (16_Cluster) | |
|
||||
| HA | HaServiceTests | Yes (HaCmdlets.Tests.ps1) | Yes (16_Cluster) | Full CRUD for groups, rules, resources, status |
|
||||
| Pools | PoolServiceTests | Yes (PoolCmdlets.Tests.ps1) | No | F046 — no integration tests |
|
||||
|
||||
### xUnit Test Stats
|
||||
|
||||
- 12 model test files, 12 service test files
|
||||
- ~382 total xUnit tests (196 service tests added in recent remediation)
|
||||
|
||||
### Test Quality Assessment
|
||||
|
||||
- **Structure**: Arrange/Act/Assert pattern used consistently
|
||||
- **Isolation**: Service tests use Moq-based IPveHttpClient mocks
|
||||
- **Fixtures**: JSON fixture files for PVE 8 and 9 response deserialization
|
||||
- **Integration**: 18 integration test files (00–16 + 99_Cleanup) covering happy paths
|
||||
- **Edge cases**: Model tests cover null/missing fields; service tests verify URL construction
|
||||
|
||||
### Finding F046: Integration Test Gaps
|
||||
|
||||
Still open. HA cmdlets now have integration coverage in 16_Cluster.Tests.ps1 (status, groups, rules, resources). Key areas still without integration tests: pool management, some newer SDN/firewall operations. Estimated 55-65 of ~172 cmdlets lack integration coverage.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4b — CI Integration Test Results
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Workflow | integration-tests.yml |
|
||||
| Latest completed run | 23571920298 |
|
||||
| Conclusion | **success** |
|
||||
| Date | 2026-03-26T00:51:58Z |
|
||||
| Branch | main |
|
||||
| SHA | d375695 |
|
||||
|
||||
**Last integration run: PASSED** (both PVE 8 and PVE 9 matrices). No CI findings to generate.
|
||||
|
||||
A newer run (23596586594) is currently in_progress.
|
||||
|
||||
The most recent failure (23566610350, 2026-03-25) was a provisioning infrastructure issue (Docker/Terraform), not a test logic failure.
|
||||
|
||||
---
|
||||
|
||||
## Phase 5 — Security Review
|
||||
|
||||
| Finding ID | Area | File | Severity | Status | Description |
|
||||
|---|---|---|---|---|---|
|
||||
| F084 | Credential exposure | PveSession.cs, format.ps1xml | Medium | **Resolved** | format.ps1xml now hides Ticket/ApiToken/CsrfToken from default output |
|
||||
| — | Credential handling | Cmdlets/ | — | Pass | All 7 password params use SecureString with Marshal try/finally |
|
||||
| — | TLS/HTTPS | PveHttpClient.cs | — | Pass | HTTPS enforced; SkipCertificateCheck opt-in with WriteWarning |
|
||||
| — | URL encoding | Services/ | — | Pass | Uri.EscapeDataString on all dynamic path segments |
|
||||
| — | Secret scanning | All files | — | Pass | No hardcoded credentials in tracked files; .env.test gitignored |
|
||||
| — | Dependencies | .csproj files | — | Pass | Newtonsoft.Json 13.0.3, SharpCompress 0.38.0, PowerShellStandard.Library 5.1.1 |
|
||||
|
||||
No new security findings this scan.
|
||||
|
||||
---
|
||||
|
||||
## Phase 6 — PSGallery Publication Readiness
|
||||
|
||||
| Finding ID | Check | Pass/Fail | Notes |
|
||||
|---|---|---|---|
|
||||
| — | ModuleVersion | Pass | 0.1.0 |
|
||||
| — | GUID | Pass | a3f7c2d1-84e5-4b9f-a061-3e2d8c5f1a7b |
|
||||
| — | Author/CompanyName | Pass | goodolclint / Worklab |
|
||||
| — | Description | Pass | Comprehensive, mentions PVE 8.x and 9.x |
|
||||
| — | PowerShellVersion | Pass | 5.1 |
|
||||
| — | CompatiblePSEditions | Pass | Desktop, Core |
|
||||
| — | Tags | Pass | 8 tags including Proxmox, PVE, IaC |
|
||||
| — | LicenseUri | Pass | Points to LICENSE on main |
|
||||
| — | ProjectUri | Pass | GitHub repo URL |
|
||||
| F021 | IconUri | **Fail** | Missing — cosmetic only |
|
||||
| — | ReleaseNotes | Pass | Preview release description |
|
||||
| — | CmdletsToExport | Pass | 172 cmdlets listed |
|
||||
| — | RequiredAssemblies | Pass | PSProxmoxVE.Core.dll, Newtonsoft.Json.dll |
|
||||
| — | FormatsToProcess | Pass | PSProxmoxVE.format.ps1xml |
|
||||
| — | netstandard2.0 target | Pass | Both projects target only netstandard2.0 |
|
||||
| — | Publish workflow | Pass | Tag-triggered, PS 5.1 smoke test, threshold >= 150 |
|
||||
| — | DotNetFrameworkVersion | Pass | 4.8 |
|
||||
| — | HelpInfoUri | Pass | Points to docs/cmdlets/ |
|
||||
|
||||
---
|
||||
|
||||
## Phase 7 — Community & Repo Maintenance
|
||||
|
||||
| Finding ID | Check | Pass/Fail | Notes |
|
||||
|---|---|---|---|
|
||||
| — | Issue templates | Pass | Bug report + feature request (YAML) + config.yml |
|
||||
| — | PR template | Pass | Structured checklist |
|
||||
| — | CONTRIBUTING.md | Pass | .NET 10.0+ SDK, build/test, coding standards, PR process |
|
||||
| — | CODE_OF_CONDUCT.md | Pass | Contributor Covenant v2.1 |
|
||||
| — | SECURITY.md | Pass | Vulnerability disclosure with 48h response SLA |
|
||||
| — | CODEOWNERS | Pass | Single maintainer |
|
||||
| — | LICENSE | Pass | MIT |
|
||||
| — | .editorconfig | Pass | C# and PS rules |
|
||||
| — | .gitattributes | Pass | Line ending normalization |
|
||||
| — | Branch protection | Pass | Documented in CLAUDE.md |
|
||||
| — | Commit conventions | Pass | Conventional commits |
|
||||
| — | DECISIONS.md | Pass | 13 active decisions, linked from CLAUDE.md |
|
||||
| — | Dependabot | Pass | NuGet + GitHub Actions weekly |
|
||||
| — | CHANGELOG | Pass | 0.1.0-preview entry |
|
||||
| — | Release process | Pass | Tag-triggered publish.yml with GitHub Releases |
|
||||
|
||||
All community standards met. No findings.
|
||||
|
||||
---
|
||||
|
||||
## Phase 9 — Prioritized Recommendations
|
||||
|
||||
### 🟡 Medium
|
||||
|
||||
| Finding ID | What | Where | Why | Fix |
|
||||
|---|---|---|---|---|
|
||||
| F046 | Integration test coverage gaps | tests/Integration/ | ~55-65 cmdlets lack end-to-end integration tests | Add integration tests for pools, newer SDN/firewall operations |
|
||||
| F061 | PVE 9.0 endpoints partially covered (5/42) | — | 37 new PVE 9.0 endpoints unimplemented (SDN fabrics, bulk actions, OCI registry) | Prioritize SDN fabrics (14 endpoints) and bulk actions (6 endpoints) |
|
||||
|
||||
### 🟢 Low
|
||||
|
||||
| Finding ID | What | Where | Why | Fix |
|
||||
|---|---|---|---|---|
|
||||
| F021 | No IconUri in manifest PSData | PSProxmoxVE.psd1 | Cosmetic — improves PSGallery listing appearance | Add icon to repo and reference in manifest |
|
||||
| F054 | Ceph subsystem 0% coverage | — | 40 endpoints for OSD, MON, pools, status. Critical for hyperconverged setups | Implement Ceph cmdlets when demand warrants |
|
||||
| F067 | Disk management 0% coverage | — | 18 endpoints for LVM, ZFS, SMART. Needed for storage provisioning | Implement disk cmdlets |
|
||||
| F068 | Notifications 0% coverage | — | 32 cluster notification endpoints (PVE 8.1+) | Implement notification cmdlets |
|
||||
| F069 | ACME/Certificates 0% coverage | — | 23 combined endpoints for TLS certificate management | Implement ACME/cert cmdlets |
|
||||
File diff suppressed because it is too large
Load Diff
@@ -217,7 +217,7 @@ namespace PSProxmoxVE.Core.Services
|
||||
/// <param name="session">The authenticated PVE session.</param>
|
||||
/// <param name="hostname">The hostname or IP of an existing cluster node.</param>
|
||||
/// <param name="fingerprint">The TLS certificate fingerprint of the cluster node.</param>
|
||||
/// <param name="password">The root password for the cluster node (plain string; cmdlet layer handles SecureString conversion per D002).</param>
|
||||
/// <param name="password">The root password for the cluster node (plain string; cmdlet layer handles SecureString conversion per ADR 0002).</param>
|
||||
/// <param name="links">Optional Corosync link addresses.</param>
|
||||
/// <param name="nodeid">Optional node ID for this node.</param>
|
||||
/// <param name="votes">Optional number of quorum votes.</param>
|
||||
|
||||
@@ -20,7 +20,7 @@ namespace PSProxmoxVE.Core.Utilities
|
||||
/// qmpstatus=paused for a suspended VM.
|
||||
/// Locked: the guest config carries a `lock:` property (backup, clone, migrate,
|
||||
/// snapshot). This is not the /var/lock/qemu-server flock, which PVE does not expose
|
||||
/// through status/current or any other endpoint — see DECISIONS.md D015 and D020.
|
||||
/// through status/current or any other endpoint — see docs/decisions/ ADR 0015 and 0020.
|
||||
/// </returns>
|
||||
public static (bool StatusMatched, bool Locked) Evaluate(string json, string expectedStatus)
|
||||
{
|
||||
|
||||
@@ -39,7 +39,7 @@ RUN if [ "$(dpkg --print-architecture)" = "amd64" ]; then \
|
||||
# Install Pester and prepare module directory.
|
||||
# Pinned, not floored: this image is rebuilt on every CI run, so a version range
|
||||
# lets a new Pester major reach the gating lane with no commit to this repo.
|
||||
# Bump deliberately, the way D017 treats the nested PVE package set.
|
||||
# Bump deliberately, the way ADR 0017 treats the nested PVE package set.
|
||||
ARG PESTER_VERSION=6.1.0
|
||||
RUN pwsh -NoProfile -Command \
|
||||
"Set-PSRepository -Name PSGallery -InstallationPolicy Trusted; \
|
||||
|
||||
@@ -209,7 +209,7 @@ namespace PSProxmoxVE.Core.Tests.Models
|
||||
Assert.Equal(2, config!.Cores); // typed property still works
|
||||
Assert.True(config.AdditionalProperties.ContainsKey("hostpci0"));
|
||||
Assert.Equal("0000:01:00.0,pcie=1", config.AdditionalProperties["hostpci0"]);
|
||||
// Value must be a native type (string), never a Newtonsoft JToken (D013).
|
||||
// Value must be a native type (string), never a Newtonsoft JToken (ADR 0013).
|
||||
Assert.IsType<string>(config.AdditionalProperties["hostpci0"]);
|
||||
Assert.DoesNotContain("Newtonsoft", config.AdditionalProperties["hostpci0"]!.GetType().FullName);
|
||||
}
|
||||
|
||||
@@ -112,7 +112,7 @@ Describe 'Cluster Config & HA Lifecycle — Integration' -Tag 'Integration' {
|
||||
$result | Should -Not -BeNullOrEmpty
|
||||
$script:ClusterCreated = $true
|
||||
|
||||
# New-PveCluster -Wait returns only once the cluster is quorate (D014),
|
||||
# New-PveCluster -Wait returns only once the cluster is quorate (ADR 0014),
|
||||
# which is what makes the join below safe without a sleep.
|
||||
$cluster = @(Get-PveClusterStatus -ErrorAction Stop) |
|
||||
Where-Object { $_.Type -eq 'cluster' } | Select-Object -First 1
|
||||
|
||||
Reference in New Issue
Block a user