fix(proxmox): auto-select snippets-capable storage for cloud-init (#154)

The cloud-init snippet path (qemu-guest-agent enablement + E1
packages/runcmd) required the configured/disk storage to hold snippets;
on a local-lvm-only node it silently no-op'd, which stalls DHCP-mode IP
discovery for ~10 minutes and drops requested node prep.

- client.PickSnippetsStorage (pure, unit-tested): prefer the configured
  storage when it's snippets-capable, else the node's first snippets-capable
  storage (sorted), else ok=false.
- The handler resolves the storage via ListNodeStorages before uploading,
  so an LVM-default node still lands its snippet on e.g. "local".
- Fail-fast: when a VM explicitly requested packages/runcmd and the node has
  no snippets-capable storage, create returns an actionable error instead of
  a silently-broken node. The best-effort agent-only case still degrades
  gracefully (logged, non-fatal), preserving prior behavior.

Hardens both the bootstrap QGA path and the E1 per-VM vendor snippet (they
share applyCloudInitVendorSnippet).

Tests: PickSnippetsStorage (prefer/fallback/none); handler auto-select
(LVM default → snippet on local) and fail-fast (no snippets storage →
error); existing E1 handler test updated to serve /storage.

Author: astrojerms

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`

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/<node>/network?type=bridge.
 type NodeBridge struct {

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)
+	}
+}

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 {

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/<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 {