diff --git a/ROADMAP.md b/ROADMAP.md index 9638cba..844d3ed 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -550,16 +550,21 @@ plan/state); harden the provider contract before the ecosystem widens. no-ops when the disk storage is `local-lvm` (LVM storages can't hold `snippets` content). Use a static `ip=…` or a snippet-capable storage until the QGA-install path is hardened (follow-up below). -- [~] **Bootstrap QGA robustness (follow-up).** `EnsureQemuAgentSnippet` - needs a `snippets`-capable storage; on a `local-lvm`-only node it - no-ops, so DHCP-mode bootstrap can't discover the VM's IP. *Done:* the - DHCP IP-wait timeout now fails with an **actionable** error naming the - likely cause (qemu-guest-agent not running) and the static-IP - workaround, instead of a bare timeout. *Remaining (the real fix):* - install qemu-guest-agent without depending on the disk storage — pick a - snippets-capable storage automatically (the proxmox client now has - `ListNodeStorages`), or fail fast at create time when no such storage - exists rather than after a 10-minute IP-wait. +- [x] **Bootstrap QGA robustness (follow-up) — FIXED.** `EnsureQemuAgentSnippet` + needed a `snippets`-capable storage; on a `local-lvm`-only node it + no-op'd, so DHCP-mode bootstrap couldn't discover the VM's IP. *Earlier:* + the DHCP IP-wait timeout fails with an **actionable** error naming the + likely cause + the static-IP workaround. *Now the real fix:* the + cloud-init snippet path **auto-selects a snippets-capable storage** — + `client.PickSnippetsStorage` (pure, unit-tested) prefers the configured + storage when it can hold snippets, else the node's first snippets-capable + one; the handler resolves it via `ListNodeStorages` before uploading. So a + node whose default/disk storage is LVM-only still gets its qemu-guest-agent + (and E1 packages/runcmd) snippet on `local`. When a VM **explicitly** + requested `packages`/`runcmd` and the node has **no** snippets-capable + storage, create now **fails fast** with a clear error instead of producing + a silently-broken node; the agent-only case still degrades gracefully. + Also hardens the E1 per-VM vendor snippet, which shares this path. - [x] **Bug: Cluster apply doesn't detect an out-of-band child-VM deletion — FIXED.** Surfaced 2026-07-07 recovering a k3s worker whose VM had been deleted outside openctl: Cluster `Get` reported `Ready` diff --git a/pkg/proxmox/client/client.go b/pkg/proxmox/client/client.go index 4792610..ba852cd 100644 --- a/pkg/proxmox/client/client.go +++ b/pkg/proxmox/client/client.go @@ -135,6 +135,42 @@ func (c *Client) ListNodeStorages(ctx context.Context, node string) ([]*NodeStor return out, nil } +// storageSupportsSnippets reports whether a storage lists the "snippets" +// content type — required to host cloud-init user/vendor-data. LVM-backed +// stores (local-lvm, etc.) can't; a "dir" store like "local" typically can. +func storageSupportsSnippets(s *NodeStorage) bool { + for c := range strings.SplitSeq(s.Content, ",") { + if strings.TrimSpace(c) == "snippets" { + return true + } + } + return false +} + +// PickSnippetsStorage chooses a storage that can hold cloud-init snippets from a +// node's storage list. It prefers `preferred` when that storage exists and is +// snippets-capable; otherwise it returns the first snippets-capable storage +// (sorted by ID for determinism). ok is false when the node has none — callers +// should fail fast at create time rather than after a long IP-discovery wait. +// Pure (no network) so it is unit-testable independent of the API. +func PickSnippetsStorage(storages []*NodeStorage, preferred string) (storage string, ok bool) { + var capable []string + for _, s := range storages { + if !storageSupportsSnippets(s) { + continue + } + if s.Storage == preferred { + return preferred, true + } + capable = append(capable, s.Storage) + } + if len(capable) == 0 { + return "", false + } + sort.Strings(capable) + return capable[0], true +} + // NodeBridge is a network bridge on a node, as returned by // /nodes//network?type=bridge. type NodeBridge struct { diff --git a/pkg/proxmox/client/storage_select_test.go b/pkg/proxmox/client/storage_select_test.go new file mode 100644 index 0000000..d352670 --- /dev/null +++ b/pkg/proxmox/client/storage_select_test.go @@ -0,0 +1,43 @@ +package client + +import "testing" + +func storages() []*NodeStorage { + return []*NodeStorage{ + {Storage: "local-lvm", Type: "lvmthin", Content: "images,rootdir", Active: 1}, + {Storage: "local", Type: "dir", Content: "snippets,vztmpl,iso", Active: 1}, + {Storage: "cephfs", Type: "cephfs", Content: "snippets,backup", Active: 1}, + } +} + +func TestPickSnippetsStorage_PrefersConfiguredWhenCapable(t *testing.T) { + got, ok := PickSnippetsStorage(storages(), "cephfs") + if !ok || got != "cephfs" { + t.Errorf("got (%q,%v), want (cephfs,true) — configured snippets-capable storage preferred", got, ok) + } +} + +func TestPickSnippetsStorage_FallsBackWhenPreferredNotCapable(t *testing.T) { + // Preferred is the LVM disk store (no snippets) → fall back to the first + // snippets-capable, sorted by ID: "cephfs" < "local". + got, ok := PickSnippetsStorage(storages(), "local-lvm") + if !ok || got != "cephfs" { + t.Errorf("got (%q,%v), want (cephfs,true) — first snippets-capable by ID", got, ok) + } +} + +func TestPickSnippetsStorage_FallsBackWhenNoPreferred(t *testing.T) { + got, ok := PickSnippetsStorage(storages(), "") + if !ok || got != "cephfs" { + t.Errorf("got (%q,%v), want (cephfs,true)", got, ok) + } +} + +func TestPickSnippetsStorage_NoneCapable(t *testing.T) { + only := []*NodeStorage{ + {Storage: "local-lvm", Type: "lvmthin", Content: "images,rootdir", Active: 1}, + } + if got, ok := PickSnippetsStorage(only, "local-lvm"); ok { + t.Errorf("got (%q,true), want (\"\",false) — LVM-only node has no snippets storage", got) + } +} diff --git a/pkg/proxmox/handler/handler.go b/pkg/proxmox/handler/handler.go index c64d3ba..c531a60 100644 --- a/pkg/proxmox/handler/handler.go +++ b/pkg/proxmox/handler/handler.go @@ -354,7 +354,9 @@ func (h *Handler) createVMFromTemplate(ctx context.Context, name, node string, s snippetStorage = s } } - h.applyCloudInitVendorSnippet(ctx, node, vmid, snippetStorage, spec.CloudInit) + if err := h.applyCloudInitVendorSnippet(ctx, node, vmid, snippetStorage, spec.CloudInit); err != nil { + return nil, err + } } if spec.StartOnCreate { @@ -375,34 +377,62 @@ func (h *Handler) createVMFromTemplate(ctx context.Context, name, node string, s // otherwise it uses the shared static qemu-agent snippet — the pre-existing, // metal-validated path, byte-identical. // -// Every failure is non-fatal and surfaced via debug logs: a storage that can't -// hold snippets (e.g. LVM) just means no agent/packages, not a failed create — -// matching the prior behavior. -func (h *Handler) applyCloudInitVendorSnippet(ctx context.Context, node string, vmid int, snippetStorage string, ci *resources.CloudInitSpec) { +// The snippet storage is resolved automatically: `preferredStorage` is used +// when it can hold snippets, otherwise the node's first snippets-capable +// storage — so a node whose default/disk storage is LVM-only (which can't hold +// snippets) still works, and the qemu-guest-agent snippet lands instead of +// silently no-op'ing (which stalls DHCP-mode IP discovery for ~10 minutes). +// +// Failure handling depends on intent: +// - When the VM explicitly requested packages/runcmd, any failure (no +// snippets-capable storage, upload, config) is FATAL and returned — the +// user asked for node prep we can't deliver, so fail fast at create rather +// than produce a silently-broken node. +// - For the best-effort qemu-guest-agent-only case (no packages/runcmd), a +// failure degrades gracefully (logged, nil) — matching the prior behavior. +func (h *Handler) applyCloudInitVendorSnippet(ctx context.Context, node string, vmid int, preferredStorage string, ci *resources.CloudInitSpec) error { + custom := ci != nil && (len(ci.Packages) > 0 || len(ci.RunCmd) > 0) + // fail returns err when the caller explicitly requested packages/runcmd, + // else swallows it (best-effort agent snippet). + fail := func(err error) error { + if custom { + return err + } + debugf("applyCloudInitVendorSnippet: %v (best-effort agent snippet skipped)", err) + return nil + } + + storages, err := h.client.ListNodeStorages(ctx, node) + if err != nil { + return fail(fmt.Errorf("list storages on %s: %w", node, err)) + } + storage, ok := client.PickSnippetsStorage(storages, preferredStorage) + if !ok { + return fail(fmt.Errorf("no snippets-capable storage on node %q (cloud-init needs a storage with the \"snippets\" content type, e.g. \"local\")", node)) + } + var snippetName string - if ci != nil && (len(ci.Packages) > 0 || len(ci.RunCmd) > 0) { + if custom { content, err := client.RenderVendorData(ci.Packages, ci.RunCmd) if err != nil { - debugf("applyCloudInitVendorSnippet: render vendor data: %v", err) - return + return fail(fmt.Errorf("render vendor data: %w", err)) } snippetName = client.VendorSnippetName(vmid) - if err := h.client.UploadSnippet(ctx, node, snippetStorage, snippetName, content); err != nil { - debugf("applyCloudInitVendorSnippet: upload vendor snippet: %v", err) - return + if err := h.client.UploadSnippet(ctx, node, storage, snippetName, content); err != nil { + return fail(fmt.Errorf("upload vendor snippet: %w", err)) } } else { - if err := h.client.EnsureQemuAgentSnippet(ctx, node, snippetStorage); err != nil { - debugf("applyCloudInitVendorSnippet: ensure agent snippet: %v", err) - return + if err := h.client.EnsureQemuAgentSnippet(ctx, node, storage); err != nil { + return fail(fmt.Errorf("ensure agent snippet: %w", err)) } snippetName = client.QemuAgentSnippetName } - cicustom := fmt.Sprintf("vendor=%s:snippets/%s", snippetStorage, snippetName) + cicustom := fmt.Sprintf("vendor=%s:snippets/%s", storage, snippetName) debugf("applyCloudInitVendorSnippet: setting cicustom=%s", cicustom) if err := h.client.ConfigureVM(ctx, node, vmid, map[string]any{"cicustom": cicustom}); err != nil { - debugf("applyCloudInitVendorSnippet: set cicustom: %v", err) + return fail(fmt.Errorf("set cicustom: %w", err)) } + return nil } func primaryDiskStorage(spec *resources.VMSpec) string { @@ -508,7 +538,9 @@ func (h *Handler) createVMFromCloudImage(ctx context.Context, name, node string, // detection, plus any requested packages/runcmd). Using vendor= (not // user=) so it merges with Proxmox's generated cloud-init user-data // instead of replacing it. - h.applyCloudInitVendorSnippet(ctx, node, vmid, spec.CloudImage.Storage, spec.CloudInit) + if err := h.applyCloudInitVendorSnippet(ctx, node, vmid, spec.CloudImage.Storage, spec.CloudInit); err != nil { + return nil, err + } // Regenerate cloud-init with new settings (error ignored - non-fatal) if spec.CloudInit != nil { diff --git a/pkg/proxmox/handler/handler_test.go b/pkg/proxmox/handler/handler_test.go index ecde259..6f757d6 100644 --- a/pkg/proxmox/handler/handler_test.go +++ b/pkg/proxmox/handler/handler_test.go @@ -363,6 +363,11 @@ func TestCreateVMUploadsPerVMVendorSnippet(t *testing.T) { }}) case r.URL.Path == "/api2/json/cluster/nextid" && r.Method == "GET": json.NewEncoder(w).Encode(map[string]any{"data": "200"}) + case r.URL.Path == "/api2/json/nodes/pve1/storage" && r.Method == "GET": + json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{ + {"storage": "local", "type": "dir", "content": "snippets,vztmpl,iso", "active": 1}, + {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "active": 1}, + }}) case r.URL.Path == "/api2/json/nodes/pve1/qemu/9000/clone" && r.Method == "POST": json.NewEncoder(w).Encode(map[string]any{"data": ""}) case r.URL.Path == "/api2/json/nodes/pve1/storage/local/upload" && r.Method == "POST": @@ -431,6 +436,112 @@ func TestCreateVMUploadsPerVMVendorSnippet(t *testing.T) { } } +// snippetHardeningServer is a create-path fake whose /storage response is +// configurable, so tests can exercise auto-selection and the no-snippets case. +// It records the upload target storage and the cicustom value. +func snippetHardeningServer(t *testing.T, storageData []map[string]any, uploadStorage *string, cicustom *string) *httptest.Server { + t.Helper() + return httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api2/json/nodes" && r.Method == "GET": + json.NewEncoder(w).Encode(map[string]any{"data": []map[string]string{{"node": "pve1"}}}) + case r.URL.Path == "/api2/json/nodes/pve1/qemu" && r.Method == "GET": + json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{ + {"vmid": 9000, "name": "ubuntu-template", "template": 1, "status": "stopped"}, + }}) + case r.URL.Path == "/api2/json/cluster/nextid" && r.Method == "GET": + json.NewEncoder(w).Encode(map[string]any{"data": "200"}) + case r.URL.Path == "/api2/json/nodes/pve1/storage" && r.Method == "GET": + json.NewEncoder(w).Encode(map[string]any{"data": storageData}) + case r.URL.Path == "/api2/json/nodes/pve1/qemu/9000/clone" && r.Method == "POST": + json.NewEncoder(w).Encode(map[string]any{"data": ""}) + case strings.HasSuffix(r.URL.Path, "/upload") && r.Method == "POST": + // /api2/json/nodes/pve1/storage//upload + parts := strings.Split(r.URL.Path, "/") + *uploadStorage = parts[len(parts)-2] + json.NewEncoder(w).Encode(map[string]any{"data": ""}) + case r.URL.Path == "/api2/json/nodes/pve1/qemu/200/config": + if err := r.ParseForm(); err != nil { + t.Fatalf("ParseForm: %v", err) + } + if c := r.Form.Get("cicustom"); c != "" { + *cicustom = c + } + json.NewEncoder(w).Encode(map[string]any{"data": ""}) + default: + t.Fatalf("unexpected request: %s %s", r.Method, r.URL.Path) + } + })) +} + +func createVMWithPackages(t *testing.T, h *Handler) (*protocol.Response, error) { + t.Helper() + return h.Handle(context.Background(), &protocol.Request{ + Version: protocol.ProtocolVersion, + Action: protocol.ActionCreate, + ResourceType: "VirtualMachine", + Manifest: &protocol.Resource{ + APIVersion: "proxmox.openctl.io/v1", + Kind: "VirtualMachine", + Metadata: protocol.ResourceMetadata{Name: "longhorn-node"}, + Spec: map[string]any{ + "node": "pve1", + "template": map[string]any{"name": "ubuntu-template"}, + "cloudInit": map[string]any{"packages": []any{"open-iscsi"}}, + }, + }, + }) +} + +// TestCreateVMAutoSelectsSnippetsStorage: the preferred/default storage is +// LVM-only (no snippets), so the vendor snippet must land on the node's +// snippets-capable storage instead of silently failing. +func TestCreateVMAutoSelectsSnippetsStorage(t *testing.T) { + var uploadStorage, cicustom string + server := snippetHardeningServer(t, []map[string]any{ + {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "active": 1}, + {"storage": "local", "type": "dir", "content": "snippets,vztmpl", "active": 1}, + }, &uploadStorage, &cicustom) + defer server.Close() + + // Default storage is the LVM one (can't hold snippets). + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s", Node: "pve1", + Defaults: map[string]string{"storage": "local-lvm"}}) + resp, err := createVMWithPackages(t, h) + if err != nil { + t.Fatalf("Handle: %v", err) + } + if resp.Status != protocol.StatusSuccess { + t.Fatalf("status = %s, resp=%+v", resp.Status, resp) + } + if uploadStorage != "local" { + t.Errorf("uploaded to %q, want local (auto-selected snippets-capable storage)", uploadStorage) + } + if cicustom != "vendor=local:snippets/openctl-vendor-200.yaml" { + t.Errorf("cicustom = %q, want vendor=local:snippets/openctl-vendor-200.yaml", cicustom) + } +} + +// TestCreateVMFailsFastWhenNoSnippetsStorage: packages/runcmd were explicitly +// requested but the node has no snippets-capable storage, so create must fail +// fast with an actionable error rather than produce a silently-broken node. +func TestCreateVMFailsFastWhenNoSnippetsStorage(t *testing.T) { + var uploadStorage, cicustom string + server := snippetHardeningServer(t, []map[string]any{ + {"storage": "local-lvm", "type": "lvmthin", "content": "images,rootdir", "active": 1}, + }, &uploadStorage, &cicustom) + defer server.Close() + + h := New(&protocol.ProviderConfig{Endpoint: server.URL, TokenID: "t", TokenSecret: "s", Node: "pve1"}) + _, err := createVMWithPackages(t, h) + if err == nil { + t.Fatal("expected a fail-fast error when no snippets-capable storage exists") + } + if !strings.Contains(err.Error(), "snippets") { + t.Errorf("error should mention snippets storage, got: %v", err) + } +} + func cloneValues(in url.Values) url.Values { out := make(url.Values, len(in)) for k, values := range in {