From b3577b6994301afb987cd629494b12e137e4630c Mon Sep 17 00:00:00 2001 From: Jeremy Harrington Date: Thu, 8 Oct 2026 16:55:56 -0700 Subject: [PATCH 1/3] fix(proxmox): observe live VM config without false drift Decode numeric configuration strings and propagate failed configuration reads. Observe native VM settings and use provider-owned comparison semantics across Get, List, Watch, and reconciliation, excluding creation-only and write-only inputs while preserving real drift. Cover numeric decoding, configuration outages, keyed device comparison, and automatic reconciliation with regressions. Verified with repository/plugin tests, focused race tests, a full build, and read-only Get/List against the homelab dev-system VM. --- CONTROLLER.md | 22 +- ROADMAP.md | 8 + docs/homelab-verification.md | 12 +- internal/controller/providers/provider.go | 17 ++ .../providers/proxmox/apply_observed_test.go | 12 +- .../providers/proxmox/conformance_test.go | 4 + .../providers/proxmox/multicontext_test.go | 15 +- .../controller/providers/proxmox/provider.go | 14 +- .../providers/proxmox/provider_test.go | 8 +- internal/controller/reconciler/reconciler.go | 3 +- .../controller/server/proxmox_drift_test.go | 175 ++++++++++++++++ internal/controller/server/resource.go | 10 +- internal/controller/server/watch.go | 8 +- pkg/proxmox/client/client.go | 86 ++++++-- pkg/proxmox/client/client_test.go | 93 +++++++++ pkg/proxmox/handler/handler.go | 11 +- pkg/proxmox/handler/handler_test.go | 117 +++++++++++ pkg/proxmox/handler/resize.go | 5 +- pkg/proxmox/resources/vm.go | 8 +- pkg/proxmox/resources/vm_compare.go | 194 ++++++++++++++++++ pkg/proxmox/resources/vm_observation.go | 193 +++++++++++++++++ pkg/proxmox/resources/vm_observation_test.go | 144 +++++++++++++ 22 files changed, 1098 insertions(+), 61 deletions(-) create mode 100644 internal/controller/server/proxmox_drift_test.go create mode 100644 pkg/proxmox/resources/vm_compare.go create mode 100644 pkg/proxmox/resources/vm_observation.go create mode 100644 pkg/proxmox/resources/vm_observation_test.go diff --git a/CONTROLLER.md b/CONTROLLER.md index 23c6f3c..f0eb157 100644 --- a/CONTROLLER.md +++ b/CONTROLLER.md @@ -20,10 +20,21 @@ in. Any change requires re-opening the discussion. gap-filling. - **Comparison:** loose. Only manifest-specified fields are tracked. Provider-set defaults are unmanaged. +- **VM observation:** Get and List read authoritative Proxmox configuration, + including configured CPU/RAM, disks, NICs, agent, native cloud-init settings, + firmware, EFI, and PCI passthrough. Configuration-read/decoding failures are + errors, not incomplete observations; numeric JSON strings are accepted. +- **VM comparison:** declared disk/NIC slots are matched by name; undeclared + slots remain unmanaged. Equivalent binary disk-size units and MAC address + casing compare equal. Creation/routing inputs (`context`, `template`, + `image`, `cloudImage`, `startOnCreate`) and unobservable cloud-init inputs + (`password`, `packages`, `runcmd`) are excluded from live drift, not echoed + into observations. They remain in desired manifests and desired-to-desired + dry-run diffs; changing creation/first-boot inputs requires replacement. - **Apply on existing atomic resource (e.g. `VirtualMachine`):** update in place for the fields the provider can change live — for `VirtualMachine`: memory, CPU (cores/sockets), and disk **growth**. Non-resizable differences - (template, networks, cloud-init) are not mutated; they surface as drift and + (networks, native cloud-init settings) are not mutated; they surface as drift and still require delete + re-apply. Disk **shrink** is rejected (Proxmox can't shrink) — delete + re-apply to size down. Re-applying an unchanged spec is a no-op (identical config, disks already at size). @@ -362,9 +373,12 @@ destructive guardrails enforced. `internal/controller/server/drift.go` walks the desired spec and surfaces only the keys where desired ≠ observed; provider-set defaults are unmanaged. -- [x] VirtualMachine drift on Get: works generically via the loose - comparison (proxmox `VMToResource` returns spec keys that overlap - with the manifest, e.g. `cpu.cores`, `memory.size`). +- [x] VirtualMachine drift on Get/List/Watch and periodic reconciliation uses + the same provider-owned comparison projection over native configuration. + Missing declared settings still drift; only explicit creation-only, + routing, write-only, and first-boot execution inputs are excluded. + Failed configuration reads surface as errors, including name-scoped + Watch (an outage must not masquerade as deletion). - [x] Cluster structural drift on Get: the k3s provider synthesizes `spec.nodes.controlPlane.count` and each `spec.nodes.workers[*] .count` from the *actual* children list, so post-apply Gets reveal diff --git a/ROADMAP.md b/ROADMAP.md index 7c47a35..ba9b233 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -45,6 +45,14 @@ plugin protocol → Terraform/OpenTofu host → run-anywhere Linux daemon (OIDC slice) and CUE-WASM validation are parked behind that. (Mobile layout has since shipped.) +**Proxmox VM observation correction (2026-10-08):** Get/List now decode numeric +configuration strings and observe native managed settings instead of partial +runtime inventory. Get/List/Watch and reconciliation share provider-owned drift +semantics: declared disk/NIC slots matched by name, equivalent sizes normalized, +creation/write-only inputs excluded, and read outages reported rather than +mistaken for deletion. Root/plugin tests and a read-only homelab Get/List smoke +on `dev-system` pass with zero drift; no VM configuration was changed. + ## Homelab completion (COMPLETE — 2026-07-12) Backend-vs-vision gap audit for running the full homelab through openctl diff --git a/docs/homelab-verification.md b/docs/homelab-verification.md index 91cd2f2..ca752bb 100644 --- a/docs/homelab-verification.md +++ b/docs/homelab-verification.md @@ -22,12 +22,12 @@ sees every VM on the node (so the collision guard can't be fooled) and its `-o yaml` surfaces the **vmid** — the field that proves an in-place resize did not recreate the VM. -The verify VM is created **stopped** on purpose. A resize test doesn't need a -booted guest, it's faster, and it keeps memory observation reliable: `proxmox -get vms` reports a *running* VM's live `MaxMem`, which lags a config change until -reboot — so a running VM would show the old memory even though the config was -correctly updated. (This nuance was found on the first real-hardware run and is -why the harness now creates stopped.) +The verify VM is created **stopped** on purpose: booting a guest is unnecessary +for a resize test and makes the harness slower. Get/List now report configured +CPU/RAM from Proxmox's configuration API, even while a running guest still uses +its pre-reboot allocation. Guest-visible resources can therefore differ until +the guest applies the change or reboots; the configuration API is authoritative +for validating the resize. ## Safety diff --git a/internal/controller/providers/provider.go b/internal/controller/providers/provider.go index 07eb7df..60c69a3 100644 --- a/internal/controller/providers/provider.go +++ b/internal/controller/providers/provider.go @@ -57,6 +57,23 @@ type ObservedOnly interface { ObservedOnlyKinds() []string } +// DriftProjector supplies provider-specific observation comparison semantics. +// Creation-only and write-only inputs cannot be compared to live state, and +// named collections may need alignment. Projections must not mutate either +// spec or substitute desired values for missing live configuration. +type DriftProjector interface { + DriftSpecs(kind string, desired, observed map[string]any) (map[string]any, map[string]any) +} + +// DriftSpecs projects desired-versus-live comparison inputs. Manifest editing +// and dry-run desired-versus-desired diffs must retain the original specs. +func DriftSpecs(p Provider, kind string, desired, observed map[string]any) (map[string]any, map[string]any) { + if projector, ok := p.(DriftProjector); ok { + return projector.DriftSpecs(kind, desired, observed) + } + return desired, observed +} + // DryRunner is an optional provider capability for previewing what an // Apply would do without performing it. Composite providers (k3s Cluster // → VMs) implement this to surface per-child actions and the gates the diff --git a/internal/controller/providers/proxmox/apply_observed_test.go b/internal/controller/providers/proxmox/apply_observed_test.go index d8bd816..5216d8c 100644 --- a/internal/controller/providers/proxmox/apply_observed_test.go +++ b/internal/controller/providers/proxmox/apply_observed_test.go @@ -16,10 +16,8 @@ import ( // nil result here regresses that contract (and would fail the shared // providertest conformance battery once proxmox is bound to it). // -// This drives the apply-on-existing path: the VM "vm0" already exists, so the -// handler's applyVM takes the no-op branch (returns observed state, no clone), -// and Apply then reads it back. A permissive catch-all keeps incidental calls (config, guest-agent -// IP) benign so the test asserts only the return-value contract. +// The desired manifest only specifies placement; the returned resource must +// still expose independently observed configured hardware. func TestApplyReturnsObservedVM(t *testing.T) { srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -28,6 +26,8 @@ func TestApplyReturnsObservedVM(t *testing.T) { _, _ = io.WriteString(w, `{"data":[{"node":"pve1","status":"online"}]}`) case "/api2/json/nodes/pve1/qemu": _, _ = io.WriteString(w, `{"data":[{"vmid":100,"name":"vm0","status":"running","node":"pve1","template":0}]}`) + case "/api2/json/nodes/pve1/qemu/100/config": + _, _ = io.WriteString(w, `{"data":{"cores":4,"sockets":1,"memory":"4096"}}`) default: // config / resize / agent-network-get / etc. — benign success so // the update path completes without hitting an unhandled route. @@ -52,7 +52,7 @@ func TestApplyReturnsObservedVM(t *testing.T) { if got.Kind != kindVM || got.Metadata.Name != "vm0" { t.Errorf("Apply returned %s/%s, want %s/vm0", got.Kind, got.Metadata.Name, kindVM) } - if got.APIVersion == "" { - t.Error("Apply result APIVersion is empty; providers must stamp it") + if got.Spec["cpu"].(map[string]any)["cores"] != 4 || got.Spec["memory"].(map[string]any)["size"] != 4096 { + t.Errorf("Apply returned incomplete or invented hardware: %v", got.Spec) } } diff --git a/internal/controller/providers/proxmox/conformance_test.go b/internal/controller/providers/proxmox/conformance_test.go index 0fafc39..538d593 100644 --- a/internal/controller/providers/proxmox/conformance_test.go +++ b/internal/controller/providers/proxmox/conformance_test.go @@ -59,6 +59,10 @@ func (f *fakeProxmox) newServer(t *testing.T) *httptest.Server { newid, _ := strconv.Atoi(r.Form.Get("newid")) f.vms[r.Form.Get("name")] = newid _ = json.NewEncoder(w).Encode(map[string]any{"data": ""}) + case strings.HasSuffix(path, "/config") && r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{"data": map[string]any{ + "cores": 1, "sockets": 1, "memory": "512", + }}) case r.Method == http.MethodDelete && strings.Contains(path, "/qemu/"): vmid, _ := strconv.Atoi(path[strings.LastIndex(path, "/")+1:]) for name, id := range f.vms { diff --git a/internal/controller/providers/proxmox/multicontext_test.go b/internal/controller/providers/proxmox/multicontext_test.go index 42d1a01..6a35eec 100644 --- a/internal/controller/providers/proxmox/multicontext_test.go +++ b/internal/controller/providers/proxmox/multicontext_test.go @@ -78,13 +78,15 @@ func TestApplyUnknownContextErrors(t *testing.T) { // configured endpoint, merged into one result set. func TestListMergesAcrossEndpoints(t *testing.T) { srvA := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveA","status":"online"}]}`, - "/api2/json/nodes/pveA/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pveA","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveA","status":"online"}]}`, + "/api2/json/nodes/pveA/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pveA","template":0}]}`, + "/api2/json/nodes/pveA/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvA.Close() srvB := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, - "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, + "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes/pveB/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvB.Close() @@ -115,8 +117,9 @@ func TestGetRoutesToOwningEndpoint(t *testing.T) { }) defer srvA.Close() srvB := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, - "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes": `{"data":[{"node":"pveB","status":"online"}]}`, + "/api2/json/nodes/pveB/qemu": `{"data":[{"vmid":200,"name":"cache-01","status":"running","node":"pveB","template":0}]}`, + "/api2/json/nodes/pveB/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srvB.Close() diff --git a/internal/controller/providers/proxmox/provider.go b/internal/controller/providers/proxmox/provider.go index 60d6932..f679d51 100644 --- a/internal/controller/providers/proxmox/provider.go +++ b/internal/controller/providers/proxmox/provider.go @@ -17,6 +17,7 @@ import ( "github.com/openctl/openctl/internal/controller/providers" "github.com/openctl/openctl/pkg/protocol" pmhandler "github.com/openctl/openctl/pkg/proxmox/handler" + pmresources "github.com/openctl/openctl/pkg/proxmox/resources" ) const ( @@ -147,6 +148,14 @@ func (p *Provider) Kinds() []string { return []string{kindVM, kindNode} } // API rather than provisioning them — they can never be in applied_manifests. func (p *Provider) ObservedOnlyKinds() []string { return []string{kindNode} } +// DriftSpecs compares observable VM settings, excluding creation-only inputs. +func (p *Provider) DriftSpecs(kind string, desired, observed map[string]any) (map[string]any, map[string]any) { + if kind == kindVM { + return pmresources.ComparableVMSpecs(desired, observed) + } + return desired, observed +} + // Actions implements providers.Actioner. VirtualMachine supports the // standard power-lifecycle set plus "console" (opens Proxmox noVNC in // a new tab); ProxmoxNode has no runtime actions. @@ -216,9 +225,8 @@ func (p *Provider) DoAction(ctx context.Context, kind, name, action string) (*pr } } -// Apply creates a VM if missing; otherwise returns the observed state -// without mutating (per the no-op-on-existing rule). ProxmoxNode is -// observed-only and rejects Apply. +// Apply creates a missing VM or resizes an existing VM's CPU, memory, and disks, +// then returns observed state. ProxmoxNode is observed-only and rejects Apply. func (p *Provider) Apply(ctx context.Context, manifest *protocol.Resource) (*protocol.Resource, error) { if manifest.Kind == kindNode { return nil, fmt.Errorf("%s is observed-only; cannot be applied", kindNode) diff --git a/internal/controller/providers/proxmox/provider_test.go b/internal/controller/providers/proxmox/provider_test.go index 9d09c83..202352b 100644 --- a/internal/controller/providers/proxmox/provider_test.go +++ b/internal/controller/providers/proxmox/provider_test.go @@ -130,6 +130,9 @@ func TestProviderChildrenOfNodeReturnsVMs(t *testing.T) { "/api2/json/nodes/pve2/qemu": `{"data":[ {"vmid":200,"name":"cache-01","status":"running","node":"pve2","template":0} ]}`, + "/api2/json/nodes/pve1/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, + "/api2/json/nodes/pve1/qemu/101/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, + "/api2/json/nodes/pve2/qemu/200/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srv.Close() @@ -161,8 +164,9 @@ func TestProviderChildrenOfNodeReturnsVMs(t *testing.T) { func TestProviderListReturnsVMs(t *testing.T) { srv := mockProxmox(t, map[string]string{ - "/api2/json/nodes": `{"data":[{"node":"pve1","status":"online"}]}`, - "/api2/json/nodes/pve1/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pve1","template":0},{"vmid":200,"name":"tpl-x","status":"stopped","node":"pve1","template":1}]}`, + "/api2/json/nodes": `{"data":[{"node":"pve1","status":"online"}]}`, + "/api2/json/nodes/pve1/qemu": `{"data":[{"vmid":100,"name":"web-01","status":"running","node":"pve1","template":0},{"vmid":200,"name":"tpl-x","status":"stopped","node":"pve1","template":1}]}`, + "/api2/json/nodes/pve1/qemu/100/config": `{"data":{"cores":2,"sockets":1,"memory":"2048"}}`, }) defer srv.Close() diff --git a/internal/controller/reconciler/reconciler.go b/internal/controller/reconciler/reconciler.go index d6e2295..dff8d03 100644 --- a/internal/controller/reconciler/reconciler.go +++ b/internal/controller/reconciler/reconciler.go @@ -226,7 +226,8 @@ func (r *Reconciler) reconcileOne(ctx context.Context, ref manifests.Ref) { return } - drifted := !specsEqual(desired.Spec, observed.Spec) + desiredSpec, observedSpec := providers.DriftSpecs(p, ref.Kind, desired.Spec, observed.Spec) + drifted := !specsEqual(desiredSpec, observedSpec) reason := "" if drifted { reason = "spec drift" diff --git a/internal/controller/server/proxmox_drift_test.go b/internal/controller/server/proxmox_drift_test.go new file mode 100644 index 0000000..379eba7 --- /dev/null +++ b/internal/controller/server/proxmox_drift_test.go @@ -0,0 +1,175 @@ +package server + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + "time" + + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + + "github.com/openctl/openctl/internal/controller/providers" + pmprovider "github.com/openctl/openctl/internal/controller/providers/proxmox" + "github.com/openctl/openctl/internal/controller/reconciler" + apiv1 "github.com/openctl/openctl/pkg/api/v1" + "github.com/openctl/openctl/pkg/protocol" +) + +// Exercise real Proxmox decoding and observation through the controller's +// consumers. Inventory deliberately differs from configured hardware, as it +// does after a running VM's configuration changes without a reboot. +func TestProxmoxDriftUsesLiveConfiguration(t *testing.T) { + var mu sync.Mutex + config := map[string]any{ + "name": "dev-system", "cores": 8, "sockets": "1", "memory": "10240", + "scsi0": "local-lvm:vm-101-disk-0,size=300G", + "ide2": "local-lvm:vm-101-cloudinit,media=cdrom,size=4M", + "net0": "virtio=BC:24:11:CC:5D:77,bridge=vmbr0", + "agent": "1", "ciuser": "ubuntu", "ipconfig0": "ip=dhcp", + "sshkeys": url.PathEscape("ssh-ed25519 AAAA development-key"), + } + configUnavailable := false + pve := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method != http.MethodGet { + t.Errorf("unexpected mutation: %s %s", req.Method, req.URL.Path) + http.Error(w, "read-only fixture", http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", "application/json") + switch req.URL.Path { + case "/api2/json/nodes": + _, _ = w.Write([]byte(`{"data":[{"node":"pve1"}]}`)) + case "/api2/json/nodes/pve1/qemu": + _, _ = w.Write([]byte(`{"data":[{"vmid":101,"name":"dev-system","status":"stopped","cpus":2,"maxmem":2147483648}]}`)) + case "/api2/json/nodes/pve1/qemu/101/config": + mu.Lock() + defer mu.Unlock() + if configUnavailable { + http.Error(w, "configuration unavailable", http.StatusServiceUnavailable) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"data": config}) + default: + t.Errorf("unexpected read: %s", req.URL.Path) + http.NotFound(w, req) + } + })) + defer pve.Close() + + reg := providers.NewRegistry() + reg.Register(pmprovider.New(&pmprovider.Config{Endpoint: pve.URL, TokenID: "test", TokenSecret: "test"})) + addr, mat, store := startTestServerWithManifests(t, reg) + conn := dialTestServer(t, addr, mat.CACertPath) + defer func() { _ = conn.Close() }() + desired := &protocol.Resource{ + APIVersion: "proxmox.openctl.io/v1", Kind: "VirtualMachine", + Metadata: protocol.ResourceMetadata{ + Name: "dev-system", Annotations: map[string]string{"openctl.io/autoReconcile": "true"}, + }, + Spec: map[string]any{ + "context": "homelab", "node": "pve1", "template": map[string]any{"name": "ubuntu"}, + "startOnCreate": true, + "cpu": map[string]any{"cores": 8, "sockets": 1}, "memory": map[string]any{"size": 10240}, + "disks": []any{map[string]any{"name": "scsi0", "storage": "local-lvm", "size": "307200M"}}, + "networks": []any{map[string]any{"name": "net0", "bridge": "vmbr0", "model": "virtio", "firewall": false}}, + "agent": map[string]any{"enabled": true}, + "cloudInit": map[string]any{ + "user": "ubuntu", "sshKeys": []any{"ssh-ed25519 AAAA development-key"}, + "ipConfig": map[string]any{"net0": map[string]any{"ip": "dhcp"}}, + "password": map[string]any{"$secret": "initial-password"}, + "packages": []any{"build-essential"}, "runcmd": []any{"true"}, + }, + }, + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if err := store.Save(ctx, desired); err != nil { + t.Fatal(err) + } + client := apiv1.NewResourceServiceClient(conn) + get := func() *apiv1.Resource { + t.Helper() + response, err := client.Get(ctx, &apiv1.GetRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}) + if err != nil { + t.Fatal(err) + } + return response.GetResource() + } + list := func() *apiv1.Resource { + t.Helper() + response, err := client.List(ctx, &apiv1.ListRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind}) + if err != nil { + t.Fatal(err) + } + if len(response.GetResources()) != 1 { + t.Fatalf("managed inventory = %v, want dev-system only", response.GetResources()) + } + return response.GetResources()[0] + } + for _, read := range []func() *apiv1.Resource{get, list} { + if resource := read(); len(resource.GetDrift()) != 0 { + t.Fatalf("configured VM reported false drift: %v", resource.GetDrift()) + } + } + + applies := 0 + rec := reconciler.New(reg, store, 0).WithAutoApply(func(context.Context, *protocol.Resource) error { + applies++ + return nil + }) + rec.ReconcileOnce(ctx) + if applies != 0 { + t.Fatal("matching live config must not trigger automatic apply") + } + + stream, err := client.Watch(ctx, &apiv1.WatchRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}) + if err != nil { + t.Fatal(err) + } + first, err := stream.Recv() + if err != nil || first.GetType() != apiv1.WatchEvent_ADDED || len(first.GetResource().GetDrift()) != 0 { + t.Fatalf("initial watch snapshot = %v, %v; want clean ADDED", first, err) + } + mu.Lock() + config["scsi0"] = "local-lvm:vm-101-disk-0,size=320G" + mu.Unlock() + changed, err := stream.Recv() + if err != nil || changed.GetType() != apiv1.WatchEvent_MODIFIED { + t.Fatalf("live disk change = %v, %v; want MODIFIED", changed, err) + } + for _, resource := range []*apiv1.Resource{changed.GetResource(), get(), list()} { + drift := resource.GetDrift() + if len(drift) != 1 || drift[0].GetPath() != "spec.disks[0].size" { + t.Fatalf("live disk change must remain visible: %v", drift) + } + } + rec.ReconcileOnce(ctx) + if applies != 1 { + t.Fatalf("actual live config drift triggered %d automatic applies, want 1", applies) + } + + // A failed configuration read is an outage, not missing hardware or a + // deleted VM. Get/List fail, reconciliation does not enqueue mutations, + // and a name-scoped Watch terminates as unavailable instead of DELETED. + mu.Lock() + configUnavailable = true + mu.Unlock() + if _, err := client.Get(ctx, &apiv1.GetRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind, Name: desired.Metadata.Name}); status.Code(err) != codes.Internal { + t.Fatalf("configuration outage on Get: %v", err) + } + if _, err := client.List(ctx, &apiv1.ListRequest{ApiVersion: desired.APIVersion, Kind: desired.Kind}); status.Code(err) != codes.Internal { + t.Fatalf("configuration outage on List: %v", err) + } + rec.ReconcileOnce(ctx) + if applies != 1 { + t.Fatal("unavailable observations must not trigger automatic apply") + } + if event, err := stream.Recv(); status.Code(err) != codes.Unavailable { + t.Fatalf("configuration outage on Watch: event=%v error=%v; want Unavailable, never DELETED", event, err) + } +} diff --git a/internal/controller/server/resource.go b/internal/controller/server/resource.go index 2e28544..edc0aba 100644 --- a/internal/controller/server/resource.go +++ b/internal/controller/server/resource.go @@ -191,7 +191,8 @@ func (h *resourceHandler) Get(ctx context.Context, req *apiv1.GetRequest) (*apiv return nil, status.Errorf(codes.Internal, "load applied manifest: %v", lerr) } if desired != nil { - out.Drift = computeDrift(desired.Spec, r.Spec) + desiredSpec, observedSpec := providers.DriftSpecs(p, r.Kind, desired.Spec, r.Spec) + out.Drift = computeDrift(desiredSpec, observedSpec) applied, perr := resourceToProto(desired) if perr != nil { return nil, status.Errorf(codes.Internal, "encode applied: %v", perr) @@ -824,7 +825,12 @@ func (h *resourceHandler) attachDrift(ctx context.Context, out *apiv1.Resource, if desired == nil { return nil } - out.Drift = computeDrift(desired.Spec, observed.Spec) + p, err := h.registry.For(observed.APIVersion) + if err != nil { + return err + } + desiredSpec, observedSpec := providers.DriftSpecs(p, observed.Kind, desired.Spec, observed.Spec) + out.Drift = computeDrift(desiredSpec, observedSpec) return nil } diff --git a/internal/controller/server/watch.go b/internal/controller/server/watch.go index 461dfcc..8bf1311 100644 --- a/internal/controller/server/watch.go +++ b/internal/controller/server/watch.go @@ -11,6 +11,7 @@ import ( "github.com/openctl/openctl/internal/controller/manifests" "github.com/openctl/openctl/internal/controller/operations" + "github.com/openctl/openctl/internal/controller/providers" apiv1 "github.com/openctl/openctl/pkg/api/v1" "github.com/openctl/openctl/pkg/protocol" ) @@ -168,8 +169,11 @@ func (h *resourceHandler) listForWatch(ctx context.Context, apiVersion, kind, na if name != "" { r, err := p.Get(ctx, kind, name) if err != nil { - // NotFound is normal during watch — return empty, let DELETED fire. - return nil, nil // #nosec — intentional: surface as DELETED + var notFound *providers.NotFoundError + if errors.As(err, ¬Found) { + return nil, nil + } + return nil, err } if r == nil { return nil, nil diff --git a/pkg/proxmox/client/client.go b/pkg/proxmox/client/client.go index ba852cd..4919de1 100644 --- a/pkg/proxmox/client/client.go +++ b/pkg/proxmox/client/client.go @@ -15,6 +15,7 @@ import ( "net/url" "os" "sort" + "strconv" "strings" "time" @@ -232,18 +233,66 @@ type VM struct { // VMConfig represents VM configuration type VMConfig struct { - Name string `json:"name"` - Cores int `json:"cores"` - Sockets int `json:"sockets"` - Memory int `json:"memory"` - Boot string `json:"boot"` - OSType string `json:"ostype"` - SCSI0 string `json:"scsi0"` - Net0 string `json:"net0"` - IDE2 string `json:"ide2"` - IPConfig0 string `json:"ipconfig0"` - CIUser string `json:"ciuser"` - SSHKeys string `json:"sshkeys"` + Name string `json:"name"` + Cores int `json:"cores"` + Sockets int `json:"sockets"` + Memory int `json:"memory"` + Boot string `json:"boot"` + OSType string `json:"ostype"` + SCSI0 string `json:"scsi0"` + Net0 string `json:"net0"` + IDE2 string `json:"ide2"` + IPConfig0 string `json:"ipconfig0"` + CIUser string `json:"ciuser"` + SSHKeys string `json:"sshkeys"` + Raw map[string]any `json:"-"` +} + +// UnmarshalJSON preserves the full configuration while accepting Proxmox's +// integer fields as either JSON integers or decimal integer strings. +func (config *VMConfig) UnmarshalJSON(data []byte) error { + type plainVMConfig VMConfig + var decoded struct { + plainVMConfig + Cores json.RawMessage `json:"cores"` + Sockets json.RawMessage `json:"sockets"` + Memory json.RawMessage `json:"memory"` + } + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + if err := json.Unmarshal(data, &decoded.Raw); err != nil { + return err + } + if decoded.Raw == nil { + return errors.New("VM config must be a JSON object") + } + for _, field := range []struct { + name string + value json.RawMessage + dest *int + }{ + {"cores", decoded.Cores, &decoded.plainVMConfig.Cores}, + {"sockets", decoded.Sockets, &decoded.plainVMConfig.Sockets}, + {"memory", decoded.Memory, &decoded.plainVMConfig.Memory}, + } { + if len(field.value) == 0 { + continue + } + text := string(field.value) + if field.value[0] == '"' { + if err := json.Unmarshal(field.value, &text); err != nil { + return fmt.Errorf("invalid VM config %s: %w", field.name, err) + } + } + value, err := strconv.ParseInt(text, 10, strconv.IntSize) + if err != nil { + return fmt.Errorf("invalid VM config %s: %w", field.name, err) + } + *field.dest = int(value) + } + *config = VMConfig(decoded.plainVMConfig) + return nil } // Template represents a VM template @@ -312,13 +361,16 @@ func (c *Client) GetVMConfig(ctx context.Context, node string, vmid int) (*VMCon } var result struct { - Data VMConfig `json:"data"` + Data *VMConfig `json:"data"` } if err := json.Unmarshal(resp, &result); err != nil { return nil, fmt.Errorf("failed to parse VM config: %w", err) } - return &result.Data, nil + if result.Data == nil { + return nil, errors.New("missing VM config data") + } + return result.Data, nil } // CreateVM creates a new VM with the given parameters @@ -475,10 +527,8 @@ func (c *Client) ConfigureVM(ctx context.Context, node string, vmid int, params return err } -// GetVMConfigRaw returns the full VM config as a generic map. Unlike -// GetVMConfig (which uses a struct that only models a fixed subset of -// keys), this surfaces all disk and network slots that the VM actually -// has — necessary when merging flags into an arbitrary disk entry. +// GetVMConfigRaw returns the full VM config as a generic map without decoding +// typed fields, for callers merging flags into arbitrary disk entries. func (c *Client) GetVMConfigRaw(ctx context.Context, node string, vmid int) (map[string]any, error) { path := fmt.Sprintf("/api2/json/nodes/%s/qemu/%d/config", node, vmid) resp, err := c.get(ctx, path) diff --git a/pkg/proxmox/client/client_test.go b/pkg/proxmox/client/client_test.go index 6043561..c74d612 100644 --- a/pkg/proxmox/client/client_test.go +++ b/pkg/proxmox/client/client_test.go @@ -267,6 +267,99 @@ func TestGetVMConfig(t *testing.T) { } } +func TestGetVMConfigNumericCompatibility(t *testing.T) { + for _, numbers := range []string{ + `"cores":8,"sockets":1,"memory":10240`, + `"cores":"8","sockets":"1","memory":"10240"`, + `"cores":8,"sockets":1,"memory":"10240"`, + } { + t.Run(numbers, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api2/json/nodes/pve1/qemu/101/config" { + t.Errorf("unexpected request: %s", r.URL.Path) + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte(`{"data":{` + numbers + `,"name":"dev-system","scsi1":"local-lvm:vm-101-disk-1,size=300G","net2":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","ipconfig2":"ip=dhcp","hostpci3":"0000:01:00.0","agent":"1"}}`)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + config, err := c.GetVMConfig(context.Background(), "pve1", 101) + if err != nil { + t.Fatalf("GetVMConfig: %v", err) + } + if config.Cores != 8 || config.Sockets != 1 || config.Memory != 10240 || config.Name != "dev-system" { + t.Fatalf("decoded config = %+v", config) + } + for key, want := range map[string]any{ + "scsi1": "local-lvm:vm-101-disk-1,size=300G", + "net2": "virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0", + "ipconfig2": "ip=dhcp", + "hostpci3": "0000:01:00.0", + "agent": "1", + } { + if got := config.Raw[key]; got != want { + t.Errorf("Raw[%q] = %v, want %v", key, got, want) + } + } + if len(config.Raw) != 9 { + t.Errorf("Raw has %d fields, want 9", len(config.Raw)) + } + encoded, err := json.Marshal(config) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(encoded), `"Raw"`) || strings.Contains(string(encoded), `"hostpci3"`) { + t.Errorf("Raw leaked into encoded typed config: %s", encoded) + } + }) + } +} + +func TestGetVMConfigRejectsInvalidNumbers(t *testing.T) { + for _, field := range []string{"cores", "sockets", "memory"} { + for _, value := range []string{ + `"bad"`, `""`, `"8.5"`, `8.5`, `8.0`, `1e3`, + `true`, `null`, `{}`, `[]`, `" 8"`, `"8 "`, + `9223372036854775808`, `"9223372036854775808"`, + `-9223372036854775809`, `"-9223372036854775809"`, + } { + t.Run(field+"/"+value, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"data":{"` + field + `":` + value + `}}`)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + config, err := c.GetVMConfig(context.Background(), "pve1", 101) + if err == nil || config != nil { + t.Fatalf("expected decoding failure, got config=%+v, err=%v", config, err) + } + if !strings.Contains(err.Error(), field) { + t.Errorf("error does not identify %s: %v", field, err) + } + }) + } + } +} + +func TestGetVMConfigRejectsMissingData(t *testing.T) { + for _, response := range []string{`{}`, `{"data":null}`, `{"data":[]}`, `{"data":""}`} { + t.Run(response, func(t *testing.T) { + server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(response)) + }) + defer server.Close() + c := New(server.URL, "test", "test") + c.httpClient = server.Client() + if config, err := c.GetVMConfig(context.Background(), "pve1", 101); err == nil || config != nil { + t.Fatalf("expected failure, got config=%+v, err=%v", config, err) + } + }) + } +} + func TestListTemplates(t *testing.T) { server := mockServer(t, func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { diff --git a/pkg/proxmox/handler/handler.go b/pkg/proxmox/handler/handler.go index c531a60..80c46c5 100644 --- a/pkg/proxmox/handler/handler.go +++ b/pkg/proxmox/handler/handler.go @@ -208,7 +208,11 @@ func (h *Handler) listVMs(ctx context.Context) (*protocol.Response, error) { if vm.Template == 1 { continue } - result = append(result, resources.VMToResource(vm, nil)) + config, err := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + if err != nil { + return nil, fmt.Errorf("read VM %q config: %w", vm.Name, err) + } + result = append(result, resources.VMToResourceWithIP(vm, config, "")) } return &protocol.Response{ @@ -228,7 +232,10 @@ func (h *Handler) getVM(ctx context.Context, name string) (*protocol.Response, e return nil, err } - config, _ := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + config, err := h.client.GetVMConfig(ctx, vm.Node, vm.VMID) + if err != nil { + return nil, fmt.Errorf("read VM %q config: %w", vm.Name, err) + } // Try to get IP if VM is running (non-blocking) var ip string diff --git a/pkg/proxmox/handler/handler_test.go b/pkg/proxmox/handler/handler_test.go index 6f757d6..b197fec 100644 --- a/pkg/proxmox/handler/handler_test.go +++ b/pkg/proxmox/handler/handler_test.go @@ -767,3 +767,120 @@ func TestCreateVMFromImage_MissingFile(t *testing.T) { t.Errorf("expected error about missing file, got: %s", resp.Error.Message) } } + +func TestVMObservationUsesConfiguredHardware(t *testing.T) { + for _, action := range []string{protocol.ActionGet, protocol.ActionList} { + t.Run(action, func(t *testing.T) { + configReads, guestReads := 0, 0 + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api2/json/nodes": + _, _ = io.WriteString(w, `{"data":[{"node":"pve1"}]}`) + case "/api2/json/nodes/pve1/qemu": + // Runtime usage and vCPU count are deliberately different from config. + _, _ = io.WriteString(w, `{"data":[{"vmid":101,"name":"dev-system","status":"running","cpus":16,"maxmem":2147483648,"mem":1048576},{"vmid":9000,"name":"template","template":1}]}`) + case "/api2/json/nodes/pve1/qemu/101/config": + configReads++ + _, _ = io.WriteString(w, `{"data":{"cores":8,"sockets":"1","memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","agent":"1","ciuser":"ubuntu","sshkeys":"ssh-ed25519%20AAAA%20test","ipconfig0":"ip=dhcp"}}`) + case "/api2/json/nodes/pve1/qemu/101/agent/network-get-interfaces": + guestReads++ + http.Error(w, "guest agent unavailable", http.StatusInternalServerError) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + http.NotFound(w, r) + } + })) + defer server.Close() + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s"}) + resp, err := h.Handle(context.Background(), &protocol.Request{ + Action: action, ResourceType: "VirtualMachine", ResourceName: "dev-system", + }) + if err != nil { + t.Fatalf("Handle: %v", err) + } + if resp.Status != protocol.StatusSuccess { + t.Fatalf("expected success, got %+v", resp) + } + resource := resp.Resource + if action == protocol.ActionList { + if len(resp.Resources) != 1 { + t.Fatalf("listed %d resources, want one non-template VM", len(resp.Resources)) + } + resource = resp.Resources[0] + } + if resource == nil { + t.Fatal("missing observation") + } + cpu, ok := resource.Spec["cpu"].(map[string]any) + if !ok || cpu["cores"] != 8 || cpu["sockets"] != 1 { + t.Errorf("observed CPU = %v, want configured cores=8 sockets=1", resource.Spec["cpu"]) + } + memory, ok := resource.Spec["memory"].(map[string]any) + if !ok || memory["size"] != 10240 { + t.Errorf("observed memory = %v, want configured size=10240", resource.Spec["memory"]) + } + if configReads != 1 { + t.Errorf("config reads = %d, want exactly one", configReads) + } + if action == protocol.ActionList && guestReads != 0 { + t.Errorf("List made %d guest-agent calls, want zero", guestReads) + } + if _, ok := resource.Status["ip"]; ok { + t.Errorf("unexpected guest IP: %v", resource.Status["ip"]) + } + }) + } +} + +func TestVMObservationPropagatesConfigFailure(t *testing.T) { + for _, action := range []string{protocol.ActionGet, protocol.ActionList} { + for _, failure := range []string{"api", "decode"} { + t.Run(action+"/"+failure, func(t *testing.T) { + configReads, guestReads := 0, 0 + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api2/json/nodes": + _, _ = io.WriteString(w, `{"data":[{"node":"pve1"}]}`) + case "/api2/json/nodes/pve1/qemu": + _, _ = io.WriteString(w, `{"data":[{"vmid":100,"name":"healthy","status":"stopped"},{"vmid":101,"name":"dev-system","status":"running"}]}`) + case "/api2/json/nodes/pve1/qemu/100/config": + configReads++ + _, _ = io.WriteString(w, `{"data":{"cores":2,"sockets":1,"memory":2048}}`) + case "/api2/json/nodes/pve1/qemu/101/config": + configReads++ + if failure == "api" { + http.Error(w, "config unavailable", http.StatusInternalServerError) + } else { + _, _ = io.WriteString(w, `{"data":{"cores":8,"memory":"invalid"}}`) + } + default: + guestReads++ + t.Errorf("unexpected request after config failure: %s", r.URL.Path) + http.NotFound(w, r) + } + })) + defer server.Close() + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s"}) + resp, err := h.Handle(context.Background(), &protocol.Request{ + Action: action, ResourceType: "VirtualMachine", ResourceName: "dev-system", + }) + if err == nil || resp != nil { + t.Fatalf("expected error and no partial observation, got resp=%+v, err=%v", resp, err) + } + if !strings.Contains(err.Error(), "dev-system") { + t.Errorf("error does not identify VM: %v", err) + } + wantReads := 1 + if action == protocol.ActionList { + wantReads = 2 + } + if configReads != wantReads { + t.Errorf("config reads = %d, want %d", configReads, wantReads) + } + if guestReads != 0 { + t.Errorf("guest reads after config failure = %d, want zero", guestReads) + } + }) + } + } +} diff --git a/pkg/proxmox/handler/resize.go b/pkg/proxmox/handler/resize.go index 1827e1a..24b7381 100644 --- a/pkg/proxmox/handler/resize.go +++ b/pkg/proxmox/handler/resize.go @@ -12,8 +12,9 @@ import ( // resizeVM applies in-place changes to an existing VM for the fields Proxmox // can update live: memory, CPU (cores/sockets), and disk growth. Other spec -// differences (template, networks, cloud-init) are NOT touched here — changing -// them still requires delete + re-apply, and they surface as drift via Get. +// differences in networks and native cloud-init settings are not applied here +// and remain visible as drift. Creation-only inputs are not live-state +// comparison targets; changing their effects requires replacement. // // This realizes the CONTROLLER.md decision to update atomic resources in place // for resizable fields rather than no-op. Idempotent: re-applying an unchanged diff --git a/pkg/proxmox/resources/vm.go b/pkg/proxmox/resources/vm.go index c86f099..a106f4d 100644 --- a/pkg/proxmox/resources/vm.go +++ b/pkg/proxmox/resources/vm.go @@ -21,13 +21,7 @@ func VMToResourceWithIP(vm *client.VM, config *client.VMConfig, ip string) *prot } if config != nil { - spec["cpu"] = map[string]any{ - "cores": config.Cores, - "sockets": config.Sockets, - } - spec["memory"] = map[string]any{ - "size": config.Memory, - } + observeVMConfig(spec, config) } else { spec["cpu"] = map[string]any{ "cores": vm.CPUs, diff --git a/pkg/proxmox/resources/vm_compare.go b/pkg/proxmox/resources/vm_compare.go new file mode 100644 index 0000000..3ff699a --- /dev/null +++ b/pkg/proxmox/resources/vm_compare.go @@ -0,0 +1,194 @@ +package resources + +import ( + "encoding/json" + "math/big" + "strconv" + "strings" +) + +// ComparableVMSpecs projects live configuration onto explicitly managed fields. +// Only creation/routing and unobservable cloud-init execution inputs are +// excluded. Unknown desired fields remain managed, so missing observations drift. +// Neither input is modified. +func ComparableVMSpecs(desired, observed map[string]any) (map[string]any, map[string]any) { + d, o := compareVMMaps(desired, observed, "") + return d, o +} + +func compareVMMaps(desired, observed map[string]any, path string) (map[string]any, map[string]any) { + d, o := make(map[string]any, len(desired)), make(map[string]any, len(desired)) + for key, value := range desired { + if path == "" { + switch key { + case "context", "template", "image", "cloudImage", "startOnCreate": + continue + } + } + if path == "cloudInit" { + switch key { + case "password", "packages", "runcmd": + continue + } + } + child := key + if path != "" { + child = path + "." + key + } + live, exists := observed[key] + if m, ok := value.(map[string]any); ok { + liveMap, _ := live.(map[string]any) + dm, om := compareVMMaps(m, liveMap, child) + // An execution-only cloud-init declaration manages no native state. + if child == "cloudInit" && len(dm) == 0 { + continue + } + d[key] = dm + if exists { + o[key] = om + } + continue + } + if list, ok := value.([]any); ok && (child == "disks" || child == "networks" || child == "hostPCI") { + liveList, _ := live.([]any) + dl, ol := compareVMLists(list, liveList, child) + d[key] = dl + if exists { + o[key] = ol + } + continue + } + if text, ok := value.(string); ok && live == text { + d[key], o[key] = value, live + continue + } + d[key] = normalizedVMValue(value, child) + if exists { + o[key] = normalizedVMValue(live, child) + } + } + return d, o +} + +func compareVMLists(desired, observed []any, path string) ([]any, []any) { + d, o := make([]any, 0, len(desired)), make([]any, 0, len(desired)) + for i, value := range desired { + m, ok := value.(map[string]any) + if !ok { + d = append(d, normalizedVMValue(value, path)) + if i < len(observed) { + o = append(o, normalizedVMValue(observed[i], path)) + } + continue + } + var live map[string]any + if path == "hostPCI" { + if i < len(observed) { + live, _ = observed[i].(map[string]any) + } + } else if name, ok := m["name"].(string); ok { + for _, candidate := range observed { + if candidateMap, ok := candidate.(map[string]any); ok && candidateMap["name"] == name { + live = candidateMap + break + } + } + } + dm, om := compareVMMaps(m, live, path) + d = append(d, dm) + if live != nil { + o = append(o, om) + } else { + o = append(o, nil) + } + } + return d, o +} + +func normalizedVMValue(value any, path string) any { + switch v := value.(type) { + case map[string]any: + m := make(map[string]any, len(v)) + for key, value := range v { + m[key] = normalizedVMValue(value, path+"."+key) + } + return m + case []any: + list := make([]any, len(v)) + for i, value := range v { + list[i] = normalizedVMValue(value, path) + } + return list + case []string: + list := make([]any, len(v)) + for i, value := range v { + list[i] = value + } + return list + case string: + if path == "disks.size" { + return binaryDiskSize(v) + } + if path == "networks.macAddress" { + return strings.ToLower(v) + } + return v + case int: + return float64(v) + case int64: + return float64(v) + case int32: + return float64(v) + case uint: + return float64(v) + case uint64: + return float64(v) + case float32: + return float64(v) + case json.Number: + if n, err := strconv.ParseFloat(string(v), 64); err == nil { + return n + } + } + return value +} + +// Proxmox disk sizes use binary units even for the G/T spellings. Rational +// arithmetic preserves fractional sizes without float rounding or overflow. +func binaryDiskSize(value string) string { + s := strings.ToUpper(strings.TrimSpace(value)) + i := 0 + for i < len(s) && ((s[i] >= '0' && s[i] <= '9') || s[i] == '.') { + i++ + } + if i == 0 { + return value + } + unit := s[i:] + power := 0 + switch unit { + case "", "B": + case "K", "KB", "KIB": + power = 1 + case "M", "MB", "MIB": + power = 2 + case "G", "GB", "GIB": + power = 3 + case "T", "TB", "TIB": + power = 4 + case "P", "PB", "PIB": + power = 5 + default: + return value + } + if whole, err := strconv.ParseUint(s[:i], 10, 64); err == nil && whole <= ^uint64(0)>>uint(10*power) { + return strconv.FormatUint(whole< 0 && key[i-1] >= '0' && key[i-1] <= '9' { + i-- + } + n, _ := strconv.Atoi(key[i:]) + return key[:i], n +} + +func numberedSlot(key string, prefixes ...string) bool { + prefix, _ := slotIndex(key) + if prefix == key { + return false + } + for _, candidate := range prefixes { + if prefix == candidate { + return true + } + } + return false +} diff --git a/pkg/proxmox/resources/vm_observation_test.go b/pkg/proxmox/resources/vm_observation_test.go new file mode 100644 index 0000000..b766ad3 --- /dev/null +++ b/pkg/proxmox/resources/vm_observation_test.go @@ -0,0 +1,144 @@ +package resources + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/openctl/openctl/pkg/proxmox/client" +) + +func observedConfig(t *testing.T, raw string) map[string]any { + t.Helper() + var config client.VMConfig + if err := json.Unmarshal([]byte(raw), &config); err != nil { + t.Fatal(err) + } + return VMToResourceWithIP(&client.VM{Name: "dev-system", Node: "pve", CPUs: 99, MaxMem: 1}, &config, "192.0.2.1").Spec +} + +func TestNativeVMConfigurationComparesWithoutCreationDrift(t *testing.T) { + observed := observedConfig(t, `{"cores":8,"sockets":1,"memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G","ide2":"local-lvm:vm-101-cloudinit,media=cdrom","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","agent":"1","ciuser":"ubuntu","sshkeys":"ssh-ed25519%20AAA+BBB%20user%40host%0A","ipconfig0":"ip=dhcp","cipassword":"SECRET"}`) + desired := map[string]any{ + "context": "lab", "template": map[string]any{"name": "ubuntu"}, "image": map[string]any{"file": "ubuntu.img"}, "cloudImage": map[string]any{"url": "https://example.test/image"}, "startOnCreate": true, + "node": "pve", "cpu": map[string]any{"cores": float64(8), "sockets": float64(1)}, "memory": map[string]any{"size": float64(10240)}, + "disks": []any{map[string]any{"name": "scsi0", "storage": "local-lvm", "size": "300GiB"}}, + "networks": []any{map[string]any{"name": "net0", "bridge": "vmbr0", "model": "virtio", "macAddress": "aa:bb:cc:dd:ee:ff", "firewall": false}}, + "agent": map[string]any{"enabled": true}, + "cloudInit": map[string]any{"user": "ubuntu", "sshKeys": []any{"ssh-ed25519 AAA+BBB user@host"}, "ipConfig": map[string]any{"net0": map[string]any{"ip": "dhcp"}}, "password": "SECRET", "packages": []any{"curl"}, "runcmd": []any{"echo hello"}}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("unexpected drift: desired=%#v observed=%#v", d, o) + } + ci := observed["cloudInit"].(map[string]any) + if _, ok := ci["password"]; ok { + t.Fatal("write-only password exposed") + } + if len(observed["disks"].([]any)) != 1 { + t.Fatal("cloud-init CD-ROM exposed as data disk") + } + if _, ok := observed["template"]; ok { + t.Fatal("creation state exposed") + } + if desired["cloudInit"].(map[string]any)["password"] != "SECRET" { + t.Fatal("comparison mutated desired state") + } + if observed["cpu"].(map[string]any)["cores"] != 8 { + t.Fatal("comparison mutated observation") + } +} + +func TestNativeVMDriftRemainsVisible(t *testing.T) { + observed := observedConfig(t, `{"cores":"8","sockets":"1","memory":"10240","scsi0":"local-lvm:vm-101-disk-0,size=300G,backup=0,ssd=1,discard=on,iothread=1","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0,tag=20,firewall=1","agent":"enabled=1,fstrim_cloned_disks=1","ciuser":"ubuntu","ciupgrade":1,"ipconfig0":"ip=dhcp","sshkeys":"ssh-ed25519%20AAA+BBB"}`) + cases := []map[string]any{ + {"cpu": map[string]any{"cores": 4}}, {"cpu": map[string]any{"sockets": 2}}, {"cpu": map[string]any{"type": "host"}}, + {"memory": map[string]any{"size": 2048}}, {"agent": map[string]any{"enabled": false}}, + {"disks": []any{map[string]any{"name": "scsi0", "size": "301G"}}}, + {"disks": []any{map[string]any{"name": "scsi1", "size": "300G"}}}, + {"disks": []any{map[string]any{"name": "scsi0", "backup": true}}}, + {"disks": []any{map[string]any{"name": "scsi0", "ssd": false, "discard": false, "iothread": false}}}, + {"networks": []any{map[string]any{"name": "net0", "firewall": false}}}, + {"networks": []any{map[string]any{"name": "net0", "bridge": "vmbr1"}}}, + {"networks": []any{map[string]any{"name": "net0", "vlan": 21}}}, + {"networks": []any{map[string]any{"name": "net1", "bridge": "vmbr0"}}}, + {"cloudInit": map[string]any{"user": "root"}}, {"cloudInit": map[string]any{"packageUpgrade": false}}, + {"cloudInit": map[string]any{"sshKeys": []any{"ssh-ed25519 CHANGED"}}}, + {"cloudInit": map[string]any{"ipConfig": map[string]any{"net0": map[string]any{"ip": "192.0.2.5/24"}}}}, + {"cloudInit": map[string]any{"ipConfig": map[string]any{"net1": map[string]any{"ip": "dhcp"}}}}, + {"futureManagedOption": true}, {"cpu": map[string]any{"futureManagedOption": true}}, + } + for i, desired := range cases { + d, o := ComparableVMSpecs(desired, observed) + if reflect.DeepEqual(d, o) { + t.Errorf("case %d lost drift for %#v", i, desired) + } + } +} + +func TestVMNamedSlotsAndFractionalDiskSizes(t *testing.T) { + observed := observedConfig(t, `{"scsi12":"fast:vm-101-disk-12,size=1.5T","scsi2":"slow:vm-101-disk-2,size=1G","net12":"virtio=AA:BB:CC:DD:EE:12,bridge=vmbr12","net2":"virtio=AA:BB:CC:DD:EE:02,bridge=vmbr2","ipconfig12":"ip=192.0.2.12/24,gw=192.0.2.1","hostpci0":"mapping=gpu,pcie=1,x-vga=1,rombar=0,mdev=nvidia-123,romfile=gpu.rom","efidisk0":"fast:vm-101-disk-0,efitype=4m,pre-enrolled-keys=1","bios":"ovmf","machine":"q35","ostype":"l26","cpu":"host,flags=+aes","nameserver":"1.1.1.1 8.8.8.8","searchdomain":"example.test"}`) + desired := map[string]any{ + "disks": []any{map[string]any{"name": "scsi12", "size": "1536GiB"}, map[string]any{"name": "scsi2", "size": "1024M"}}, + "networks": []any{map[string]any{"name": "net12", "bridge": "vmbr12"}}, + "hostPCI": []any{map[string]any{"mapping": "gpu", "pcie": true, "primaryGPU": true, "rombar": false, "mdev": "nvidia-123", "romfile": "gpu.rom"}}, + "efiDisk": map[string]any{"storage": "fast", "type": "4m", "preEnrolledKeys": true}, + "bios": "ovmf", "machine": "q35", "osType": "l26", "cpu": map[string]any{"type": "host"}, + "cloudInit": map[string]any{"ipConfig": map[string]any{"net12": map[string]any{"ip": "192.0.2.12/24", "gateway": "192.0.2.1"}}, "searchDomain": "example.test", "nameservers": []any{"1.1.1.1", "8.8.8.8"}}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("slot ordering or native observation caused drift: %#v != %#v", d, o) + } + for _, field := range []string{"bios", "machine", "osType", "hostPCI", "efiDisk"} { + missing := make(map[string]any, len(observed)) + for key, value := range observed { + if key != field { + missing[key] = value + } + } + d, o := ComparableVMSpecs(desired, missing) + if reflect.DeepEqual(d, o) { + t.Errorf("missing %s lost drift", field) + } + } +} + +func TestVMSSHKeyDecodingDoesNotEraseMalformedOrLiteralPlus(t *testing.T) { + for _, key := range []string{"ssh-ed25519 AAA+BBB", "ssh-ed25519 AAA%invalid"} { + raw, err := json.Marshal(map[string]any{"sshkeys": key}) + if err != nil { + t.Fatal(err) + } + observed := observedConfig(t, string(raw)) + keys := observed["cloudInit"].(map[string]any)["sshKeys"].([]any) + if len(keys) != 1 || keys[0] != key { + t.Fatalf("public key changed: %v", keys) + } + } +} + +func TestVMUnmanagedDefaultsAndExplicitFalseOptions(t *testing.T) { + observed := observedConfig(t, `{"scsi0":"local-lvm:vm-101-disk-0,size=8G","net0":"virtio=AA:BB:CC:DD:EE:FF,bridge=vmbr0","hostpci2":"host=0000:01:00.0"}`) + desired := map[string]any{ + "disks": []any{map[string]any{"name": "scsi0", "ssd": false, "discard": false, "iothread": false, "backup": true}}, + "networks": []any{map[string]any{"name": "net0", "firewall": false, "vlan": 0}}, + "agent": map[string]any{"enabled": false}, + } + d, o := ComparableVMSpecs(desired, observed) + if !reflect.DeepEqual(d, o) { + t.Fatalf("native default flags caused drift: %#v != %#v", d, o) + } + d, o = ComparableVMSpecs(map[string]any{"disks": []any{}, "networks": []any{}}, observed) + if !reflect.DeepEqual(d, o) { + t.Fatal("undeclared live members became managed") + } + d, o = ComparableVMSpecs(map[string]any{"hostPCI": []any{map[string]any{"device": "0000:01:00.0"}}}, observed) + if reflect.DeepEqual(d, o) { + t.Fatal("hostpci2 incorrectly satisfied desired hostpci0") + } + pci := observed["hostPCI"].([]any) + if len(pci) != 3 || pci[2].(map[string]any)["device"] != "0000:01:00.0" { + t.Fatal("numbered PCI slot was lost") + } +} From 49ba3ccca37a042fc1357974b574f5e620638ad3 Mon Sep 17 00:00:00 2001 From: Jeremy Harrington Date: Thu, 8 Oct 2026 16:59:57 -0700 Subject: [PATCH 2/3] fix(ci): satisfy Go modernization checks Use CutPrefix, SplitSeq, FieldsSeq, and slices.Contains in native VM observation. Replace the existing graph capacity expression with equivalent slices.Clip to satisfy the current analyzer. Verified Proxmox, operation graph, and controller server regressions. --- internal/controller/operations/graph.go | 3 ++- pkg/proxmox/resources/vm_observation.go | 16 ++++++---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/internal/controller/operations/graph.go b/internal/controller/operations/graph.go index 81c4141..3553255 100644 --- a/internal/controller/operations/graph.go +++ b/internal/controller/operations/graph.go @@ -3,6 +3,7 @@ package operations import ( "context" "fmt" + "slices" "sort" "strings" ) @@ -115,7 +116,7 @@ func RunGraph(ctx context.Context, concurrency int, tasks []Task) error { continue // drain remaining in-flight, launch nothing new } - newlyReady := ready[:len(ready):len(ready)] + newlyReady := slices.Clip(ready) for _, dep := range dependents[res.id] { indegree[dep]-- if indegree[dep] == 0 { diff --git a/pkg/proxmox/resources/vm_observation.go b/pkg/proxmox/resources/vm_observation.go index 39b8e23..acb38e1 100644 --- a/pkg/proxmox/resources/vm_observation.go +++ b/pkg/proxmox/resources/vm_observation.go @@ -3,6 +3,7 @@ package resources import ( "fmt" "net/url" + "slices" "sort" "strconv" "strings" @@ -33,8 +34,8 @@ func observeVMConfig(spec map[string]any, config *client.VMConfig) { memory = 512 } cpu, _ := configParts(text("cpu", "kvm64")) - if strings.HasPrefix(cpu, "cputype=") { - cpu = strings.TrimPrefix(cpu, "cputype=") + if after, ok := strings.CutPrefix(cpu, "cputype="); ok { + cpu = after } spec["cpu"] = map[string]any{"cores": cores, "sockets": sockets, "type": cpu} spec["memory"] = map[string]any{"size": memory} @@ -130,7 +131,7 @@ func observeVMConfig(spec map[string]any, config *client.VMConfig) { keysText = decoded } sshKeys := []any{} - for _, key := range strings.Split(keysText, "\n") { + for key := range strings.SplitSeq(keysText, "\n") { if key = strings.TrimSpace(key); key != "" { sshKeys = append(sshKeys, key) } @@ -138,7 +139,7 @@ func observeVMConfig(spec map[string]any, config *client.VMConfig) { ci["sshKeys"] = sshKeys ci["searchDomain"] = text("searchdomain", "") nameservers := []any{} - for _, server := range strings.Fields(text("nameserver", "")) { + for server := range strings.FieldsSeq(text("nameserver", "")) { nameservers = append(nameservers, server) } ci["nameservers"] = nameservers @@ -184,10 +185,5 @@ func numberedSlot(key string, prefixes ...string) bool { if prefix == key { return false } - for _, candidate := range prefixes { - if prefix == candidate { - return true - } - } - return false + return slices.Contains(prefixes, prefix) } From ff8ff0ed2e98f1cbe4ae2e9c078978c32e1389f8 Mon Sep 17 00:00:00 2001 From: Jeremy Harrington Date: Thu, 8 Oct 2026 17:03:26 -0700 Subject: [PATCH 3/3] test(manifests): stop periodic pulls before fixture cleanup Keep the pull context alive until cleanup acquires the repository lock, then cancel while no git command can be in flight. This prevents temporary directory removal from racing background writes without changing GitOps production behavior. Verified the periodic-pull scenario with 20 race-enabled runs. --- internal/controller/manifests/git_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/controller/manifests/git_test.go b/internal/controller/manifests/git_test.go index 3a90f56..a862e27 100644 --- a/internal/controller/manifests/git_test.go +++ b/internal/controller/manifests/git_test.go @@ -327,7 +327,9 @@ func TestRepoStartPeriodicPullReconcilesRemoteCommits(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git binary not on PATH; skipping git-dependent test") } - ctx := t.Context() // canceled when the test ends, stopping the pull goroutine + // T.Context cancels before cleanup; wait for any git command before canceling. + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) base := t.TempDir() bare := filepath.Join(base, "remote.git") @@ -362,6 +364,14 @@ func TestRepoStartPeriodicPullReconcilesRemoteCommits(t *testing.T) { t.Fatalf("NewRepo: %v", err) } + t.Cleanup(func() { + if err := repo.lock.acquire(context.Background()); err != nil { + t.Errorf("wait for repository operations: %v", err) + return + } + cancel() + repo.lock.release() + }) var mu sync.Mutex changes := 0 repo.StartPeriodicPull(ctx, 30*time.Millisecond, func(context.Context) error {