diff --git a/.specify/feature.json b/.specify/feature.json index 95a6f656..51a82834 100644 --- a/.specify/feature.json +++ b/.specify/feature.json @@ -1,3 +1,3 @@ { - "feature_directory": "specs/012-web-adopt-pairing" + "feature_directory": "specs/013-managed-instance-tags" } diff --git a/README.md b/README.md index 87cd0b82..5a7e3e07 100644 --- a/README.md +++ b/README.md @@ -255,8 +255,9 @@ remo aws info [--name N] # Show type, cores, memory, EBS size remo incus create --name [--host H] # Create container remo incus list # List registered containers remo incus info --name # Show cores, memory, root size -remo incus sync [--host H] # Discover existing containers -remo incus update --name # Update dev tools +remo incus sync [--host H] # Discover remo-managed containers +remo incus sync [--host H] --all # Also adopt non-remo containers on the host +remo incus update --name # Update dev tools (also marks as remo-managed) remo incus update --name --volume-size 40 --cores 4 --memory 4096 remo incus destroy --name [--yes] # Destroy container remo incus bootstrap # Initialize Incus on host @@ -265,8 +266,9 @@ remo incus bootstrap # Initialize Incus on host remo proxmox create --name --host # Create LXC container remo proxmox list # List registered containers remo proxmox info --name # Show cores, memory, rootfs size -remo proxmox sync --host # Discover existing containers -remo proxmox update --name # Update dev tools +remo proxmox sync --host # Discover remo-managed containers +remo proxmox sync --host --all # Also adopt non-remo containers on the node +remo proxmox update --name # Update dev tools (also marks as remo-managed) remo proxmox update --name --volume-size 40 --cores 4 --memory 4096 remo proxmox destroy --name [--yes] [--purge] # Destroy container remo proxmox bootstrap --host # Verify node + download LXC template @@ -353,10 +355,19 @@ discovery states, terminal limits, troubleshooting, and upgrade notes: ```bash remo aws sync # Discover AWS instances with 'remo' tag remo hetzner sync # Discover Hetzner VMs with 'remo' label -remo incus sync # Discover Incus containers -remo proxmox sync --host # Discover Proxmox LXC containers +remo incus sync # Discover remo-managed Incus containers +remo proxmox sync --host # Discover remo-managed Proxmox LXC containers ``` +All four providers now filter `sync` to the containers/instances **remo created**. +On Incus/Proxmox, `remo`-created containers are marked at provision time (an Incus +`user.remo=true` config key or a Proxmox `remo` guest tag), and a default `sync` +registers only those. To adopt containers `remo` did not create, use +`sync --all` (a one-time, unmarked adoption) or `remo update ` +(permanently marks one). Containers created before this feature are unmarked; +the first default `sync` after upgrading names them and prints both remedies +rather than silently dropping or re-marking them. + **SSH connection fails?** ```bash ssh-keygen -t rsa -b 4096 -f ~/.ssh/id_rsa diff --git a/specs/013-managed-instance-tags/contracts/cli-sync.md b/specs/013-managed-instance-tags/contracts/cli-sync.md new file mode 100644 index 00000000..bcd519bb --- /dev/null +++ b/specs/013-managed-instance-tags/contracts/cli-sync.md @@ -0,0 +1,59 @@ +# CLI Contract: `sync --all` and filtered-sync output + +Applies to `remo incus sync` and `remo proxmox sync`. No other command's flags +change. AWS/Hetzner `sync` is untouched (FR-011). + +## New flag + +``` +--all Register every container discovered on the host, including those + without the remo managed marker (pre-feature behavior). Default off. +``` + +- Type: boolean `is_flag` (Click), threaded to `providers.

