Support user-defined shared volumes for containers (#421)

agent/src/index.ts

@@ -16,6 +16,7 @@ import { State } from './state';
 import { getPrimaryIpv4, getServiceState, disconnectSystemBus } from './system';
 import { checkin } from './api';
 import { services, applyService } from './apply';
+import { reconcileVolumes } from './volumes';
 import { log } from './log';
 import type { CheckinRequest, ServiceStatus } from './types';
 
@@ -31,24 +32,45 @@ async function buildCheckinBody(cfg: AgentConfig, state: State): Promise<Checkin
       lastApply: state.lastApply[svc.unit] ?? 'unknown',
     };
   }
-  return {
+  const body: CheckinRequest = {
     siteId: cfg.siteId,
     hostname: os.hostname(),
     currentTime: Math.floor(Date.now() / 1000),
     ipv4Address: getPrimaryIpv4(),
     services: serviceStatus,
   };
+  // Report any not-yet-delivered volume results (persisted across runs). Only
+  // include the field when there is something to report so the manager isn't
+  // sent an empty map every check-in.
+  if (Object.keys(state.pendingVolumeResults).length > 0) {
+    body.volumes = { ...state.pendingVolumeResults };
+  }
+  return body;
 }
 
 async function main(): Promise<void> {
   const cfg = loadConfig();
   const state = State.load(cfg.stateDir);
   log.info(`agent starting: siteId=${cfg.siteId}, manager=${cfg.managerUrl}`);
   log.debug(`state dir=${cfg.stateDir}, saved etag=${state.etag ?? '(none)'}`);
+  if (Object.keys(state.pendingVolumeResults).length > 0) {
+    log.debug(`carrying ${Object.keys(state.pendingVolumeResults).length} pending volume result(s) from a prior run`);
+  }
 
   for (let pass = 0; pass < MAX_PASSES; pass++) {
     log.debug(`check-in pass ${pass + 1}/${MAX_PASSES}`);
+    // Remember what this check-in body carries so we can clear it only once the
+    // request has actually completed (the manager processes the `volumes` map
+    // before computing the ETag, so a 200 or a 304 both mean it was delivered).
+    const carriedVolumeIds = Object.keys(state.pendingVolumeResults);
     const result = await checkin(cfg, await buildCheckinBody(cfg, state), state.etag);
+
+    // The check-in completed, so any results it carried are now delivered.
+    if (carriedVolumeIds.length > 0) {
+      for (const id of carriedVolumeIds) delete state.pendingVolumeResults[id];
+      state.save();
+    }
+
     if (result.notModified) {
       log.info('check-in: config unchanged (304), nothing to apply');
       return;
@@ -59,18 +81,45 @@ async function main(): Promise<void> {
       state.lastApply[svc.unit] = await applyService(svc, result.config, cfg);
     }
 
-    // The ETag is saved even after a failed apply: a rejected config won't
-    // fix itself without a server-side change (which changes the ETag), and
-    // the failure has been reported via lastApply.
-    state.etag = result.etag;
+    // Ensure volume directories exist from the new snapshot; stash results in
+    // persistent state so they are reported on the next check-in and survive a
+    // process exit in between (they are cleared once delivered, above).
+    const volumeResults = reconcileVolumes(result.config);
+    if (volumeResults) {
+      Object.assign(state.pendingVolumeResults, volumeResults);
+    }
+
+    // A failed volume mkdir is typically transient (a not-yet-mounted volumes
+    // root, a slow shared mount, a momentary permission glitch) and WILL fix
+    // itself on a retry without any server-side config change. If we saved the
+    // ETag now, the next run would get a 304 and never re-run the reconcile,
+    // leaving the volume `failed` until the config changes. So when any volume
+    // failed this pass, do NOT persist the ETag — forcing the next check-in to
+    // re-fetch the config (200, not 304) and re-run the reconcile. Service
+    // applies keep the existing "save even on failure" behavior (a rejected
+    // nginx/dnsmasq config won't fix itself without a server-side change).
+    const volumeFailed = volumeResults
+      ? Object.values(volumeResults).some((r) => !r.applied)
+      : false;
+    if (volumeFailed) {
+      log.warn('one or more volume directories failed to provision; will retry on next check-in');
+      state.etag = undefined;
+    } else {
+      state.etag = result.etag;
+    }
     state.save();
   }
 
   // MAX_PASSES exhausted (flapping server-side config): check in once more so
-  // the final pass' apply results reach the manager instead of going stale
-  // until the next timer run.
+  // the final pass' apply/volume results reach the manager instead of going
+  // stale until the next timer run. Clear delivered results afterwards.
   log.warn(`reached MAX_PASSES (${MAX_PASSES}) without a stable config; reporting final results`);
+  const carried = Object.keys(state.pendingVolumeResults);
   await checkin(cfg, await buildCheckinBody(cfg, state), state.etag);
+  if (carried.length > 0) {
+    for (const id of carried) delete state.pendingVolumeResults[id];
+    state.save();
+  }
 }
 
 main()

agent/src/state.ts

@@ -1,14 +1,21 @@
 /** Persistent agent state: last applied config ETag + per-service apply
- * results, stored as JSON under the state dir. */
+ * results + pending per-volume provisioning results, stored as JSON under the
+ * state dir. */
 
 import fs from 'fs';
 import path from 'path';
 import { log } from './log';
-import type { ApplyResult } from './types';
+import type { ApplyResult, VolumeResult } from './types';
 
 export class State {
   etag?: string;
   lastApply: Record<string, ApplyResult> = {};
+  // Volume provisioning results not yet confirmed delivered to the manager.
+  // Persisted so they survive a process exit between reconcile and the next
+  // check-in — otherwise a saved ETag would 304 the next run and the result
+  // would be lost, leaving the volume pending until the create barrier times
+  // out. Cleared only after a check-in that carried them completes.
+  pendingVolumeResults: Record<string, VolumeResult> = {};
 
   private constructor(private readonly file: string) {}
 
@@ -22,9 +29,14 @@ export class State {
       throw err;
     }
     try {
-      const data = JSON.parse(raw) as { etag?: string; lastApply?: Record<string, ApplyResult> };
+      const data = JSON.parse(raw) as {
+        etag?: string;
+        lastApply?: Record<string, ApplyResult>;
+        pendingVolumeResults?: Record<string, VolumeResult>;
+      };
       state.etag = data.etag;
       state.lastApply = data.lastApply ?? {};
+      state.pendingVolumeResults = data.pendingVolumeResults ?? {};
     } catch (err) {
       if (!(err instanceof SyntaxError)) throw err;
       // A corrupt state file just means a full re-apply on this run.
@@ -37,7 +49,11 @@ export class State {
     fs.mkdirSync(path.dirname(this.file), { recursive: true });
     fs.writeFileSync(
       this.file,
-      JSON.stringify({ etag: this.etag, lastApply: this.lastApply }, null, 2),
+      JSON.stringify(
+        { etag: this.etag, lastApply: this.lastApply, pendingVolumeResults: this.pendingVolumeResults },
+        null,
+        2,
+      ),
     );
   }
 }

agent/src/types.ts

@@ -19,6 +19,32 @@ export interface CheckinRequest {
   currentTime: number;
   ipv4Address: string | null;
   services: Record<string, ServiceStatus>;
+  /**
+   * Per-volume directory-provisioning results, keyed by the manager-assigned
+   * Volume id: `{ <volumeId>: { applied, message? } }`. Present only when the
+   * agent processed volumes this pass. The manager writes these into
+   * Volume.status (ready/failed) at check-in.
+   */
+  volumes?: Record<string, VolumeResult>;
+}
+
+/** Outcome of ensuring one volume directory on this node. */
+export interface VolumeResult {
+  applied: boolean;
+  message?: string;
+}
+
+/** A volume directory the agent must ensure exists, as carried in the config
+ * snapshot at the site level. `uid`/`gid` are the owning host ids (the
+ * unprivileged CT's id-mapped root); the agent applies them best-effort — a
+ * no-op in an unprivileged agent guest (mkdir already yields that owner) and the
+ * real fix in a privileged agent guest (mkdir would otherwise be root-owned). */
+export interface SiteVolume {
+  id: number;
+  hostPath: string;
+  mode: 'ro' | 'rw';
+  uid: number;
+  gid: number;
 }
 
 export interface SiteContainer {
@@ -45,6 +71,8 @@ export interface SiteInfo {
   gateway: string | null;
   dnsForwarders: string | null;
   nodes: SiteNode[];
+  /** Volume directories to ensure for the whole site. Absent on older managers. */
+  volumes?: SiteVolume[];
 }
 
 export interface HttpService {

agent/src/volumes.ts

@@ -0,0 +1,174 @@
+/**
+ * Volume directory reconciliation (issue #421).
+ *
+ * The manager includes, at the site level, the volume directories that must
+ * exist (id, hostPath, mode, and the owning host uid/gid). The shared volumes
+ * root is bind-mounted into the agent guest by the installer, so these paths are
+ * visible and writable here.
+ *
+ * Ownership is applied BEST-EFFORT via chown, so it is correct whether the
+ * agent runs in an unprivileged or (rare) privileged guest:
+ *   - Unprivileged agent guest (the norm — `pct create` defaults to
+ *     `--unprivileged 1`, which includes the embedded Manager agent): the
+ *     agent's root already maps to the host owner (100000), so mkdir yields the
+ *     right owner. The chown to 100000 then targets an id outside the guest's
+ *     mapped range and is a tolerated no-op (see EINVAL/EPERM/ENOSYS below).
+ *   - Privileged agent guest (only if deliberately created with
+ *     `--unprivileged 0`): the agent is host root, so mkdir would otherwise
+ *     create a root-owned directory; the chown fixes it so an unprivileged
+ *     consuming container can write RW volumes.
+ * A failed chown is logged and ignored — it never fails the volume, since the
+ * unprivileged-guest case relies on the id-map, not the chown.
+ *
+ * There is one agent per site (not per node); the volumes root lives on storage
+ * shared across the site's nodes, so this single agent provisions every site
+ * volume regardless of which node hosts the container.
+ *
+ * Results are reported per volume id at the next check-in, which the manager
+ * writes into Volume.status. Directory creation is retain-only: the agent never
+ * removes a volume directory, so data survives container delete + recreate.
+ */
+
+import fs from 'fs';
+import { log } from './log';
+import type { SiteConfig, SiteVolume, VolumeResult } from './types';
+
+// Mode for created directories. RW volumes get owner/group rwx (the id-mapped
+// container root owns the dir, so it can write); RO volumes are r-x. World bits
+// are left closed so other tenants can't read another container's data.
+const RW_MODE = 0o0770;
+const RO_MODE = 0o0550;
+
+/**
+ * Collect the volumes to provision from the config snapshot (site-level).
+ * @param {SiteConfig} config
+ * @returns {SiteVolume[]}
+ */
+export function volumesForSite(config: SiteConfig): SiteVolume[] {
+  return config.site?.volumes ?? [];
+}
+
+/**
+ * Mount points visible to this process, from /proc/self/mountinfo (field 5,
+ * with the kernel's octal escapes for space/tab/newline/backslash decoded).
+ * @returns {string[]}
+ */
+export function readMountPoints(): string[] {
+  const text = fs.readFileSync('/proc/self/mountinfo', 'utf8');
+  return text
+    .split('\n')
+    .filter(Boolean)
+    .map((line) => line.split(' ')[4])
+    .filter((mp): mp is string => typeof mp === 'string')
+    .map((mp) => mp.replace(/\\([0-7]{3})/g, (_, oct: string) => String.fromCharCode(parseInt(oct, 8))));
+}
+
+/**
+ * The deepest mount point that contains `path` ('/' when nothing more specific
+ * does).
+ * @param {string} path
+ * @param {string[]} mountPoints
+ * @returns {string}
+ */
+export function containingMountPoint(path: string, mountPoints: string[]): string {
+  let best = '/';
+  for (const mp of mountPoints) {
+    if (mp === '/') continue;
+    if ((path === mp || path.startsWith(`${mp}/`)) && mp.length > best.length) best = mp;
+  }
+  return best;
+}
+
+/**
+ * Ensure a single volume directory exists with the right owner and mode.
+ * Idempotent: mkdir -p, best-effort chown, then chmod every run (cheap,
+ * self-healing).
+ *
+ * Refuses to create anything unless `hostPath` lies on a mount other than the
+ * agent's root filesystem. The volumes root must be bind-mounted into the agent
+ * (see "Deploying Agents"); if it isn't, `mkdir -p` would silently create the
+ * path inside the agent guest's own rootfs and report success, letting the
+ * manager's readiness barrier pass while the Proxmox host path is still
+ * missing — so the mpN attach would then fail. Reporting a failure here
+ * surfaces the misconfiguration as `Volume.status = failed` with a clear
+ * message instead.
+ * @param {SiteVolume} volume
+ * @param {string[]} mountPoints
+ * @returns {VolumeResult}
+ */
+function ensureVolume(volume: SiteVolume, mountPoints: string[]): VolumeResult {
+  const { hostPath, mode, uid, gid } = volume;
+  try {
+    if (containingMountPoint(hostPath, mountPoints) === '/') {
+      throw new Error(
+        `${hostPath} is not on a mounted volumes root (it would be created inside the agent's own ` +
+          'filesystem); bind-mount the shared volumes root into the agent container',
+      );
+    }
+    fs.mkdirSync(hostPath, { recursive: true });
+    // Best-effort ownership (see module header): the real fix in a privileged
+    // agent guest (agent is host root), and a harmless no-op in an unprivileged
+    // one — where mkdir already yields the correct owner via the id-map. Several
+    // failures are therefore EXPECTED and tolerated rather than failing the
+    // volume:
+    //   - EINVAL: the target host id (e.g. 100000) is outside the unprivileged
+    //     guest's mapped range, so it isn't a valid id to chown to from inside
+    //     the guest. This is the normal unprivileged case — ownership is already
+    //     correct from the id-map, so ignore it.
+    //   - EPERM:  the guest lacks CAP_CHOWN.
+    //   - ENOSYS: chown unsupported.
+    if (typeof uid === 'number' && typeof gid === 'number') {
+      try {
+        fs.chownSync(hostPath, uid, gid);
+      } catch (chownErr) {
+        const code = (chownErr as NodeJS.ErrnoException).code;
+        if (code === 'EINVAL' || code === 'EPERM' || code === 'ENOSYS') {
+          log.debug(`volume ${volume.id}: chown to ${uid}:${gid} skipped (${code}); relying on id-map`);
+        } else {
+          throw chownErr;
+        }
+      }
+    }
+    fs.chmodSync(hostPath, mode === 'rw' ? RW_MODE : RO_MODE);
+    log.debug(`volume ${volume.id}: ensured ${hostPath} (mode=${mode}, owner=${uid}:${gid})`);
+    return { applied: true };
+  } catch (err) {
+    const message = err instanceof Error ? err.message : String(err);
+    log.error(`volume ${volume.id}: failed to ensure ${hostPath}: ${message}`);
+    return { applied: false, message };
+  }
+}
+
+/**
+ * Reconcile all volume directories for this site. Returns a results map keyed
+ * by volume id for the check-in body, or undefined when there is nothing to do
+ * (so the check-in omits the field on older managers / empty sites).
+ * @param {SiteConfig} config
+ * @param {string[]} [mountPoints] Mount points to check paths against;
+ *   defaults to this process's /proc/self/mountinfo (injectable for tests).
+ * @returns {Record<string, VolumeResult> | undefined}
+ */
+export function reconcileVolumes(
+  config: SiteConfig,
+  mountPoints?: string[],
+): Record<string, VolumeResult> | undefined {
+  const volumes = volumesForSite(config);
+  if (volumes.length === 0) return undefined;
+
+  log.info(`volumes: ensuring ${volumes.length} directory(ies)`);
+  let mounts: string[];
+  try {
+    mounts = mountPoints ?? readMountPoints();
+  } catch (err) {
+    // Can't tell whether the volumes root is mounted, so fail closed rather
+    // than risk reporting a guest-local directory as provisioned.
+    const message = `cannot read mount table: ${err instanceof Error ? err.message : String(err)}`;
+    log.error(`volumes: ${message}`);
+    return Object.fromEntries(volumes.map((v) => [String(v.id), { applied: false, message }]));
+  }
+  const results: Record<string, VolumeResult> = {};
+  for (const volume of volumes) {
+    results[String(volume.id)] = ensureVolume(volume, mounts);
+  }
+  return results;
+}