From daf760220bd09d170449b1efc2d810d23f43303f Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Wed, 23 Sep 2026 06:58:29 +0800 Subject: [PATCH] fix(cli): pin the reaper to its storage node; drop the stale uid-1000 note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things in the same surface. --reaper-node is the supported multi-node answer: the rendered CronJob's pod gets a kubernetes.io/hostname selector, so it reads the hostPath on the node that actually holds the worlds instead of possibly scheduling where it is empty (naming a node without --worlds-host-path is fail-loud). And the render note still told operators to grant uid-1000 traverse / setfacl after #35 moved every world executor to root+DAC_OVERRIDE — it now states that fact instead of the obsolete ritual. --- cmd/felis/manifests.go | 37 ++++++++++++++++++---------- cmd/felis/manifests_test.go | 38 +++++++++++++++++++++++++++++ internal/platform/identities.go | 9 +++++++ internal/platform/workloads.go | 38 +++++++++++++++++++---------- internal/platform/workloads_test.go | 16 ++++++++++++ 5 files changed, 112 insertions(+), 26 deletions(-) diff --git a/cmd/felis/manifests.go b/cmd/felis/manifests.go index a92d9bf..7ee86ea 100644 --- a/cmd/felis/manifests.go +++ b/cmd/felis/manifests.go @@ -49,6 +49,7 @@ func cmdManifests(args []string, stdout, stderr io.Writer) int { backupPVC := fs.String("backup-pvc", "felis-backups", "name of the world-archive PVC this bundle renders in the Minecraft namespace and advertises to the backup/restore executors via FELIS_BACKUP_PVC (default: felis-backups; pass an empty value to render none, leaving backup/restore answering 503)") worldsHostPath := fs.String("worlds-host-path", "", "node directory the reaper reads worlds from: each world PVC resolves as /, or as the stock local-path directory /__ (k3s storage root: /var/lib/rancher/k3s/storage); enables the reaper CronJob (requires --archive-local-path and a non-empty --backup-pvc)") archiveLocalPath := fs.String("archive-local-path", "", "path the backup PVC is mounted at in the reaper CronJob; MUST equal felis.toml [archive] local_path") + reaperNode := fs.String("reaper-node", "", "node that holds --worlds-host-path: pins the reaper CronJob's pod there via nodeSelector kubernetes.io/hostname (multi-node clusters need this, or the reaper may schedule where the hostPath is empty)") var velocityCIDRs multiFlag fs.Var(&velocityCIDRs, "velocity-cidr", "CIDR of a Velocity proxy host allowed to reach game port 25565 (repeatable, REQUIRED)") var packageCIDRs multiFlag @@ -83,6 +84,14 @@ func cmdManifests(args []string, stdout, stderr io.Writer) int { fmt.Fprintf(stderr, "felis manifests: --panel-node-port must be in Kubernetes NodePort range 30000-32767 (got %d)\n", *panelNodePort) return 2 } + // The node pin exists only for the reaper's hostPath: naming a node without the + // worlds root would be silently dropped (no CronJob renders), so fail loud like + // the storage-trio check below. + if *reaperNode != "" && *worldsHostPath == "" { + fmt.Fprintln(stderr, "felis manifests: --reaper-node requires --worlds-host-path "+ + "(it pins the reaper CronJob, which renders only with the retention storage trio)") + return 2 + } // Retention/reaper rendering is opt-in and needs a storage topology together: // where worlds live (to read+archive them), a backup PVC (to write archives @@ -100,24 +109,25 @@ func cmdManifests(args []string, stdout, stderr io.Writer) int { "(the archive store; default felis-backups)") return 2 } - // The reaper WILL render. Three deployment preconditions this generator cannot - // check would silently turn retention into a no-op (or a permission-denied - // loop) if unmet — surface them as loudly as the fail-closed cases above, so - // an operator is never left with a reaper that reaps nothing. (All three are - // also in the WorldsHostPath flag/field docs, but nobody deploying from stdout - // reads those.) + // The reaper WILL render. Two deployment facts this generator cannot check + // would silently turn retention into a no-op if unmet — surface them as + // loudly as the fail-closed cases above, so an operator is never left with a + // reaper that reaps nothing. (Both are also in the WorldsHostPath flag/field + // docs, but nobody deploying from stdout reads those.) + pin := "the CronJob sets NO nodeSelector: a single-node starter pins it to the worlds implicitly, but on a " + + "multi-node cluster you MUST pass --reaper-node (or add a nodeSelector) for the node holding the " + + "worlds, or the reaper may schedule where the hostPath is empty" + if *reaperNode != "" { + pin = fmt.Sprintf("the CronJob is pinned to node %q via kubernetes.io/hostname — keep this pointed at the "+ + "node that actually holds the world volumes", *reaperNode) + } fmt.Fprintf(stderr, "felis manifests: note: rendering the retention reaper CronJob (worlds hostPath %q). "+ - "Three preconditions are NOT verified here:\n"+ + "These points are NOT verified here:\n"+ " - the node's world volumes must actually live below %s: the reaper resolves a world as "+ "%s/, then as the stock local-path directory /__ (what k3s "+ "writes under /var/lib/rancher/k3s/storage). Any other provisioner needs its volumes exposed as "+ "/, or each candidate's archive fails and the world is preserved;\n"+ - " - the reaper pod runs as uid 1000 and must be able to traverse %s (k3s ships its storage root "+ - "0700 root:root — the installer grants `setfacl -m u:1000:x` or o+x; a manual install must do the "+ - "same or every archive fails with permission denied and the world is preserved);\n"+ - " - the CronJob sets NO nodeSelector: a single-node starter pins it to the worlds implicitly, but "+ - "on a multi-node cluster you MUST add a nodeSelector for the node holding the worlds, or the reaper "+ - "may schedule where the hostPath is empty.\n", *worldsHostPath, *worldsHostPath, *worldsHostPath, *worldsHostPath) + " - %s.\n", *worldsHostPath, *worldsHostPath, *worldsHostPath, pin) } else { fmt.Fprintln(stderr, "felis manifests: note: retention reaper CronJob not rendered "+ "(pass --worlds-host-path and --archive-local-path — the archive PVC defaults to felis-backups — to enable it)") @@ -134,6 +144,7 @@ func cmdManifests(args []string, stdout, stderr io.Writer) int { RegistryImage: *registryImage, BackupPVC: *backupPVC, WorldsHostPath: *worldsHostPath, + ReaperNode: *reaperNode, ArchiveLocalPath: *archiveLocalPath, VelocityCIDRs: []string(velocityCIDRs), PackageSourceCIDRs: []string(packageCIDRs), diff --git a/cmd/felis/manifests_test.go b/cmd/felis/manifests_test.go index 303c46d..83fa25c 100644 --- a/cmd/felis/manifests_test.go +++ b/cmd/felis/manifests_test.go @@ -181,3 +181,41 @@ func TestManifestsRendersReaper(t *testing.T) { } } } + +// TestManifestsReaperNodePin: --reaper-node pins the rendered CronJob's pod via +// kubernetes.io/hostname and replaces the "no nodeSelector" hazard note with the +// pin confirmation; using it without the worlds root is a fail-loud 2. +func TestManifestsReaperNodePin(t *testing.T) { + var out, errBuf bytes.Buffer + code := run([]string{ + "manifests", + "--felis-image", "registry.felis.svc:5000/felis:v1", + "--velocity-cidr", "10.0.0.5/32", + "--worlds-host-path", "/var/lib/felis/worlds", + "--archive-local-path", "/backups", + "--reaper-node", "node-a", + }, &out, &errBuf) + if code != 0 { + t.Fatalf("exit code = %d, want 0; stderr=%q", code, errBuf.String()) + } + for _, want := range []string{ + "kubernetes.io/hostname: node-a", + } { + if !strings.Contains(out.String(), want) { + t.Errorf("pinned render missing %q", want) + } + } + if !strings.Contains(errBuf.String(), "node-a") { + t.Errorf("stderr must confirm the pin, got %q", errBuf.String()) + } + + var out2, err2 bytes.Buffer + if code := run([]string{ + "manifests", + "--felis-image", "registry.felis.svc:5000/felis:v1", + "--velocity-cidr", "10.0.0.5/32", + "--reaper-node", "node-a", + }, &out2, &err2); code != 2 { + t.Errorf("--reaper-node without --worlds-host-path: exit = %d, want 2", code) + } +} diff --git a/internal/platform/identities.go b/internal/platform/identities.go index aedc240..53baf2f 100644 --- a/internal/platform/identities.go +++ b/internal/platform/identities.go @@ -143,6 +143,15 @@ type Params struct { // The rendered CronJob is the correct K8s object; the actual tar depends on the // hosting node, which is not provable without a cluster. WorldsHostPath string + // ReaperNode, when non-empty, pins the rendered reaper CronJob's pod to one node + // via nodeSelector kubernetes.io/hostname — the multi-node answer to "the + // hostPath root exists on every node but a world's directory lives on exactly + // one". On a multi-node cluster the operator passes the node that holds the + // world volumes (for the local-path starter: the node whose + // /var/lib/rancher/k3s/storage carries them); leaving it empty keeps the + // single-node behaviour (no selector). It is reaper-only and only meaningful + // with WorldsHostPath set. + ReaperNode string // ArchiveLocalPath is the path the backup PVC is mounted at inside the reaper // CronJob's pod, and MUST equal felis.toml's [archive] local_path. tarLocal writes // archive refs as absolute paths under [archive] local_path (internal/backup), and diff --git a/internal/platform/workloads.go b/internal/platform/workloads.go index 6be1975..05757f9 100644 --- a/internal/platform/workloads.go +++ b/internal/platform/workloads.go @@ -38,11 +38,11 @@ import ( // (__, read from the live PVC), so pointing // --worlds-host-path at /var/lib/rancher/k3s/storage works on a default install — // see the WorldsHostPath field doc. Whether the tar finds a world still depends on -// the hosting node, and is not provable without a cluster. No nodeSelector is set: -// the single-node starter pins -// the worlds to one node implicitly; a multi-node deployment MUST add one (or the -// CronJob could schedule on a node where the hostPath is empty) — a hazard left on -// record here until multi-node retention is built. +// the hosting node, and is not provable without a cluster. No nodeSelector is set +// unless ReaperNode names one: the single-node starter pins the worlds to one node +// implicitly, while a multi-node deployment passes --reaper-node (rendered as a +// kubernetes.io/hostname selector) or the CronJob could schedule on a node where +// the hostPath is empty. const ( // configSecretName / serviceTokenSecretName are referenced BY NAME and NEVER // rendered into the bundle: felis.toml carries the database URL (a credential) @@ -605,14 +605,7 @@ func reaperCronJob(p Params) *batchv1.CronJob { ActiveDeadlineSeconds: int64Ptr(reaperActiveDeadlineSeconds), Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{Labels: labels}, - Spec: corev1.PodSpec{ - ServiceAccountName: SAReaper, - PriorityClassName: controlPlanePriorityName, - RestartPolicy: corev1.RestartPolicyNever, - SecurityContext: reaperPodSecurityContext(), - Containers: []corev1.Container{container}, - Volumes: volumes, - }, + Spec: reaperPodSpec(p, container, volumes), }, }, }, @@ -620,6 +613,25 @@ func reaperCronJob(p Params) *batchv1.CronJob { } } +// reaperPodSpec is the reaper Job's pod template. It lives apart from the CronJob +// literal only so the optional node pin is one visible branch: with ReaperNode +// set the pod carries a kubernetes.io/hostname selector, keeping the reaper on +// the node that actually holds the worlds hostPath on a multi-node cluster. +func reaperPodSpec(p Params, container corev1.Container, volumes []corev1.Volume) corev1.PodSpec { + spec := corev1.PodSpec{ + ServiceAccountName: SAReaper, + PriorityClassName: controlPlanePriorityName, + RestartPolicy: corev1.RestartPolicyNever, + SecurityContext: reaperPodSecurityContext(), + Containers: []corev1.Container{container}, + Volumes: volumes, + } + if p.ReaperNode != "" { + spec.NodeSelector = map[string]string{"kubernetes.io/hostname": p.ReaperNode} + } + return spec +} + // controlPlaneDeployment assembles a single-replica control-plane Deployment. The // Deployment, its selector, and the pod template all carry // controlPlanePodLabels(component) so the three agree (a selector mismatch would diff --git a/internal/platform/workloads_test.go b/internal/platform/workloads_test.go index 9c87db4..40ad8e0 100644 --- a/internal/platform/workloads_test.go +++ b/internal/platform/workloads_test.go @@ -648,6 +648,22 @@ func TestReaperCronJob_Gating(t *testing.T) { } } +// TestReaperCronJob_NodePin proves the optional multi-node pin: no selector by +// default (the single-node starter), and exactly the kubernetes.io/hostname +// selector when ReaperNode names the node holding the worlds hostPath. +func TestReaperCronJob_NodePin(t *testing.T) { + ps, _ := cronPodSpec(t, reaperCronJob(reaperParams())) + if ps.NodeSelector != nil { + t.Errorf("NodeSelector = %v, want none without ReaperNode", ps.NodeSelector) + } + p := reaperParams() + p.ReaperNode = "node-a" + ps, _ = cronPodSpec(t, reaperCronJob(p)) + if got := ps.NodeSelector["kubernetes.io/hostname"]; got != "node-a" { + t.Errorf("nodeSelector = %v, want kubernetes.io/hostname=node-a", ps.NodeSelector) + } +} + // TestReaperCronJob_Shape pins the rendered CronJob: its scheduling guards, its // run-as identity (felis-reaper WITH an auto-mounted token, because it legitimately // calls the K8s API — unlike the weak Job/registry pods), the hardening, the