.sync(all=)`. +- Coexists with the existing `--host`, `--user`, `--use-ip` options. + +## Behavior contract + +| Host state | Command | Registered | stdout hint/summary | +|------------|---------|-----------|---------------------| +| mix of marked + unmarked | `sync` (default) | only marked | names skipped unmarked containers + count + both remedies | +| all marked | `sync` (default) | all | normal `Synced N container(s)…` (no skip hint) | +| all unmarked | `sync` (default) | none | `Synced 0…` + skip hint naming all skipped + remedies | +| any | `sync --all` | all | normal summary; if ≥1 was unmarked, ALSO a line distinguishing the unmarked/adopted count + round-trip warning | + +## Output contract — default filtered sync that skipped containers (FR-008) + +Must include, in this spirit (exact wording flexible): + +``` +Synced 1 container(s) from ''. +Skipped 2 unmarked container(s): plex, homeassistant + • Adopt all this run: remo sync --host --all + • Mark one permanently: remo update +``` + +Requirements: +- The skipped container **names** are listed (clarification 1), not just a count. +- Both remedies are named: `--all` and `remo update `. +- Emitted via `core.output` (`print_info`/`print_warning`), consistent with the + existing `Synced N…` line. + +## Output contract — `--all` adopting unmarked containers (FR-009) + +``` +Synced 3 container(s) from '' (2 not remo-created; adopted via --all). +Note: a later default `sync` will drop the 2 unmarked one(s) again. +``` + +Requirements: +- The count of registered-but-unmarked containers is distinguished from the + total (FR-009). +- The round-trip behavior is stated plainly (Edge Case: mixed-marker `--all`). + +## Non-goals (unchanged behavior) + +- Registry line format is unchanged (FR-012). +- `remo shell` / `remo cp` connection path is unchanged (FR-012). +- Lifecycle commands (`destroy`, `snapshot`, resize) gain **no** marker check; + they operate uniformly on any registry entry (clarification 2). diff --git a/specs/013-managed-instance-tags/contracts/marker-commands.md b/specs/013-managed-instance-tags/contracts/marker-commands.md new file mode 100644 index 00000000..e3c8c949 --- /dev/null +++ b/specs/013-managed-instance-tags/contracts/marker-commands.md @@ -0,0 +1,58 @@ +# Host-Command Contract: marker apply & read + +All commands run through the existing per-host SSH helpers +(`_ssh_run_on_incus_host` / `_ssh_run`), which also handle `host == "localhost"` +for Incus. ``/`` are `shlex.quote`d. + +## Incus + +### Apply marker (create + update) — idempotent (FR-001, FR-002, FR-004) + +``` +incus config set user.remo=true +``` +- Success: rc 0 (no-op when already set — SC-005). +- Failure: warn per FR-005; do not fail the enclosing command on this alone. + +### Read markers for sync — single bulk query (FR-013) + +``` +incus list -f csv -c n,user.remo +``` +- Output rows: `,`; `marker-value == "true"` ⇒ marked. +- Default sync keeps marked rows; `--all` keeps all and counts `!= "true"`. +- Fallback (older Incus, if the column form is unreliable): two queries — + `incus list -f csv -c n` (all) and `incus list user.remo=true -f csv -c n` + (marked) — still bounded, still no per-container round-trip. + +## Proxmox + +### Apply marker (create + update) — union, preserve tags (FR-001..FR-004) + +Read current tags, then write only if `remo` is absent: +``` +pct config # parse the `tags:` line → set +# if "remo" not in set: +pct set --tags "" # existing order preserved, remo appended +``` +- Separator: split read on `[;, ]+`; join write with `;`. +- `remo` already present ⇒ skip the `pct set` entirely (strict no-op, no + reorder — SC-005, FR-003). +- Failure: warn per FR-005; do not fail the enclosing command on this alone. + +### Read markers for sync — inventory + one bulk tag read (FR-013) + +``` +pct list # existing: vmid + name inventory +grep -H '^tags:' /etc/pve/lxc/*.conf # one round-trip: vmid → tag line +``` +- `.conf` with `remo` in its tag line ⇒ that container is marked. +- A vmid absent from the grep output (no `tags:` line) ⇒ unmarked. +- Consistent with existing snapshot code that reads `/etc/pve/lxc/.conf`. + +## Invariants across both providers + +- `sync` issues **no** apply/remove/modify command — read-only (FR-010). +- Marker literals come only from `core/config.py` constants (fixed, not + configurable). +- Behavior is identical for localhost and remote Incus hosts. diff --git a/specs/013-managed-instance-tags/data-model.md b/specs/013-managed-instance-tags/data-model.md new file mode 100644 index 00000000..83538441 --- /dev/null +++ b/specs/013-managed-instance-tags/data-model.md @@ -0,0 +1,80 @@ +# Phase 1 Data Model: Managed-Instance Tagging & Filtered Sync + +This feature adds provider-side metadata and a small amount of in-memory +sync-time state. It introduces **no** new persisted local entity and does not +change the `KnownHost` registry representation (FR-012, out-of-scope: registry +does not record marker state). + +## Entity: Managed Marker (provider-side, authoritative) + +A fixed, built-in piece of container metadata indicating the container was +created and is managed by `remo`. Not user-configurable (clarified). + +| Provider | Form | Key/Value | Namespace safety | +|----------|------|-----------|------------------| +| Incus | config key | `user.remo` = `true` | `user.*` is reserved for user metadata; cannot collide with Incus keys | +| Proxmox | guest tag | bare tag `remo` in the tag set | tag is one member of a set; other tags preserved | + +**Constants** (single source, `core/config.py`): +- `INCUS_MANAGED_CONFIG_KEY = "user.remo"` +- `INCUS_MANAGED_CONFIG_VALUE = "true"` +- `PROXMOX_MANAGED_TAG = "remo"` + +**Lifecycle / state transitions**: + +``` +unmarked ──create()──▶ marked (FR-001: applied at provision time) +unmarked ──update()──▶ marked (FR-004: backfill; idempotent) +marked ──create()/update()──▶ marked (FR-002: no-op re-apply) +marked ──user removes tag/key manually──▶ unmarked (Edge Case: intentional "unmanage"; sync does not re-add) +``` + +`sync` never transitions this state (FR-010: read-only on container state). + +**Validation / invariants**: +- **Idempotency (FR-002)**: re-applying MUST NOT alter any other config. + - Incus: `incus config set user.remo=true` on an already-set key is a + no-op by construction. + - Proxmox: apply only writes when `remo ∉ tags`; otherwise it is skipped, so + the tag list is never reordered (SC-005). +- **Tag preservation (FR-003)**: Proxmox apply computes `new = existing ∪ + {remo}` and writes `;`-joined; no existing tag removed or altered. +- **Apply-failure tolerance (FR-005)**: a failed apply during `create`/`update` + warns but does not, by itself, fail the command when the container was + otherwise created/configured. + +## In-memory value: Discovered Container (sync-time, transient) + +Produced while scanning a host; never persisted with marker state. + +| Field | Type | Source (Incus) | Source (Proxmox) | +|-------|------|----------------|------------------| +| `name` | str | col `n` of `incus list` | Name column of `pct list` | +| `vmid` | str | (n/a) | VMID column of `pct list` | +| `marked` | bool | col `user.remo` == `true` | `remo ∈` tags from `/etc/pve/lxc/.conf` | + +**Sync selection rule**: +- Default (`all=False`): register iff `marked` is true. Collect names of + `marked == False` into a `skipped` list for the hint. +- `--all` (`all=True`): register every discovered container; collect names of + `marked == False` into an `adopted_unmarked` list for the summary. + +## Entity: KnownHost (existing — unchanged) + +The registry line format is untouched (FR-012). For reference, marker state is +**not** a field here: +- Incus: `name = "/"`, `instance_id = `. +- Proxmox: `name = "/"`, `instance_id = `, `region = + `. + +## Relationships + +``` +KnownHost (registry, connection-only) + │ 1:1 by name + ▼ +Container (provider-side) + │ carries 0..1 + ▼ +Managed Marker ── authoritative on provider; drives sync inclusion +``` diff --git a/specs/013-managed-instance-tags/plan.md b/specs/013-managed-instance-tags/plan.md new file mode 100644 index 00000000..03f108a3 --- /dev/null +++ b/specs/013-managed-instance-tags/plan.md @@ -0,0 +1,131 @@ +# Implementation Plan: Managed-Instance Tagging & Filtered Sync (Incus / Proxmox) + +**Branch**: `013-managed-instance-tags` | **Date**: 2026-07-22 | **Spec**: [spec.md](./spec.md) + +**Input**: Feature specification from `/specs/013-managed-instance-tags/spec.md` + +## Summary + +Mark `remo`-created Incus and Proxmox containers with a provider-native managed +marker at provision time, and make `sync` filter on that marker by default so it +stops importing every unrelated container on a hypervisor host. This brings the +two hypervisor providers in line with AWS (`tag:remo=true`) and Hetzner +(`label_selector=remo`), which already filter on a native marker. + +Technical approach: apply and read the marker **host-side in the Python provider +layer** (`providers/incus.py`, `providers/proxmox.py`), reusing the existing +per-host SSH helpers (`_ssh_run_on_incus_host`, `_ssh_run`). This is required +because `update`'s dev-tools playbook connects to the *container's* IP, not the +hypervisor host — only the Python layer holds a host/node connection at both +`create` and `update` time. The marker literal is a fixed built-in constant +(clarified), so a single shared definition in `core/config.py` keeps Incus and +Proxmox consistent. `sync` gains an `--all` flag that restores today's unfiltered +behavior; the default path filters on the marker and prints an actionable hint +(naming skipped containers) when it skips anything. + +## Technical Context + +**Language/Version**: Python 3.11+ (existing `remo_cli` src-layout package) + +**Primary Dependencies**: Click (CLI), stdlib `subprocess`/`shlex` for host +commands over SSH. No new runtime dependencies. Provider marker mechanics use +the native `incus` and `pct`/`pvesh` CLIs already invoked over SSH. + +**Storage**: Marker is authoritative on the **provider side** (Incus `user.*` +config key; Proxmox guest tag). The local flat-file registry +(`~/.config/remo/known_hosts`) is unchanged — it does not record marker state +(explicitly out of scope). + +**Testing**: pytest with `mocker.patch` of the provider SSH helpers +(`tests/unit/providers/`, `tests/unit/cli/providers/`), mirroring the existing +snapshot test suites. No live hypervisor required. + +**Target Platform**: Linux workstation running the `remo` CLI against Incus +(localhost or remote) and Proxmox nodes over SSH. + +**Project Type**: Single-project Python CLI (three-layer: cli/ → providers/ → +core/). + +**Performance Goals**: Marker detection during `sync` MUST stay bounded to a +small, constant number of host queries (FR-013) — no per-container round-trip +that scales with unrelated containers. + +**Constraints**: Marker application MUST be idempotent (FR-002) and MUST +preserve pre-existing Proxmox tags (FR-003). `sync` MUST remain read-only on +container state (FR-010). AWS/Hetzner behavior MUST NOT change (FR-011). + +**Scale/Scope**: Two providers, two commands touched each (`create`, `update`, +`sync`), plus one shared constant and a hint helper. Tens of containers per host +is the realistic ceiling. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +The constitution is Ansible-centric; this feature is implemented in the Python +provider layer and touches **no Ansible**. The principles still map cleanly: + +| Principle | Applies? | How this plan satisfies it | +|-----------|----------|----------------------------| +| I. Defensive Variable Access (Ansible) | N/A | No Ansible tasks added or changed; marker logic lives in Python. | +| II. Test All Conditional Paths | ✅ | Tests cover marked/unmarked/mixed hosts, default vs `--all`, idempotent re-apply, and marker-apply failure (FR-005). Both branches of every new `if all:`/`if marked:` path exercised. | +| III. Idempotent by Default | ✅ | `create`/`update` re-apply is a no-op (Incus: set-same-value; Proxmox: skip write when `remo` already in tag set). Verified by SC-005 (config identical apart from marker). | +| IV. Fail Fast with Clear Messages | ✅ | Marker-apply failure warns with actionable text (FR-005); filtered `sync` prints a named hint with both remedies (FR-008). | +| V. Documentation Reflects Reality | ✅ | README `sync` sections (lines ~236–268, ~354–357) updated to state Incus/Proxmox now filter by default and document `--all`. | + +**Gate result: PASS** — no violations, Complexity Tracking not required. + +## Project Structure + +### Documentation (this feature) + +```text +specs/013-managed-instance-tags/ +├── plan.md # This file +├── research.md # Phase 0 output — marker mechanics decisions +├── data-model.md # Phase 1 output — marker + entities +├── quickstart.md # Phase 1 output — validation scenarios +├── contracts/ # Phase 1 output +│ ├── cli-sync.md # `--all` flag + sync output contract +│ └── marker-commands.md # host-command contract per provider +└── tasks.md # /speckit-tasks output (NOT created here) +``` + +### Source Code (repository root) + +```text +src/remo_cli/ +├── core/ +│ └── config.py # + fixed marker constants (single source) +├── providers/ +│ ├── incus.py # + _apply_managed_marker(), marker-aware +│ │ # listing; create()/update() apply marker; +│ │ # sync(all=False) filters +│ └── proxmox.py # + _apply_managed_marker() (tag union), +│ # bulk tag read; create()/update() apply; +│ # sync(all=False) filters +├── cli/providers/ +│ ├── incus.py # + `--all` flag on `sync` +│ └── proxmox.py # + `--all` flag on `sync` +└── core/ + └── output.py # (reuse) print_info/print_warning for hint + +tests/unit/ +├── providers/ +│ ├── test_incus_marker.py # new: apply/idempotency/filtered sync +│ └── test_proxmox_marker.py # new: tag union/preserve/filtered sync +└── cli/providers/ + ├── test_incus_sync_all.py # new: `--all` flag wiring + hint output + └── test_proxmox_sync_all.py # new: `--all` flag wiring + hint output + +README.md # sync docs updated (Principle V) +``` + +**Structure Decision**: Single-project Python CLI, existing three-layer +architecture. Marker business logic goes in `providers/`, the fixed constant in +`core/config.py`, and only the thin `--all` flag lands in `cli/providers/`. No +new modules or Ansible roles are introduced. + +## Complexity Tracking + +> No constitution violations — section intentionally empty. diff --git a/specs/013-managed-instance-tags/quickstart.md b/specs/013-managed-instance-tags/quickstart.md new file mode 100644 index 00000000..dd601ef2 --- /dev/null +++ b/specs/013-managed-instance-tags/quickstart.md @@ -0,0 +1,93 @@ +# Quickstart & Validation: Managed-Instance Tagging & Filtered Sync + +Runnable validation for the feature. Assumes a working `remo` dev env +(`uv sync --all-extras`) and access to either an Incus host (localhost is fine) +or a Proxmox node over SSH with at least one hand-created (non-remo) container. + +Details live in the design docs — see [contracts/cli-sync.md](./contracts/cli-sync.md), +[contracts/marker-commands.md](./contracts/marker-commands.md), and +[data-model.md](./data-model.md). + +## Automated checks (no hypervisor needed) + +```bash +uv run pytest tests/unit/providers/test_incus_marker.py \ + tests/unit/providers/test_proxmox_marker.py \ + tests/unit/cli/providers/test_incus_sync_all.py \ + tests/unit/cli/providers/test_proxmox_sync_all.py +uv run mypy src/remo_cli +uv run ruff check src/remo_cli +``` + +These mock the SSH helpers (as the existing snapshot suites do) and assert: +marker apply is idempotent, Proxmox tag union preserves existing tags, default +sync filters, `--all` registers everything, and the hint/summary text matches +the CLI contract. + +## Scenario 1 — filtered sync only pulls remo containers (US1, SC-001) + +```bash +# Incus host with one remo container + one hand-made container: +remo incus create --name dev1 --host # applies user.remo=true +incus launch images:ubuntu/24.04 plex # unrelated, unmarked + +remo incus sync --host +# Expect: "Synced 1 container(s)…" and a hint naming 'plex' as skipped, +# with the --all and `remo incus update` remedies. +remo incus list # dev1 present; plex absent +``` + +## Scenario 2 — create is picked up by the next sync (SC-002) + +```bash +remo proxmox create --name dev1 --host +remo proxmox sync --host +remo proxmox list # dev1 present with no extra action +``` + +## Scenario 3 — `--all` adopts everything, with a clear summary (US2, SC-003) + +```bash +remo proxmox sync --host --all +# Expect: every container registered; if any were unmarked, a summary line +# distinguishing the unmarked/adopted count + round-trip warning. +``` + +## Scenario 4 — backfill via update (US3, SC-004) + +```bash +# A container remo made before this feature (no marker): +remo incus sync --host # it is skipped (unmarked) — see hint +remo incus update --name dev1 --host # applies the marker +remo incus sync --host # now dev1 is registered +``` + +## Scenario 5 — idempotent re-apply preserves Proxmox tags (SC-005) + +```bash +# Give a container a user tag first: +ssh 'pct set --tags mytag' +remo proxmox update --name dev1 --host # adds 'remo', keeps 'mytag' +ssh 'pct config | grep ^tags:' # tags: mytag;remo +remo proxmox update --name dev1 --host # re-run: no change +ssh 'pct config | grep ^tags:' # still: mytag;remo (no reorder) +``` + +## Scenario 6 — upgrade hint is unmissable (SC-006) + +```bash +# On a host of pre-existing unmarked remo containers, a first default sync: +remo incus sync --host +# Expect: registers nothing, and the hint names BOTH remedies +# (--all and `remo incus update `). No silent registry wipe, +# no silent re-marking. +``` + +## Pass criteria + +- Default `sync` registers exactly the marked containers; the skip hint names + the skipped containers and both remedies. +- `--all` reproduces pre-feature behavior and flags adopted-unmarked counts. +- `update` marks pre-existing containers; re-apply is a no-op that preserves + existing Proxmox tags. +- No change to AWS/Hetzner sync, the registry format, or the connect path. diff --git a/specs/013-managed-instance-tags/research.md b/specs/013-managed-instance-tags/research.md new file mode 100644 index 00000000..6156c0eb --- /dev/null +++ b/specs/013-managed-instance-tags/research.md @@ -0,0 +1,132 @@ +# Phase 0 Research: Managed-Instance Tagging & Filtered Sync + +All Technical Context items are resolved — no `NEEDS CLARIFICATION` remains. +The three clarifications from `/speckit-clarify` (hint names skipped containers; +lifecycle commands stay uniform / no marker guard; marker is a fixed constant) +are treated as settled inputs. + +## Decision 1 — Where the marker is applied (host-side, Python layer) + +**Decision**: Apply and read the marker in the Python provider layer +(`providers/incus.py`, `providers/proxmox.py`), reusing the existing per-host +SSH helpers `_ssh_run_on_incus_host(host, user, cmd)` and `_ssh_run(host, user, +cmd)`. Not in Ansible. + +**Rationale**: +- `update` MUST apply the marker (FR-004, the backfill path), but `update`'s + `*_configure.yml` playbook connects to the **container's IP**, not the + hypervisor host — it has no host connection to run `incus config set` / `pct + set`. The Python provider layer is the only place that holds a host/node SSH + context at *both* `create` and `update` time (it already uses it for + `_resolve_vmid`, `_resolve_container_ip`, and all snapshot operations). +- Keeping the marker literal in Python (`core/config.py`) gives the single, + fixed, cross-provider source of truth the clarification requires, instead of + threading a value through Ansible extra-vars. +- Localhost Incus is handled transparently: `_ssh_run_on_incus_host` already + runs `bash -c ` when `host == "localhost"` (Edge Case: localhost parity). + +**Alternatives considered**: +- *Apply in the Ansible create role* (`incus config set ... user.remo=true`, + `pct set ... --tags`): works for `create` but not for `update` (no host + connection there), so it would need a second mechanism anyway. Rejected — + duplicates the marker literal across Ansible + Python and splits the logic. +- *Record marker state in the local registry*: explicitly out of scope; the + registry stays connection-only and the provider side stays authoritative. + +## Decision 2 — Incus marker: `user.remo=true` config key, single-query listing + +**Decision**: Marker is the config key `user.remo` with value `true`. +- **Apply** (`create` + `update`): `incus config set user.remo=true`. +- **List for sync**: one query `incus list -f csv -c n,user.remo` yields + `,` rows. A row whose second column is `true` is marked; + empty is unmarked. This single bulk query serves BOTH the default filtered + path (keep rows where marker == `true`) and the `--all` path (register all + rows, and count rows where marker != `true` for the FR-009 summary). + +**Rationale**: +- `user.*` is the Incus namespace reserved for user metadata — cannot collide + with Incus's own keys (Edge Case: config key collision). +- Setting an already-present identical key is a true no-op — idempotent by + construction (FR-002), satisfying SC-005 without a read-before-write. +- One column-augmented list call satisfies FR-013 (bounded, single query, no + per-container round-trip) and gives the `--all` summary its unmarked count for + free. Incus/LXD `list -c` accepts arbitrary config keys as columns by name. + +**Alternatives considered**: +- *Server-side filter* `incus list user.remo=true -f csv -c n`: clean for the + default path, but `--all` still needs the full list AND the marked set to + compute the unmarked count → two queries. The column form gets both from one. + Kept as a documented fallback if the column form misbehaves on an older Incus. +- *A `user.remo.*` sub-namespace or JSON blob*: over-engineered; a single + boolean-valued key matches the AWS `remo=true` analog exactly. + +## Decision 3 — Proxmox marker: `remo` guest tag, bulk conf read, tag-set union + +**Decision**: Marker is the guest tag `remo` (a bare tag, matching Proxmox's +set-of-strings tag model). +- **Apply** (`create` + `update`): read the current tag set from `pct config + ` (the `tags:` line), and if `remo` is absent, write the **union** back + with `pct set --tags ""`. If `remo` is already + present, skip the write entirely (guaranteed no reorder / no-op). +- **List for sync**: keep the existing `pct list` for the vmid/name inventory, + and add ONE bulk tag read: `grep -H '^tags:' /etc/pve/lxc/*.conf`, which maps + each `.conf` to its tag line in a single SSH round-trip. Containers with + no `tags:` line are simply absent from the map → treated as unmarked. Marked = + `remo` ∈ tag set for that vmid. + +**Rationale**: +- Proxmox tags are a set; applying the marker as a union preserves all + pre-existing user tags and never removes/reorders them (FR-003, Edge Case: + container already carries user tags). Skipping the write when `remo` is present + keeps re-application a strict no-op (FR-002 / SC-005). +- Reading tags from `/etc/pve/lxc/*.conf` in one `grep` is bulk and bounded + (FR-013) and is consistent with the existing snapshot code, which already + reads `/etc/pve/lxc/.conf` over SSH. No per-container `pct config` loop. +- Tag separator: Proxmox stores tags separated by `;` (and accepts `;`, `,`, or + space on input). We split on `[;, ]+` when reading and join with `;` on write. + +**Alternatives considered**: +- *`pvesh get /nodes//lxc --output-format json`* (includes `tags` per CT + in one call): also bulk, but requires the cluster node name and reworks the + established `pct list` + conf-file parsing the provider already relies on. + Rejected as a larger, higher-risk change for no functional gain. +- *Per-container `pct config | grep tags`*: violates the spirit of + FR-013 (round-trips scale with unrelated containers). Rejected. + +## Decision 4 — Fixed marker constants live in `core/config.py` + +**Decision**: Add three module-level constants to `src/remo_cli/core/config.py`: +`INCUS_MANAGED_CONFIG_KEY = "user.remo"`, `INCUS_MANAGED_CONFIG_VALUE = "true"`, +`PROXMOX_MANAGED_TAG = "remo"`. Both providers import from here. + +**Rationale**: `core/config.py` is the existing shared, provider-agnostic config +module. A single definition site enforces the "stable, namespaced, consistent +across both providers" requirement and the clarified "fixed, not user- +configurable" decision. No env var or option reads override them. + +**Alternatives considered**: a new `core/managed_marker.py` module — unnecessary +for three constants and two tiny helpers that already live with their provider. + +## Decision 5 — `--all` flag surface and the skip hint + +**Decision**: +- Add `--all` as a Click `is_flag` option on `remo incus sync` and `remo proxmox + sync`, threaded to `providers.*.sync(all=)`. +- Default (filtered) `sync` that skips ≥1 unmarked container prints, via + `print_info`/`print_warning`, a hint that **names the skipped containers**, + states the count, and lists both remedies: `--all` (one-time adoption) and + `remo update ` (permanent mark). (FR-008, clarified to + include names.) +- `--all` that registers ≥1 unmarked container prints a summary distinguishing + the unmarked count and warning about the round-trip drop on the next default + sync (FR-009 + Edge Case: mixed-marker host with `--all`). + +**Rationale**: `--all` mirrors the existing boolean flag ergonomics used across +the CLI (`--use-ip`, `--yes`). Naming skipped containers makes the `update +` remedy directly actionable (clarification 1). Lifecycle commands are +**not** touched — per clarification 2 they operate uniformly on any registry +entry with no marker guard. + +**Alternatives considered**: a positive `--managed-only` default-on flag pair — +rejected; `--all` as an explicit opt-out reads better and matches the spec's +language exactly. diff --git a/specs/013-managed-instance-tags/spec.md b/specs/013-managed-instance-tags/spec.md new file mode 100644 index 00000000..7f9abae3 --- /dev/null +++ b/specs/013-managed-instance-tags/spec.md @@ -0,0 +1,309 @@ +# Feature Specification: Managed-Instance Tagging & Filtered Sync (Incus / Proxmox) + +**Feature Branch**: `013-managed-instance-tags` +**Created**: 2026-07-22 +**Status**: Draft +**Input**: User description: "Tag Incus/Proxmox containers at provision time so `sync` only pulls in remo-managed instances, matching the cloud providers (AWS `tag:remo=true`, Hetzner `label_selector=remo`). Provide an escape hatch to adopt everything on a host." + +## Problem & Motivation + +`remo` has four providers whose `sync` command reconciles the local known-hosts +registry with the instances that actually exist. Today they split into two +mental models: + +- **Cloud providers (AWS, Hetzner)** apply a provider-native marker at create + time (`remo=true` tag on AWS, `remo` label on Hetzner) and `sync` filters on + it. A user's unrelated EC2 instances or Hetzner servers are never touched. +- **Hypervisor providers (Incus, Proxmox)** apply no marker. `sync` runs + `incus list` / `pct list` over SSH to the host and registers **every** + container on the box — including containers the user created by hand, or that + belong to unrelated workloads (a Home Assistant LXC, a Plex container, etc.). + +This asymmetry means the same verb (`sync`) means "reconcile my remo instances" +on two providers and "import literally everything on this host" on the other +two. This feature closes that gap: `remo`-created Incus/Proxmox containers are +marked at provision time, and `sync` filters on that marker by default, with an +explicit `--all` opt-out for users who deliberately want to adopt every +container on a host. + +## Clarifications + +### Session 2026-07-22 + +- Q: When a default `sync` skips unmarked containers, what should the hint include? → A: Names + count + remedies (list the skipped container names alongside the count and both remedies). +- Q: Should this feature guard lifecycle commands (`destroy`/`snapshot`/resize) against `--all`-adopted, unmarked containers? → A: Out of scope — lifecycle commands operate uniformly on any registry entry; no marker check is added. +- Q: Should the managed marker key/value be fixed or configurable? → A: Fixed built-in constant (not user-configurable), matching AWS/Hetzner marker behavior. + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 - Sync only pulls in remo-managed containers (Priority: P1) + +A developer runs `remo` on a Proxmox node (or Incus host) that also hosts +unrelated LXC containers — a media server, a home-automation box, a database +they run by hand. They run `remo proxmox sync ` and expect their registry +to contain only the dev containers `remo` created for them, not the entire +inventory of the node. + +**Why this priority**: This is the core defect being fixed and the behavior that +brings Incus/Proxmox in line with AWS/Hetzner. Without it, `sync` pollutes the +registry (and the `remo shell` picker) with containers `remo` can neither manage +nor safely connect to, and gives the two hypervisor providers a different mental +model from the two cloud providers. + +**Independent Test**: On a host with one remo-created container and one +hand-created container, run `sync` and confirm only the remo-created container +is registered. + +**Acceptance Scenarios**: + +1. **Given** an Incus/Proxmox host with a mix of remo-created and non-remo + containers, **When** the user runs `sync` with no flags, **Then** only the + remo-created (marker-bearing) containers are registered, and the summary line + reports the count of registered containers. +2. **Given** a container that `remo` created (via `create`), **When** the user + runs `sync`, **Then** that container is always registered because `create` + applied the managed marker. +3. **Given** a host on which every container is remo-created, **When** the user + runs `sync`, **Then** the result is identical to today's behavior (all of + them registered). +4. **Given** a host on which no container carries the marker, **When** the user + runs `sync` with no flags, **Then** zero containers are registered and the + command prints a hint naming the skipped untagged containers and how to + include or adopt them (see US2, US3). + +--- + +### User Story 2 - Adopt every container on a host with `--all` (Priority: P2) + +A developer is standing up `remo` against an existing Proxmox node whose dev +containers were created before this feature (or by another tool), and they +deliberately want to register all of them regardless of marker. They run +`sync --all` and get today's unfiltered behavior. + +**Why this priority**: Preserves the existing capability for the "this whole box +is mine, import all of it" case and provides the migration path for containers +that predate the marker. Lower than P1 because it is an explicit opt-in, not the +default. + +**Independent Test**: On a host with only unmarked containers, run `sync --all` +and confirm all of them are registered. + +**Acceptance Scenarios**: + +1. **Given** a host with a mix of marked and unmarked containers, **When** the + user runs `sync --all`, **Then** every container on the host is registered, + regardless of marker. +2. **Given** `--all` is used, **When** `sync` completes, **Then** the summary + distinguishes how many of the registered containers were unmarked, so the + user understands they adopted containers `remo` did not create. + +--- + +### User Story 3 - Backfill the marker onto pre-existing remo containers (Priority: P2) + +A developer upgrades `remo` to a version with this feature. Their existing +remo-created containers have no marker yet, so the new default `sync` would drop +them from the registry. They want a low-friction way to bring those containers +into the managed set so future filtered syncs see them. + +**Why this priority**: Without a backfill path, upgrading is a regression: the +first `sync` after upgrade silently empties the registry of real remo +containers. This story makes the upgrade non-destructive. + +**Independent Test**: Take a container that `remo` created before this feature +(no marker), run the backfill path, then run a default `sync` and confirm the +container is now registered. + +**Acceptance Scenarios**: + +1. **Given** an existing remo container with no marker, **When** the user runs + `remo update `, **Then** the managed marker is applied + (idempotently) as part of update, and a subsequent default `sync` registers + the container. +2. **Given** an existing remo container with no marker, **When** the user runs + `sync --all`, **Then** the container is registered for that run even though it + lacks the marker (adoption without mutating the container). +3. **Given** a default `sync` skips one or more unmarked containers, **When** the + command finishes, **Then** it prints a hint naming the skipped containers and + their count, and the two ways to include them (`--all` for a one-time + adoption, or `remo update ` to mark one permanently). + +--- + +### Edge Cases + +- **Proxmox container already carries user tags**: Proxmox guest tags are a set. + Applying the `remo` marker MUST preserve any existing tags on the container and + MUST NOT remove or reorder the user's own tags. Re-applying when the marker is + already present is a no-op. +- **Incus config key collision**: The Incus marker is a `user.*` config key, + which lives in a namespace reserved for user metadata and cannot collide with + Incus's own keys. If the key already exists with the expected value, applying + it again is a no-op. +- **Marker present but container is stopped**: The marker is stored in container + configuration, not runtime state, so a stopped container is still discovered by + a filtered `sync` (subject to whatever address-resolution limits already apply + to stopped containers today — unchanged by this feature). +- **User manually removes the marker**: If a user strips the marker off a + remo-created container, a default `sync` will no longer see it. This is treated + as an intentional "unmanage this container" action; `remo` does not fight the + user by re-adding markers during `sync` (which is read-only — see FR-010). +- **Mixed-marker host with `--all`**: When `--all` adopts unmarked containers, + those registry entries are indistinguishable from marked ones once written + (the registry does not record marker state). Re-running a default `sync` later + will drop the unmarked ones again. The `--all` summary MUST make this + round-trip behavior clear enough that the user is not surprised. +- **`create` on a container name that already exists**: `create` is already + idempotent (re-runs configure without re-creating). It MUST ensure the marker + is present on that pre-existing container as part of the run, so a container + first made by `remo` before this feature becomes marked the next time `create` + touches it. +- **Localhost Incus**: The Incus provider supports `host == "localhost"` + (running `incus list` directly). Marker application and filtering MUST behave + identically for localhost and remote hosts. + +## Requirements *(mandatory)* + +### Functional Requirements + +**Marking at provision time** + +- **FR-001**: When `remo` creates an Incus or Proxmox container, it MUST apply a + provider-native managed marker to that container as part of the create flow. + On Proxmox the marker MUST be a guest tag; on Incus the marker MUST be a + `user.*` configuration key. +- **FR-002**: Marker application MUST be idempotent: applying it to a container + that already carries it MUST succeed as a no-op and MUST NOT alter any other + container configuration. +- **FR-003**: On Proxmox, applying the marker MUST preserve all pre-existing + guest tags on the container. The marker is added to the tag set; no existing + tag may be removed or altered. +- **FR-004**: `remo update ` MUST ensure the managed marker is + present on the target container (applying it if absent), so that `update` + doubles as the backfill path for containers created before this feature. +- **FR-005**: Marker application failure during `create`/`update` MUST be + surfaced to the user but MUST NOT, on its own, fail the overall command if the + container was otherwise created/configured successfully; the command MUST warn + that the container is unmarked and will require `--all` or a re-run of `update` + to be picked up by a default `sync`. + +**Filtered sync** + +- **FR-006**: By default (no `--all` flag), `remo sync` on Incus and + Proxmox MUST register only containers that carry the managed marker. Unmarked + containers MUST NOT be registered. +- **FR-007**: `sync` MUST accept an `--all` flag that disables marker filtering + and registers every container discovered on the host — the pre-feature + behavior. +- **FR-008**: When a default (filtered) `sync` skips one or more unmarked + containers, the command MUST print an informational hint stating how many were + skipped, **naming the skipped containers**, and how to include them: `--all` + for a one-time adoption, or `remo update ` to permanently mark + one. Naming the skipped containers makes the `update ` remedy directly + actionable without a separate lookup. +- **FR-009**: When `--all` is used and one or more registered containers were + unmarked, the `sync` summary MUST distinguish the unmarked count so the user + understands they adopted containers `remo` did not create. +- **FR-010**: `sync` MUST remain read-only with respect to remote container + state: it MUST NOT apply, remove, or modify markers on any container. Marker + mutation happens only through `create` and `update`. + +**Cross-cutting** + +- **FR-011**: This feature MUST NOT change how AWS and Hetzner `sync` behave; + they already filter on their native markers. The goal is parity, achieved by + bringing Incus/Proxmox up to the cloud model, not by altering the cloud + providers. +- **FR-012**: This feature MUST NOT change the connection path (`remo shell`, + `remo cp`) or the registry line format for a container. The marker lives on + the provider side (container config), not in the local registry. +- **FR-013**: Marker presence MUST be detectable by `sync` in a single, + bounded set of host queries (no per-container extra round-trip that scales the + sync time linearly with unrelated containers where the provider offers a bulk + query). Where a bulk marker-aware listing is available it SHOULD be preferred. + +### Out of Scope + +- **Changing AWS/Hetzner marking**: their `remo` tag/label mechanism is + unchanged. +- **A generic "adopt this one existing container into the managed set" verb** + that both registers *and* marks in one step. Backfill is via `update` (marks) + or `--all` (registers without marking). A dedicated adopt verb may be + considered separately. +- **Registering an SSH-reachable container without host access** — that is the + subject of the companion feature `014-register-ssh-host` and is explicitly not + addressed here (this feature still requires host/node access, because marking + and discovery both run against the hypervisor). +- **Recording marker state in the local registry.** The registry continues to + store only what is needed to connect; marker state is authoritative on the + provider side. +- **Auto-migration** that silently re-marks or re-registers all pre-existing + containers on first upgrade. Migration is user-initiated (`update` or + `--all`), with a hint to guide it. +- **Guarding lifecycle commands against `--all`-adopted containers.** `destroy`, + `snapshot`, and resize operate on any registry entry uniformly; this feature + adds no marker-based refusal or warning to them. + +### Key Entities + +- **Managed marker**: A provider-native piece of container metadata indicating + a container was created and is managed by `remo`. On Proxmox it is a guest tag; + on Incus it is a `user.*` config key. It is a fixed built-in constant (not + user-configurable) and the hypervisor analog of the AWS `remo=true` tag and the + Hetzner `remo` label. +- **Container** (existing): An Incus/Proxmox instance, already modeled as a + `KnownHost` in the registry (name in `host/container` form, VMID in + `instance_id` for Proxmox). This feature adds a marker on the provider side but + does not change the registry representation. + +### Assumptions + +- Proxmox guest tags (`pct set --tags …`, visible in `pct config`) are + available on the target Proxmox versions `remo` already supports. The Proxmox + provider already requires SSH access to the node, so applying and reading tags + needs no new capability. +- Incus `user.*` config keys (`incus config set user.remo=true`, filterable + via `incus list user.remo=true`) are available on the Incus versions `remo` + already supports. +- The marker is a **fixed built-in constant, not user-configurable** (matching + how the AWS `remo=true` tag and Hetzner `remo` label are hard-coded today). The + exact literal key/value (e.g. tag `remo`, config key `user.remo=true`) is a + design decision for the plan; the requirement is that it is stable, namespaced + to avoid collisions, and consistent across the two providers' conceptual + models. No env var or config option overrides it. +- Users who deliberately run `sync --all` on a shared host understand they are + registering containers `remo` did not create. Lifecycle commands (`destroy`, + `snapshot`, resize) operate **uniformly on any registry entry** and do NOT + consult the marker: this feature adds no marker check to those commands. The + expectation that lifecycle actions stay "scoped to what `remo` created" is a + usage convention (the user chose to adopt those entries via `--all`), not an + enforced guard — enforcing it would require recording marker state in the + registry, which is explicitly out of scope. +- The upgrade path is acceptable as "first default `sync` after upgrade shows a + hint and registers nothing until the user runs `update` or `--all`" — a loud, + reversible no-op is preferred over silently emptying the registry or silently + re-marking every container on the box. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: On a host containing exactly one remo-created container and any + number of unrelated containers, a default `sync` registers exactly one entry — + on 100% of attempts across both Incus and Proxmox. +- **SC-002**: A container created via `remo create` is registered by + the very next default `sync` with no additional user action — on 100% of + attempts. +- **SC-003**: Running `sync --all` on the same host registers the same set of + containers that the pre-feature `sync` registered — i.e. no regression for the + "adopt everything" workflow. +- **SC-004**: After running `remo update ` on a pre-existing + unmarked container, that container appears in the next default `sync` — on + 100% of attempts, demonstrating the backfill path. +- **SC-005**: Applying the marker twice (e.g. `create` then `update`, or two + `update`s) leaves the container configuration identical apart from the single + marker, with all pre-existing Proxmox tags intact — verified by comparing + container config before and after. +- **SC-006**: A user who upgrades and runs a default `sync` on a host of + pre-existing unmarked containers is never left guessing: the command's hint + names both the `--all` and `update` remedies — on 100% of such runs. diff --git a/specs/013-managed-instance-tags/tasks.md b/specs/013-managed-instance-tags/tasks.md new file mode 100644 index 00000000..12088f0c --- /dev/null +++ b/specs/013-managed-instance-tags/tasks.md @@ -0,0 +1,198 @@ +--- +description: "Task list for Managed-Instance Tagging & Filtered Sync (Incus / Proxmox)" +--- + +# Tasks: Managed-Instance Tagging & Filtered Sync (Incus / Proxmox) + +**Input**: Design documents from `/specs/013-managed-instance-tags/` + +**Prerequisites**: plan.md, spec.md, research.md, data-model.md, contracts/, quickstart.md + +**Tests**: INCLUDED — required by Constitution Principle II (Test All Conditional +Paths), the spec's Success Criteria, and the quickstart's named test files. Tests +are written to mock the provider SSH helpers (no live hypervisor needed), mirroring +the existing snapshot suites. + +**Organization**: Tasks are grouped by user story (US1 P1, US2 P2, US3 P2) for +independent implementation and testing. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Can run in parallel (different files, no dependencies on incomplete tasks) +- **[Story]**: US1 / US2 / US3 (setup, foundational, polish have no story label) + +## Path Conventions + +Single-project Python CLI: `src/remo_cli/`, `tests/` at repository root (per plan.md). + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Confirm the working baseline. This feature adds no new runtime deps. + +- [X] T001 Verify dev env with `uv sync --all-extras`, confirm no new runtime dependency is required (per plan.md), and establish a green baseline with `uv run pytest -q`. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: The fixed marker constant and the shared apply helper that BOTH `create` (US1) and `update` (US3) depend on. + +**⚠️ CRITICAL**: No user story work can begin until this phase is complete. + +- [X] T002 Add fixed marker constants `INCUS_MANAGED_CONFIG_KEY = "user.remo"`, `INCUS_MANAGED_CONFIG_VALUE = "true"`, and `PROXMOX_MANAGED_TAG = "remo"` (single source, not user-configurable) to src/remo_cli/core/config.py. +- [X] T003 [P] Implement `_apply_managed_marker(host, user, name)` in src/remo_cli/providers/incus.py — runs `incus config set user.remo=true` via `_ssh_run_on_incus_host` (handles localhost + remote); idempotent no-op when already set; returns `(ok, err)` for FR-005 warn-not-fail. Depends on T002. +- [X] T004 [P] Implement `_apply_managed_marker(host, user, vmid)` in src/remo_cli/providers/proxmox.py — reads the `tags:` line from `pct config `, and only when `remo` is absent writes the union `pct set --tags ""` (split on `[;, ]+`, join with `;`, preserve order); strict no-op when `remo` present; returns `(ok, err)`. Depends on T002. + +**Checkpoint**: Marker constant + apply helpers ready — user stories can begin. + +--- + +## Phase 3: User Story 1 - Sync only pulls in remo-managed containers (Priority: P1) 🎯 MVP + +**Goal**: Default `remo sync` registers only marker-bearing containers; `create` marks what it makes; a skip hint names anything skipped. + +**Independent Test**: On a host with one remo-created and one hand-created container, run `sync` (no flags) → only the remo-created one is registered, and the hint names the skipped one plus both remedies. + +### Tests for User Story 1 + +- [X] T005 [P] [US1] Write tests in tests/unit/providers/test_incus_marker.py covering: `create()` calls the apply helper; apply is idempotent; the marker-aware read helper parses `incus list -f csv -c n,user.remo`; default `sync(all=False)` registers only marked containers; the skip hint names skipped containers + count + both remedies; localhost parity. Also assert **FR-010** (`sync` issues NO `incus config set`/apply call — read-only) and **FR-013** (`sync` makes a bounded number of host queries — a single `incus list`, with no per-container round-trip). Mock `_ssh_run_on_incus_host` / `subprocess.run`. +- [X] T006 [P] [US1] Write tests in tests/unit/providers/test_proxmox_marker.py covering: `create()` calls the apply helper; tag union preserves pre-existing tags; the bulk tag read (`grep -H '^tags:' /etc/pve/lxc/*.conf`) classifies marked vs unmarked; default `sync(all=False)` filters; skip hint names skipped + remedies. Also assert **FR-010** (`sync` issues NO `pct set`/apply call) and **FR-013** (`sync` uses one bulk `grep` tag read plus `pct list` — no per-container `pct config` loop; assert the SSH-call count does not scale with container count). Mock `_ssh_run` / `subprocess.run`. + +### Implementation for User Story 1 + +- [X] T007 [P] [US1] Add an Incus marker-aware read helper in src/remo_cli/providers/incus.py that runs `incus list -f csv -c n,user.remo` (localhost + remote) and returns `[(name, marked: bool)]` (marked = value == "true"). +- [X] T008 [P] [US1] Add a Proxmox bulk tag read helper in src/remo_cli/providers/proxmox.py that runs one `grep -H '^tags:' /etc/pve/lxc/*.conf` over SSH and returns `{vmid: set(tags)}`, plus a `remo ∈ tags` classifier (vmid absent ⇒ unmarked). +- [X] T009 [US1] Wire `create()` in src/remo_cli/providers/incus.py to call `_apply_managed_marker` after `rc == 0` (post `save_known_host`); on failure `print_warning` the unmarked-container guidance but do NOT change the command's rc (FR-005). Same file as T007 — sequential. +- [X] T010 [US1] Wire `create()` in src/remo_cli/providers/proxmox.py to call `_apply_managed_marker(host, user, vmid)` after `rc == 0`, using the `vmid` from `_resolve_vmid`; if that `vmid` is empty, `print_warning` that the container could not be marked (and how to backfill) rather than crashing; warn-not-fail on any apply failure (FR-005). Same file as T008 — sequential. +- [X] T011 [US1] Change `sync()` in src/remo_cli/providers/incus.py to accept `all: bool = False`; in the default path register only marked containers (via T007 helper), collect skipped names, and emit the named skip hint (count + `--all` + `remo incus update `) per contracts/cli-sync.md. Depends on T007. +- [X] T012 [US1] Change `sync()` in src/remo_cli/providers/proxmox.py to accept `all: bool = False`; default path registers only marked containers (via T008 helper), collects skipped names, and emits the named skip hint. Depends on T008. + +**Checkpoint**: Default sync now filters on both providers and `create` marks — US1 fully testable (SC-001, SC-002). + +--- + +## Phase 4: User Story 2 - Adopt every container on a host with `--all` (Priority: P2) + +**Goal**: `sync --all` restores pre-feature unfiltered behavior and, when it registers unmarked containers, distinguishes the adopted-unmarked count. + +**Independent Test**: On a host with only unmarked containers, `sync --all` registers all of them; the summary flags how many were not remo-created. + +### Tests for User Story 2 + +- [X] T013 [P] [US2] Write tests in tests/unit/cli/providers/test_incus_sync_all.py: the `--all` flag threads to `providers.incus.sync(all=True)`; all containers register; when ≥1 was unmarked the summary distinguishes the adopted-unmarked count and states the round-trip drop (FR-009). Use Click `CliRunner` + mocks. +- [X] T014 [P] [US2] Write tests in tests/unit/cli/providers/test_proxmox_sync_all.py: same `--all` behavior for Proxmox (registers all, adopted-unmarked summary). + +### Implementation for User Story 2 + +- [X] T015 [P] [US2] Add the `all=True` branch to `sync()` in src/remo_cli/providers/incus.py: register every discovered container, count those with `marked == False`, and emit the adopted-unmarked summary + round-trip warning (FR-007, FR-009). Depends on T011. +- [X] T016 [P] [US2] Add the `all=True` branch to `sync()` in src/remo_cli/providers/proxmox.py (register all, adopted summary). Depends on T012. +- [X] T017 [P] [US2] Add `--all` (`is_flag`) option to `remo incus sync` in src/remo_cli/cli/providers/incus.py and pass it to `providers_incus.sync(all=…)`. +- [X] T018 [P] [US2] Add `--all` (`is_flag`) option to `remo proxmox sync` in src/remo_cli/cli/providers/proxmox.py and pass it to `providers_proxmox.sync(all=…)`. + +**Checkpoint**: US1 filtered default AND US2 `--all` opt-out both work (SC-003). + +--- + +## Phase 5: User Story 3 - Backfill the marker via `update` (Priority: P2) + +**Goal**: `remo update ` applies the marker (idempotently), making it the low-friction backfill path for pre-existing remo containers. + +**Independent Test**: Take a pre-feature (unmarked) remo container, run `update`, then a default `sync` → the container is now registered. + +### Tests for User Story 3 + +- [X] T019 [P] [US3] Extend tests/unit/providers/test_incus_marker.py: `update()` applies the marker (backfill, FR-004); re-running `update` is a no-op (SC-005); a container whose marker was manually removed is skipped by default sync (edge case). Depends on T005. +- [X] T020 [P] [US3] Extend tests/unit/providers/test_proxmox_marker.py: `update()` applies the marker preserving existing tags; two `update`s leave `tags: mytag;remo` unchanged/unreordered (SC-005). Depends on T006. + +### Implementation for User Story 3 + +- [X] T021 [P] [US3] Wire `update()` in src/remo_cli/providers/incus.py to call `_apply_managed_marker` once `host`/`user` are resolved (before/around the configure playbook); warn-not-fail on failure (FR-004, FR-005). Depends on T003. +- [X] T022 [P] [US3] Wire `update()` in src/remo_cli/providers/proxmox.py to call `_apply_managed_marker(host, user, vmid)` once `host`/`user`/`vmid` are resolved; warn-not-fail (FR-004, FR-005). Depends on T004. + +**Checkpoint**: All three stories independently functional (SC-004, SC-005, SC-006). + +--- + +## Phase 6: Polish & Cross-Cutting Concerns + +- [X] T023 [P] Update README.md sync sections (lines ~236–268 and ~354–357) to state Incus/Proxmox `sync` now registers only remo-managed containers by default and document the `--all` opt-out and the `update` backfill path (Constitution Principle V). +- [X] T024 Run `uv run mypy src/remo_cli` and `uv run ruff check src/remo_cli`; resolve any findings introduced by this feature. +- [X] T025 Run the full `uv run pytest` and walk quickstart.md Scenarios 1–6 to confirm SC-001…SC-006. +- [X] T026 [P] Confirm FR-011 (no AWS/Hetzner change): existing AWS/Hetzner sync tests still pass and no marker logic leaked into src/remo_cli/providers/aws.py or hetzner.py. +- [X] T027 [P] Confirm FR-012 (registry & connect path unchanged): assert the `KnownHost` written by Incus/Proxmox `sync`/`create` keeps its existing fields with no marker column added (registry line format unchanged), and that `remo shell`/`remo cp` resolution is untouched — via existing registry/known-hosts tests plus a focused assertion in tests/unit/providers/test_incus_marker.py / test_proxmox_marker.py. + +--- + +## Dependencies & Execution Order + +### Phase Dependencies + +- **Setup (Phase 1)**: no dependencies. +- **Foundational (Phase 2)**: depends on Setup; T002 blocks T003/T004; BLOCKS all user stories. +- **User Stories (Phase 3–5)**: all depend on Foundational. + - US1 (P1) is the MVP and should land first (it flips the default behavior). + - US2 (P2) provider branches depend on US1's `sync()` (T015→T011, T016→T012); its CLI-flag and test tasks are otherwise independent. + - US3 (P2) depends only on the Foundational apply helpers (T021→T003, T022→T004); independent of US1/US2 code paths. +- **Polish (Phase 6)**: after the desired stories are complete. + +### User Story Dependencies + +- **US1 (P1)**: Foundational only. Delivers filtered default + create-marks + hint. +- **US2 (P2)**: extends US1's `sync()` for the `--all` branch; independently testable. +- **US3 (P2)**: Foundational only; parallelizable with US1/US2 (touches `update`, not `sync`). + +### Within Each User Story + +- Tests written to fail first, then implementation. +- Same-file tasks run sequentially (e.g., T007→T009→T011 all edit `providers/incus.py`). + +### Parallel Opportunities + +- Foundational: T003 ∥ T004 (different provider files). +- US1: T005 ∥ T006 (tests), T007 ∥ T008 (read helpers, different files). +- US2: T013 ∥ T014, and T015 ∥ T016 ∥ T017 ∥ T018 (four distinct files). +- US3: T019 ∥ T020, T021 ∥ T022. +- US1 and US3 can be developed in parallel by different people once Foundational is done. + +--- + +## Parallel Example: User Story 1 + +```bash +# Tests first (different files): +Task: "test_incus_marker.py — create-marks, filtered sync, hint" # T005 +Task: "test_proxmox_marker.py — tag union, filtered sync, hint" # T006 + +# Read helpers (different files): +Task: "Incus marker-aware list helper in providers/incus.py" # T007 +Task: "Proxmox bulk tag read helper in providers/proxmox.py" # T008 +``` + +--- + +## Implementation Strategy + +### MVP First (User Story 1 only) + +1. Phase 1 Setup → 2. Phase 2 Foundational (T002–T004) → 3. Phase 3 US1 (T005–T012). +4. **STOP and VALIDATE**: default `sync` filters and `create` marks (SC-001, SC-002). +5. Ship — this alone closes the core defect. + +### Incremental Delivery + +1. Foundational → US1 (MVP: filtered default) → 2. US2 (`--all` escape hatch) → + 3. US3 (`update` backfill) → 4. Polish (README, lint, quickstart). + Each story adds value without breaking the previous one. + +--- + +## Notes + +- The marker is applied HOST-SIDE in the Python provider layer (not Ansible), + because `update`'s configure playbook connects to the container IP, not the + hypervisor host — see plan.md / research.md Decision 1. +- `sync` stays read-only (FR-010): it never calls the apply helper. +- Registry line format and the `remo shell`/`cp` connect path are unchanged (FR-012). +- Lifecycle commands (`destroy`/`snapshot`/resize) get NO marker guard (clarification 2). diff --git a/src/remo_cli/cli/providers/incus.py b/src/remo_cli/cli/providers/incus.py index e4c0ab05..fbfe1605 100644 --- a/src/remo_cli/cli/providers/incus.py +++ b/src/remo_cli/cli/providers/incus.py @@ -192,9 +192,15 @@ def info(name: str, host: str, user: str) -> None: is_flag=True, help="Store each container's IP address in known_hosts instead of its name (for setups without DNS/MagicDNS).", ) -def sync(host: str, user: str, use_ip: bool) -> None: +@click.option( + "--all", + "include_all", + is_flag=True, + help="Register every container on the host, including those without the remo managed marker (pre-feature behavior).", +) +def sync(host: str, user: str, use_ip: bool, include_all: bool) -> None: """Discover containers from an Incus host.""" - providers_incus.sync(host=host, user=user, use_ip=use_ip) + providers_incus.sync(host=host, user=user, use_ip=use_ip, include_all=include_all) @incus.command() diff --git a/src/remo_cli/cli/providers/proxmox.py b/src/remo_cli/cli/providers/proxmox.py index 6e71baf6..58b2a7f3 100644 --- a/src/remo_cli/cli/providers/proxmox.py +++ b/src/remo_cli/cli/providers/proxmox.py @@ -219,9 +219,15 @@ def info(name: str, host: str, user: str) -> None: is_flag=True, help="Store each container's IP address in known_hosts instead of its name (for setups without DNS/MagicDNS).", ) -def sync(host: str, user: str, use_ip: bool) -> None: +@click.option( + "--all", + "include_all", + is_flag=True, + help="Register every container on the host, including those without the remo managed marker (pre-feature behavior).", +) +def sync(host: str, user: str, use_ip: bool, include_all: bool) -> None: """Discover containers from a Proxmox host.""" - providers_proxmox.sync(host=host, user=user, use_ip=use_ip) + providers_proxmox.sync(host=host, user=user, use_ip=use_ip, include_all=include_all) @proxmox.command() diff --git a/src/remo_cli/core/config.py b/src/remo_cli/core/config.py index cb46d54a..d3c44980 100644 --- a/src/remo_cli/core/config.py +++ b/src/remo_cli/core/config.py @@ -5,6 +5,19 @@ import os from pathlib import Path +# --------------------------------------------------------------------------- +# Managed-instance marker (feature 013-managed-instance-tags) +# +# Fixed, built-in constants — NOT user-configurable. These are the hypervisor +# analog of the AWS ``remo=true`` tag and the Hetzner ``remo`` label, applied to +# remo-created Incus/Proxmox containers so ``sync`` can filter on them by +# default. The single definition site keeps both providers consistent. +# --------------------------------------------------------------------------- + +INCUS_MANAGED_CONFIG_KEY = "user.remo" +INCUS_MANAGED_CONFIG_VALUE = "true" +PROXMOX_MANAGED_TAG = "remo" + def _resolve_remo_home() -> Path: """Resolve the remo config directory path with no filesystem side effects. diff --git a/src/remo_cli/providers/incus.py b/src/remo_cli/providers/incus.py index fcfbe657..308fab8e 100644 --- a/src/remo_cli/providers/incus.py +++ b/src/remo_cli/providers/incus.py @@ -15,6 +15,10 @@ from datetime import datetime, timezone from remo_cli.core.ansible_runner import run_playbook +from remo_cli.core.config import ( + INCUS_MANAGED_CONFIG_KEY, + INCUS_MANAGED_CONFIG_VALUE, +) from remo_cli.core.known_hosts import ( clear_known_hosts_by_prefix, get_known_hosts, @@ -124,6 +128,57 @@ def _extract_eth0_ip(incus_output: str) -> str: return "" +# --------------------------------------------------------------------------- +# Managed marker (feature 013-managed-instance-tags) +# --------------------------------------------------------------------------- + + +def _apply_managed_marker(host: str, user: str, name: str) -> tuple[bool, str]: + """Apply the remo managed marker to Incus container *name* (host-side). + + Runs ``incus config set user.remo=true`` on the Incus host (or + locally when ``host == "localhost"``). Setting an already-present identical + key is a no-op, so this is idempotent (FR-002). Returns ``(ok, err)`` where + *err* is a short message on failure — callers warn but do not fail the whole + command on this alone (FR-005). + """ + cmd = ( + f"incus config set {shlex.quote(name)} " + f"{INCUS_MANAGED_CONFIG_KEY}={INCUS_MANAGED_CONFIG_VALUE}" + ) + result = _ssh_run_on_incus_host(host, user, cmd) + if result.returncode != 0: + return False, (result.stderr.strip() or result.stdout.strip()) + return True, "" + + +def _list_containers_with_marker(host: str, user: str) -> list[tuple[str, bool]]: + """Return ``[(name, marked), ...]`` for every container on *host*. + + Uses a single bulk query ``incus list -f csv -c n,`` (FR-013): + the second CSV column holds the marker value, so no per-container round-trip + is needed. Raises :class:`RuntimeError` if the ``incus list`` call fails so + the caller can surface it. + """ + cmd = f"incus list -f csv -c n,{INCUS_MANAGED_CONFIG_KEY}" + result = _ssh_run_on_incus_host(host, user, cmd) + if result.returncode != 0: + raise RuntimeError(result.stderr.strip() or result.stdout.strip()) + + rows: list[tuple[str, bool]] = [] + for line in result.stdout.splitlines(): + line = line.strip() + if not line: + continue + parts = line.split(",") + cname = parts[0].strip() + if not cname: + continue + marker = parts[1].strip() if len(parts) > 1 else "" + rows.append((cname, marker == INCUS_MANAGED_CONFIG_VALUE)) + return rows + + # --------------------------------------------------------------------------- # Public API # --------------------------------------------------------------------------- @@ -230,6 +285,16 @@ def create( ) ) + # FR-001: mark the container as remo-managed so a default `sync` picks + # it up. FR-005: a marking failure warns but does not fail create. + ok, err = _apply_managed_marker(host, user, name) + if not ok: + print_warning( + f"Container '{name}' was created but could not be marked as " + f"remo-managed ({err}). A default `remo incus sync` will skip " + f"it; use `--all` or re-run `remo incus update` to include it." + ) + if volume_size or cores or memory: rc = _run_resize_playbook( name=name, @@ -350,6 +415,15 @@ def update( if not user and looked_up_user: user = looked_up_user + # FR-004: `update` doubles as the backfill path — ensure the managed marker + # is present (idempotent). FR-005: warn on failure but do not fail update. + ok, err = _apply_managed_marker(host, user, name) + if not ok: + print_warning( + f"Could not mark container '{name}' as remo-managed ({err}); " + f"it may not be picked up by a default `remo incus sync`." + ) + if volume_size or cores or memory: bits: list[str] = [] if volume_size: @@ -520,50 +594,44 @@ def info(name: str, host: str = "", user: str = "") -> int: return 0 -def sync(host: str = "localhost", user: str = "", use_ip: bool = False) -> None: +def sync( + host: str = "localhost", + user: str = "", + use_ip: bool = False, + include_all: bool = False, +) -> None: """Discover Incus containers on *host* and register them in known-hosts. - For localhost, runs ``incus list -f csv -c n`` directly. For remote hosts, - the same command is executed over SSH. All previously registered entries - for the given host prefix are cleared before the newly discovered - containers are saved. - - When *use_ip* is true, each container's eth0 IP is resolved and stored as - the ``host`` field; otherwise the container name itself is stored (and - relies on DNS/MagicDNS for resolution at connect time). + Uses a single ``incus list -f csv -c n,user.remo`` query (locally when + ``host == "localhost"``, else over SSH) that returns each container's name + and managed-marker value (FR-013). By default (``include_all=False``) only + marker-bearing containers are registered (FR-006) and any skipped unmarked + containers are named in a hint (FR-008). With ``include_all=True`` every + container is registered — the pre-feature behavior (FR-007) — and unmarked + adoptions are called out in the summary (FR-009). + + This function never mutates container state: it applies/removes no marker + (FR-010). When *use_ip* is true, each container's eth0 IP is resolved and + stored as the ``host`` field; otherwise the container name is stored. """ - if host == "localhost": - result = subprocess.run( - ["incus", "list", "-f", "csv", "-c", "n"], - capture_output=True, - text=True, - ) - if result.returncode != 0: - print_error(f"Failed to list containers: {result.stderr.strip()}") - sys.exit(1) - else: - ssh_target = f"{user}@{host}" if user else host - result = subprocess.run( - ["ssh", "-o", "ConnectTimeout=10", ssh_target, - "incus list -f csv -c n"], - capture_output=True, - text=True, - ) - if result.returncode != 0: - print_error( - f"Failed to list containers on '{host}': " - f"{result.stderr.strip()}" - ) - sys.exit(1) - - containers = [ - line.strip() for line in result.stdout.splitlines() if line.strip() - ] + try: + discovered = _list_containers_with_marker(host, user) + except RuntimeError as e: + print_error(f"Failed to list containers on '{host}': {e}") + sys.exit(1) # Clear existing entries for this host before re-populating. clear_known_hosts_by_prefix("incus", f"{host}/") - for name in containers: + registered = 0 + skipped: list[str] = [] + adopted_unmarked: list[str] = [] + for name, marked in discovered: + if not include_all and not marked: + skipped.append(name) + continue + if include_all and not marked: + adopted_unmarked.append(name) if use_ip: container_host = _resolve_container_ip(name, host, user) or name else: @@ -578,8 +646,30 @@ def sync(host: str = "localhost", user: str = "", use_ip: bool = False) -> None: access_mode="direct", ) ) + registered += 1 + + print_info(f"Synced {registered} container(s) from '{host}'.") + + if not include_all and skipped: + print_warning( + f"Skipped {len(skipped)} unmarked container(s): {', '.join(skipped)}" + ) + print_info( + f" • Adopt all this run: remo incus sync --host {host} --all" + ) + print_info( + " • Mark one permanently: remo incus update --name " + f"--host {host}" + ) - print_info(f"Synced {len(containers)} container(s) from '{host}'.") + if include_all and adopted_unmarked: + print_warning( + f"{len(adopted_unmarked)} of the registered container(s) are not " + f"remo-created (adopted via --all): {', '.join(adopted_unmarked)}" + ) + print_info( + "Note: a later default `sync` will drop those unmarked one(s) again." + ) def bootstrap( diff --git a/src/remo_cli/providers/proxmox.py b/src/remo_cli/providers/proxmox.py index d1e41f2e..085bd276 100644 --- a/src/remo_cli/providers/proxmox.py +++ b/src/remo_cli/providers/proxmox.py @@ -20,6 +20,7 @@ from datetime import datetime, timezone from remo_cli.core.ansible_runner import run_playbook +from remo_cli.core.config import PROXMOX_MANAGED_TAG from remo_cli.core.known_hosts import ( clear_known_hosts_by_prefix, get_known_hosts, @@ -80,6 +81,104 @@ def _ssh_run(host: str, user: str, command: str) -> subprocess.CompletedProcess[ ) +def _run_on_node( + host: str, user: str, command: str +) -> subprocess.CompletedProcess[str]: + """Run *command* on the Proxmox node — locally when ``host == 'localhost'``, + otherwise over SSH. Used by marker apply/read so both paths behave the same. + """ + if host == "localhost": + return subprocess.run( + ["bash", "-c", command], capture_output=True, text=True + ) + return _ssh_run(host, user, command) + + +# --------------------------------------------------------------------------- +# Managed marker (feature 013-managed-instance-tags) +# --------------------------------------------------------------------------- + + +def _parse_container_tags(config_text: str) -> list[str]: + """Return the ordered guest tags from the ``tags:`` line of ``pct config``. + + Proxmox stores tags separated by ``;`` (and accepts ``;``, ``,`` or space + on input); returns ``[]`` when the container has no tags. + """ + line = _parse_pct_config_field(config_text, "tags") + if not line: + return [] + return [t for t in re.split(r"[;, ]+", line.strip()) if t] + + +def _apply_managed_marker(host: str, user: str, vmid: str) -> tuple[bool, str]: + """Apply the remo managed tag to Proxmox LXC *vmid* (host-side). + + Reads the current tag set from ``pct config `` and, only when the + ``remo`` tag is absent, writes the union back with ``pct set --tags`` + (FR-003: existing tags preserved and not reordered; the new tag is + appended). When ``remo`` is already present this is a strict no-op (FR-002, + SC-005). Returns ``(ok, err)``; a failure warns but does not fail the + enclosing command on its own (FR-005). + """ + if not vmid: + return False, "VMID could not be resolved" + + cfg = _run_on_node(host, user, f"pct config {shlex.quote(vmid)}") + if cfg.returncode != 0: + return False, (cfg.stderr.strip() or cfg.stdout.strip()) + + tags = _parse_container_tags(cfg.stdout) + if PROXMOX_MANAGED_TAG in tags: + return True, "" # already marked — no-op, no reorder + + joined = ";".join([*tags, PROXMOX_MANAGED_TAG]) + res = _run_on_node( + host, user, f"pct set {shlex.quote(vmid)} --tags {shlex.quote(joined)}" + ) + if res.returncode != 0: + return False, (res.stderr.strip() or res.stdout.strip()) + return True, "" + + +def _read_tags_by_vmid(host: str, user: str) -> dict[str, set[str]]: + """Return ``{vmid: {tags}}`` for every LXC on *host* in one bulk query. + + Dumps every ``/etc/pve/lxc/*.conf`` in a single round-trip (FR-013) and + reads only each container's *current* tags — the ``tags:`` line above the + first ``[snapshot]`` section. Snapshot sections carry their own ``tags:`` + lines (a copy of the config at snapshot time), so a naive + ``grep '^tags:'`` would let an old snapshot's tags shadow the live tags and + mis-classify a container. A vmid with no current ``tags:`` line is absent + from the map and treated as unmarked by callers. + """ + result = _run_on_node( + host, + user, + 'for f in /etc/pve/lxc/*.conf; do echo "@@@$f"; cat "$f"; done 2>/dev/null', + ) + mapping: dict[str, set[str]] = {} + vmid: str | None = None + in_snapshot_section = False + for line in result.stdout.splitlines(): + if line.startswith("@@@"): + m = re.search(r"/(\d+)\.conf$", line) + vmid = m.group(1) if m else None + in_snapshot_section = False + continue + if vmid is None or in_snapshot_section: + continue + if line.startswith("["): + in_snapshot_section = True # entering a snapshot's stored config + continue + if line.startswith("tags:"): + _, _, tag_values = line.partition(":") + mapping[vmid] = { + t for t in re.split(r"[;, ]+", tag_values.strip()) if t + } + return mapping + + def _resolve_vmid(name: str, host: str, user: str) -> str: """Determine the VMID for container *name* on the Proxmox *host*. @@ -274,6 +373,23 @@ def create( ) ) + # FR-001: mark the container as remo-managed. FR-005: a marking failure + # (including an unresolved VMID) warns but does not fail create. + if vmid: + ok, err = _apply_managed_marker(host, user, vmid) + if not ok: + print_warning( + f"Container '{name}' was created but could not be marked " + f"as remo-managed ({err}); a default `remo proxmox sync` " + f"will skip it (use `--all` or `remo proxmox update`)." + ) + else: + print_warning( + f"Container '{name}' was created but its VMID could not be " + f"resolved, so it was not marked as remo-managed; run " + f"`remo proxmox update --name {name} --host {host}` to mark it." + ) + # If the container already existed, site.yml skipped pct create and # did not apply the requested resource values. Run the resize # playbook as a follow-up; idempotent (no-op when values match). @@ -416,6 +532,24 @@ def update( if not user: user = "root" + # FR-004: `update` is the backfill path — ensure the managed marker is + # present (idempotent, preserving existing tags). FR-005: warn on failure + # but do not fail update. Resolve the VMID if the registry did not have it. + if not vmid: + vmid = _resolve_vmid(name, host, user) + if vmid: + ok, err = _apply_managed_marker(host, user, vmid) + if not ok: + print_warning( + f"Could not mark container '{name}' as remo-managed ({err}); " + f"it may not be picked up by a default `remo proxmox sync`." + ) + else: + print_warning( + f"Could not resolve a VMID for '{name}'; it was not marked as " + f"remo-managed." + ) + if volume_size or cores or memory: bits: list[str] = [] if volume_size: @@ -591,14 +725,24 @@ def _parse_pct_config_field(config_text: str, field: str) -> str: return match.group(1).strip() if match else "" -def sync(host: str, user: str = "", use_ip: bool = False) -> None: +def sync( + host: str, + user: str = "", + use_ip: bool = False, + include_all: bool = False, +) -> None: """Discover Proxmox LXC containers on *host* and register them. - Runs ``pct list`` over SSH (or locally if host == "localhost"), - parses the output, then queries each container for its VMID. When - *use_ip* is true, each container's eth0 IP is also resolved and stored - as the ``host`` field; otherwise the container name itself is stored - (and relies on DNS/MagicDNS for resolution at connect time). + Runs ``pct list`` for the vmid/name inventory and one bulk + ``grep '^tags:' /etc/pve/lxc/*.conf`` to read every container's tags in a + single round-trip (FR-013). By default (``include_all=False``) only + containers carrying the ``remo`` managed tag are registered (FR-006), and + skipped unmarked containers are named in a hint (FR-008). With + ``include_all=True`` every container is registered — the pre-feature + behavior (FR-007) — and unmarked adoptions are called out (FR-009). + + This function never mutates container state (FR-010). When *use_ip* is true, + each container's eth0 IP is resolved and stored as the ``host`` field. Existing entries with the host prefix are cleared first. """ @@ -632,9 +776,20 @@ def sync(host: str, user: str = "", use_ip: bool = False) -> None: hostname = parts[-1] containers.append((vmid, hostname)) + tags_by_vmid = _read_tags_by_vmid(host, user) + clear_known_hosts_by_prefix("proxmox", f"{host}/") + registered = 0 + skipped: list[str] = [] + adopted_unmarked: list[str] = [] for vmid, hostname in containers: + marked = PROXMOX_MANAGED_TAG in tags_by_vmid.get(vmid, set()) + if not include_all and not marked: + skipped.append(hostname) + continue + if include_all and not marked: + adopted_unmarked.append(hostname) if use_ip: container_host = _resolve_container_ip(hostname, host, user, vmid=vmid) or hostname else: @@ -650,8 +805,30 @@ def sync(host: str, user: str = "", use_ip: bool = False) -> None: region=user or "root", ) ) + registered += 1 - print_info(f"Synced {len(containers)} container(s) from '{host}'.") + print_info(f"Synced {registered} container(s) from '{host}'.") + + if not include_all and skipped: + print_warning( + f"Skipped {len(skipped)} unmarked container(s): {', '.join(skipped)}" + ) + print_info( + f" • Adopt all this run: remo proxmox sync --host {host} --all" + ) + print_info( + " • Mark one permanently: remo proxmox update --name " + f"--host {host}" + ) + + if include_all and adopted_unmarked: + print_warning( + f"{len(adopted_unmarked)} of the registered container(s) are not " + f"remo-created (adopted via --all): {', '.join(adopted_unmarked)}" + ) + print_info( + "Note: a later default `sync` will drop those unmarked one(s) again." + ) def bootstrap( diff --git a/tests/unit/cli/providers/test_incus_sync_all.py b/tests/unit/cli/providers/test_incus_sync_all.py new file mode 100644 index 00000000..af444875 --- /dev/null +++ b/tests/unit/cli/providers/test_incus_sync_all.py @@ -0,0 +1,31 @@ +"""Tests for the `--all` flag on `remo incus sync` (cli/providers/incus.py).""" + +from __future__ import annotations + +import pytest +from click.testing import CliRunner + +from remo_cli.cli.providers.incus import incus + + +@pytest.fixture +def runner(): + return CliRunner() + + +def test_default_sync_passes_include_all_false(runner, mocker): + spy = mocker.patch( + "remo_cli.cli.providers.incus.providers_incus.sync", return_value=None + ) + result = runner.invoke(incus, ["sync", "--host", "h", "--user", "u"]) + assert result.exit_code == 0 + assert spy.call_args.kwargs["include_all"] is False + + +def test_all_flag_threads_include_all_true(runner, mocker): + spy = mocker.patch( + "remo_cli.cli.providers.incus.providers_incus.sync", return_value=None + ) + result = runner.invoke(incus, ["sync", "--host", "h", "--all"]) + assert result.exit_code == 0 + assert spy.call_args.kwargs["include_all"] is True diff --git a/tests/unit/cli/providers/test_proxmox_sync_all.py b/tests/unit/cli/providers/test_proxmox_sync_all.py new file mode 100644 index 00000000..8d24fb43 --- /dev/null +++ b/tests/unit/cli/providers/test_proxmox_sync_all.py @@ -0,0 +1,31 @@ +"""Tests for the `--all` flag on `remo proxmox sync` (cli/providers/proxmox.py).""" + +from __future__ import annotations + +import pytest +from click.testing import CliRunner + +from remo_cli.cli.providers.proxmox import proxmox + + +@pytest.fixture +def runner(): + return CliRunner() + + +def test_default_sync_passes_include_all_false(runner, mocker): + spy = mocker.patch( + "remo_cli.cli.providers.proxmox.providers_proxmox.sync", return_value=None + ) + result = runner.invoke(proxmox, ["sync", "--host", "node", "--user", "root"]) + assert result.exit_code == 0 + assert spy.call_args.kwargs["include_all"] is False + + +def test_all_flag_threads_include_all_true(runner, mocker): + spy = mocker.patch( + "remo_cli.cli.providers.proxmox.providers_proxmox.sync", return_value=None + ) + result = runner.invoke(proxmox, ["sync", "--host", "node", "--all"]) + assert result.exit_code == 0 + assert spy.call_args.kwargs["include_all"] is True diff --git a/tests/unit/providers/test_incus_marker.py b/tests/unit/providers/test_incus_marker.py new file mode 100644 index 00000000..189787a4 --- /dev/null +++ b/tests/unit/providers/test_incus_marker.py @@ -0,0 +1,214 @@ +"""Tests for the Incus managed-marker feature (providers/incus.py). + +Covers marker apply/read helpers, create/update wiring, and filtered sync — +including FR-010 (sync is read-only) and FR-013 (sync makes a bounded number of +host queries). All SSH is mocked; no live Incus host is required. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from remo_cli.providers import incus as providers_incus + + +def _completed(rc: int, stdout: str = "", stderr: str = "") -> MagicMock: + cp = MagicMock() + cp.returncode = rc + cp.stdout = stdout + cp.stderr = stderr + return cp + + +@pytest.fixture +def patch_host(mocker): + """Patch the per-host SSH helper used for all marker host commands.""" + return mocker.patch( + "remo_cli.providers.incus._ssh_run_on_incus_host", autospec=True + ) + + +# --------------------------------------------------------------------------- +# _apply_managed_marker +# --------------------------------------------------------------------------- + + +class TestApplyMarker: + def test_runs_incus_config_set(self, patch_host): + patch_host.return_value = _completed(0) + ok, err = providers_incus._apply_managed_marker("h", "u", "dev1") + assert ok is True + assert err == "" + cmd = patch_host.call_args.args[2] + assert cmd == "incus config set dev1 user.remo=true" + + def test_failure_returns_message_not_exception(self, patch_host): + patch_host.return_value = _completed(1, stderr="boom") + ok, err = providers_incus._apply_managed_marker("h", "u", "dev1") + assert ok is False + assert "boom" in err + + +# --------------------------------------------------------------------------- +# _list_containers_with_marker +# --------------------------------------------------------------------------- + + +class TestListWithMarker: + def test_parses_marked_and_unmarked(self, patch_host): + patch_host.return_value = _completed(0, stdout="dev1,true\nplex,\n") + rows = providers_incus._list_containers_with_marker("h", "u") + assert rows == [("dev1", True), ("plex", False)] + + def test_uses_single_bulk_query(self, patch_host): + patch_host.return_value = _completed(0, stdout="dev1,true\n") + providers_incus._list_containers_with_marker("h", "u") + assert patch_host.call_count == 1 # FR-013: one bulk query + assert "incus list -f csv -c n,user.remo" in patch_host.call_args.args[2] + + def test_failure_raises(self, patch_host): + patch_host.return_value = _completed(1, stderr="nope") + with pytest.raises(RuntimeError): + providers_incus._list_containers_with_marker("h", "u") + + +# --------------------------------------------------------------------------- +# create() wiring +# --------------------------------------------------------------------------- + + +class TestCreateMarks: + def test_create_applies_marker(self, mocker): + mocker.patch("remo_cli.providers.incus.run_playbook", return_value=0) + mocker.patch("remo_cli.providers.incus.remove_known_host") + mocker.patch("remo_cli.providers.incus.save_known_host") + mocker.patch("remo_cli.providers.incus.detect_timezone", return_value="") + mocker.patch( + "remo_cli.providers.incus.get_current_version", return_value="unknown" + ) + apply = mocker.patch( + "remo_cli.providers.incus._apply_managed_marker", + return_value=(True, ""), + ) + rc = providers_incus.create(name="dev1", host="h", user="u") + assert rc == 0 + apply.assert_called_once_with("h", "u", "dev1") + + def test_marker_failure_warns_but_create_succeeds(self, mocker): + mocker.patch("remo_cli.providers.incus.run_playbook", return_value=0) + mocker.patch("remo_cli.providers.incus.remove_known_host") + mocker.patch("remo_cli.providers.incus.save_known_host") + mocker.patch("remo_cli.providers.incus.detect_timezone", return_value="") + mocker.patch( + "remo_cli.providers.incus.get_current_version", return_value="unknown" + ) + mocker.patch( + "remo_cli.providers.incus._apply_managed_marker", + return_value=(False, "denied"), + ) + warn = mocker.patch("remo_cli.providers.incus.print_warning") + rc = providers_incus.create(name="dev1", host="h", user="u") + assert rc == 0 # FR-005: create still succeeds + assert warn.called + + +# --------------------------------------------------------------------------- +# update() wiring (backfill) +# --------------------------------------------------------------------------- + + +class TestUpdateBackfill: + def test_update_applies_marker(self, mocker): + apply = mocker.patch( + "remo_cli.providers.incus._apply_managed_marker", + return_value=(True, ""), + ) + mocker.patch( + "remo_cli.providers.incus._resolve_container_ip", return_value="10.0.0.5" + ) + mocker.patch("remo_cli.providers.incus.run_playbook", return_value=0) + mocker.patch("remo_cli.providers.incus.detect_timezone", return_value="") + mocker.patch( + "remo_cli.providers.incus.get_current_version", return_value="unknown" + ) + rc = providers_incus.update(name="dev1", host="h", user="u") + assert rc == 0 + apply.assert_called_once_with("h", "u", "dev1") + + +# --------------------------------------------------------------------------- +# sync() — filtering, hint, FR-010 (read-only), FR-013 (bounded) +# --------------------------------------------------------------------------- + + +@pytest.fixture +def patch_registry(mocker): + save = mocker.patch("remo_cli.providers.incus.save_known_host") + mocker.patch("remo_cli.providers.incus.clear_known_hosts_by_prefix") + return save + + +class TestSyncFiltering: + def test_default_registers_only_marked(self, patch_host, patch_registry, mocker): + patch_host.return_value = _completed(0, stdout="dev1,true\nplex,\n") + info = mocker.patch("remo_cli.providers.incus.print_info") + warn = mocker.patch("remo_cli.providers.incus.print_warning") + + providers_incus.sync(host="h", user="u") + + # Only dev1 registered. + saved = [c.args[0].name for c in patch_registry.call_args_list] + assert saved == ["h/dev1"] + # Hint names the skipped container. + warn_text = " ".join(str(c.args[0]) for c in warn.call_args_list) + assert "plex" in warn_text + info_text = " ".join(str(c.args[0]) for c in info.call_args_list) + assert "--all" in info_text and "remo incus update" in info_text + + def test_all_registers_everything(self, patch_host, patch_registry, mocker): + patch_host.return_value = _completed(0, stdout="dev1,true\nplex,\n") + warn = mocker.patch("remo_cli.providers.incus.print_warning") + mocker.patch("remo_cli.providers.incus.print_info") + + providers_incus.sync(host="h", user="u", include_all=True) + + saved = sorted(c.args[0].name for c in patch_registry.call_args_list) + assert saved == ["h/dev1", "h/plex"] + warn_text = " ".join(str(c.args[0]) for c in warn.call_args_list) + assert "plex" in warn_text + # The adopted-unmarked summary must actually be emitted (FR-009). + assert "not remo-created" in warn_text + + def test_sync_is_read_only_and_bounded(self, patch_host, patch_registry, mocker): + patch_host.return_value = _completed(0, stdout="dev1,true\nplex,\n") + mocker.patch("remo_cli.providers.incus.print_info") + mocker.patch("remo_cli.providers.incus.print_warning") + + providers_incus.sync(host="h", user="u") + + # FR-013: a single bulk host query, regardless of container count. + assert patch_host.call_count == 1 + # FR-010: sync issues no marker mutation. + for call in patch_host.call_args_list: + assert "config set" not in call.args[2] + + def test_registry_shape_unchanged(self, patch_host, patch_registry, mocker): + # FR-012: the KnownHost written by sync keeps its pre-feature fields; + # marker state is not recorded in the registry. + patch_host.return_value = _completed(0, stdout="dev1,true\n") + mocker.patch("remo_cli.providers.incus.print_info") + mocker.patch("remo_cli.providers.incus.print_warning") + + providers_incus.sync(host="h", user="u") + + kh = patch_registry.call_args.args[0] + assert kh.type == "incus" + assert kh.name == "h/dev1" + assert kh.user == "remo" + assert kh.instance_id == "u" + assert kh.access_mode == "direct" + # No marker attribute leaked onto the registry entry. + assert not hasattr(kh, "marker") + assert not hasattr(kh, "managed") diff --git a/tests/unit/providers/test_proxmox_marker.py b/tests/unit/providers/test_proxmox_marker.py new file mode 100644 index 00000000..d1cbee03 --- /dev/null +++ b/tests/unit/providers/test_proxmox_marker.py @@ -0,0 +1,245 @@ +"""Tests for the Proxmox managed-marker feature (providers/proxmox.py). + +Covers tag apply (union, preserve, no-op), bulk tag read, create/update wiring, +and filtered sync — including FR-003 (preserve tags), FR-010 (read-only), and +FR-013 (bounded queries). All SSH is mocked. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +import pytest + +from remo_cli.providers import proxmox as providers_proxmox + + +def _completed(rc: int, stdout: str = "", stderr: str = "") -> MagicMock: + cp = MagicMock() + cp.returncode = rc + cp.stdout = stdout + cp.stderr = stderr + return cp + + +# --------------------------------------------------------------------------- +# _apply_managed_marker — union, preserve, idempotent no-op +# --------------------------------------------------------------------------- + + +class TestApplyMarker: + def test_appends_remo_preserving_existing_tags(self, mocker): + node = mocker.patch( + "remo_cli.providers.proxmox._run_on_node", autospec=True + ) + node.side_effect = [ + _completed(0, stdout="hostname: dev1\ntags: mytag\ncores: 2\n"), + _completed(0), # pct set + ] + ok, err = providers_proxmox._apply_managed_marker("h", "u", "100") + assert ok is True and err == "" + set_cmd = node.call_args_list[1].args[2] + # existing tag preserved + appended (shell-quoted for the ; separator) + assert "--tags 'mytag;remo'" in set_cmd + + def test_noop_when_already_marked(self, mocker): + node = mocker.patch( + "remo_cli.providers.proxmox._run_on_node", autospec=True + ) + node.return_value = _completed(0, stdout="tags: mytag;remo\n") + ok, err = providers_proxmox._apply_managed_marker("h", "u", "100") + assert ok is True + # FR-002/SC-005: no `pct set` issued — only the config read. + assert node.call_count == 1 + + def test_empty_vmid_is_a_soft_failure(self, mocker): + node = mocker.patch("remo_cli.providers.proxmox._run_on_node") + ok, err = providers_proxmox._apply_managed_marker("h", "u", "") + assert ok is False and err + node.assert_not_called() + + +# --------------------------------------------------------------------------- +# _read_tags_by_vmid — one bulk grep +# --------------------------------------------------------------------------- + + +class TestReadTags: + def test_parses_conf_dump(self, mocker): + node = mocker.patch( + "remo_cli.providers.proxmox._run_on_node", autospec=True + ) + node.return_value = _completed( + 0, + stdout=( + "@@@/etc/pve/lxc/100.conf\n" + "arch: amd64\n" + "tags: remo\n" + "@@@/etc/pve/lxc/101.conf\n" + "tags: media;plex\n" + ), + ) + mapping = providers_proxmox._read_tags_by_vmid("h", "u") + assert mapping == {"100": {"remo"}, "101": {"media", "plex"}} + assert node.call_count == 1 # FR-013 + + def test_ignores_snapshot_section_tags(self, mocker): + # A snapshot section's tags: line must NOT shadow the live tags — + # regression for the grep-last-wins mis-classification bug. + node = mocker.patch( + "remo_cli.providers.proxmox._run_on_node", autospec=True + ) + node.return_value = _completed( + 0, + stdout=( + "@@@/etc/pve/lxc/100.conf\n" + "tags: media;remo\n" # current: marked + "[pre-upgrade]\n" + "tags: media\n" # old snapshot: no remo + "@@@/etc/pve/lxc/101.conf\n" + "tags: media\n" # current: unmarked + "[snap]\n" + "tags: media;remo\n" # old snapshot: had remo + ), + ) + mapping = providers_proxmox._read_tags_by_vmid("h", "u") + assert mapping == {"100": {"media", "remo"}, "101": {"media"}} + + +# --------------------------------------------------------------------------- +# create() / update() wiring +# --------------------------------------------------------------------------- + + +class TestCreateUpdateWiring: + def test_create_marks_resolved_vmid(self, mocker): + mocker.patch("remo_cli.providers.proxmox.run_playbook", return_value=0) + mocker.patch("remo_cli.providers.proxmox.remove_known_host") + mocker.patch("remo_cli.providers.proxmox.save_known_host") + mocker.patch("remo_cli.providers.proxmox.detect_timezone", return_value="") + mocker.patch( + "remo_cli.providers.proxmox.get_current_version", return_value="unknown" + ) + mocker.patch( + "remo_cli.providers.proxmox.resolve_devcontainer_runtime", + return_value="devcontainer", + ) + mocker.patch("remo_cli.providers.proxmox._resolve_vmid", return_value="100") + apply = mocker.patch( + "remo_cli.providers.proxmox._apply_managed_marker", + return_value=(True, ""), + ) + rc = providers_proxmox.create(name="dev1", host="node", user="root") + assert rc == 0 + apply.assert_called_once_with("node", "root", "100") + + def test_update_backfills_marker(self, mocker): + mocker.patch("remo_cli.providers.proxmox._resolve_vmid", return_value="100") + mocker.patch( + "remo_cli.providers.proxmox._resolve_container_ip", return_value="10.0.0.9" + ) + mocker.patch("remo_cli.providers.proxmox.run_playbook", return_value=0) + mocker.patch("remo_cli.providers.proxmox.detect_timezone", return_value="") + mocker.patch( + "remo_cli.providers.proxmox.get_current_version", return_value="unknown" + ) + mocker.patch( + "remo_cli.providers.proxmox.resolve_devcontainer_runtime", + return_value="devcontainer", + ) + apply = mocker.patch( + "remo_cli.providers.proxmox._apply_managed_marker", + return_value=(True, ""), + ) + rc = providers_proxmox.update(name="dev1", host="node", user="root") + assert rc == 0 + apply.assert_called_once_with("node", "root", "100") + + +# --------------------------------------------------------------------------- +# sync() — filtering, hint, FR-010 (read-only), FR-013 (bounded) +# --------------------------------------------------------------------------- + + +_PCT_LIST = ( + "VMID Status Lock Name\n" + "100 running dev1\n" + "101 running plex\n" +) + + +@pytest.fixture +def patch_registry(mocker): + save = mocker.patch("remo_cli.providers.proxmox.save_known_host") + mocker.patch("remo_cli.providers.proxmox.clear_known_hosts_by_prefix") + return save + + +class TestSyncFiltering: + def _wire_ssh(self, mocker): + """Route `pct list` and the bulk conf dump through _ssh_run.""" + def side_effect(host, user, cmd): + if cmd == "pct list": + return _completed(0, stdout=_PCT_LIST) + if cmd.startswith("for f in /etc/pve/lxc/"): + return _completed( + 0, stdout="@@@/etc/pve/lxc/100.conf\ntags: remo\n" + ) + return _completed(0) + + return mocker.patch( + "remo_cli.providers.proxmox._ssh_run", side_effect=side_effect + ) + + def test_default_registers_only_marked(self, patch_registry, mocker): + self._wire_ssh(mocker) + mocker.patch("remo_cli.providers.proxmox.print_info") + warn = mocker.patch("remo_cli.providers.proxmox.print_warning") + + providers_proxmox.sync(host="node", user="root") + + saved = [c.args[0].name for c in patch_registry.call_args_list] + assert saved == ["node/dev1"] + warn_text = " ".join(str(c.args[0]) for c in warn.call_args_list) + assert "plex" in warn_text + + def test_all_registers_everything(self, patch_registry, mocker): + self._wire_ssh(mocker) + mocker.patch("remo_cli.providers.proxmox.print_info") + mocker.patch("remo_cli.providers.proxmox.print_warning") + + providers_proxmox.sync(host="node", user="root", include_all=True) + + saved = sorted(c.args[0].name for c in patch_registry.call_args_list) + assert saved == ["node/dev1", "node/plex"] + + def test_read_only_and_bounded(self, patch_registry, mocker): + ssh = self._wire_ssh(mocker) + mocker.patch("remo_cli.providers.proxmox.print_info") + mocker.patch("remo_cli.providers.proxmox.print_warning") + + providers_proxmox.sync(host="node", user="root") + + # FR-013: two bulk calls (pct list + one grep), no per-container loop. + assert ssh.call_count == 2 + # FR-010: no marker mutation during sync. + for call in ssh.call_args_list: + assert "pct set" not in call.args[2] + + def test_registry_shape_unchanged(self, patch_registry, mocker): + # FR-012: the KnownHost written by sync keeps its pre-feature fields. + self._wire_ssh(mocker) + mocker.patch("remo_cli.providers.proxmox.print_info") + mocker.patch("remo_cli.providers.proxmox.print_warning") + + providers_proxmox.sync(host="node", user="root") + + kh = patch_registry.call_args.args[0] + assert kh.type == "proxmox" + assert kh.name == "node/dev1" + assert kh.user == "remo" + assert kh.instance_id == "100" + assert kh.region == "root" + assert kh.access_mode == "direct" + assert not hasattr(kh, "marker") + assert not hasattr(kh, "managed")