fix(dataset rm): delete staging files from a uid-65532 pod, not jobs-manager (#259)

internal/cli/dataset_rm.go

@@ -178,18 +178,30 @@ undone — re-pushing the data is the only way back.`)
  }
 	}
 
-	// 7. Execute the in-cluster teardown.
+	// 7. Execute the in-cluster teardown. File removal runs in a
+	// short-lived pod that shares the stage pod's identity (uid
+	// 65532 + fsGroup 65532), so it owns and can delete the staging
+	// files on any volume type — including hostPath, where fsGroup is
+	// a no-op (tracebloc/client#259).
 	p.Infof("Removing in-cluster artifacts…")
-	res, err := push.Teardown(ctx, cs, resolved.RestConfig, resolved.Namespace, plan)
+	res, err := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{
+ Namespace: resolved.Namespace,
+ PVCClaimName: pvc.ClaimName,
+ PVCMountPath: pvc.MountPath,
+ Table: a.Table,
+ ServiceAccountName: release.IngestorSAName,
+ // Image left empty → push.DefaultStagePodImage (alpine; has rm).
+	})
 	if err != nil {
- // Teardown is two sequential destructive ops; if the table drop
- // succeeded but file removal didn't, say so — both ops are
- // idempotent, so re-running completes the cleanup. (Bugbot #49)
+ // Two sequential destructive ops. If the table dropped but file
+ // removal didn't, the drop is idempotent (DROP TABLE IF EXISTS),
+ // so re-running is safe; if it keeps failing, remove the leftover
+ // staging dirs on the node directly.
  if res.DroppedTable {
  return &exitError{code: 7, err: fmt.Errorf(
- "teardown incomplete — the table %s.%s was dropped, but removing its files failed; "+
- "re-run `tracebloc dataset rm %s` to remove the leftover files: %w",
- plan.Database, plan.Table, a.Table, err)}
+ "teardown incomplete — the table %s.%s was dropped, but removing its files failed: %w; "+
+ "re-run `tracebloc dataset rm %s`, or delete the leftover staging dirs on the node",
+ plan.Database, plan.Table, err, a.Table)}
  }
  return &exitError{code: 7, err: fmt.Errorf("teardown failed: %w", err)}
 	}

internal/push/teardown.go

@@ -5,11 +5,11 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/rest"
 )
 
 // IngestionDatabase is the MySQL schema jobs-manager ingests tables
@@ -49,24 +49,30 @@ type TeardownResult struct {
 	RemovedPaths []string
 }
 
-// Teardown performs the in-cluster teardown described by plan, mirroring
-// the manual kubectl-exec cleanup:
+// Teardown performs the in-cluster teardown described by plan:
 //
 // - DROP the MySQL table by exec-ing `mysql` inside the mysql pod,
 // referencing the pod's own $MYSQL_ROOT_PASSWORD — so no database
 // credential ever transits the CLI.
-// - rm -rf the PVC dirs by exec-ing inside the jobs-manager pod,
-// which mounts the shared PVC at SharedRoot.
+// - rm -rf the PVC dirs from a short-lived pod that mirrors the CLI's
+// stage pod (uid 65532 + fsGroup 65532, shared PVC mounted) — built
+// from podOpts via BuildStagePodSpec.
 //
-// DESIGN NOTE: this exec-into-existing-pods approach is the
-// "CLI-direct teardown" (the alternative considered was a server-side
-// jobs-manager delete endpoint). It assumes (a) a pod whose name
-// contains "mysql" exposes $MYSQL_ROOT_PASSWORD, and (b) the
-// jobs-manager pod mounts the shared PVC at SharedRoot — both true for
-// the current parent chart, but worth confirming before this ships.
-func Teardown(ctx context.Context, cs kubernetes.Interface, cfg *rest.Config, namespace string, plan TeardownPlan) (TeardownResult, error) {
+// Why an ephemeral stage-identity pod and NOT the long-lived
+// jobs-manager pod (tracebloc/client#259): the staging files under
+// SharedRoot/.tracebloc-staging/<table> are written by the stage pod as
+// uid 65532 (and the ingestor's SharedRoot/<table> files as uid 65534).
+// The jobs-manager pod runs as a different non-root uid with no shared
+// fsGroup, so its `rm` hit EACCES on 65532-owned files in a
+// non-group-writable directory and left orphans. A teardown pod that
+// runs as the same uid that wrote the staging files OWNS them, so it
+// deletes them by ownership — which works on hostPath (where fsGroup is
+// a no-op, kubernetes/kubernetes#138411) and CSI alike.
+//
+// DESIGN NOTE: still assumes a pod whose name contains "mysql" exposes
+// $MYSQL_ROOT_PASSWORD (true for the current parent chart).
+func Teardown(ctx context.Context, cs kubernetes.Interface, exec Executor, namespace string, plan TeardownPlan, podOpts PodSpecOptions) (TeardownResult, error) {
 	var res TeardownResult
-	exec := &SPDYExecutor{Config: cfg, Client: cs}
 
 	// 1. DROP the table — mysql pod, localhost, its own root password.
 	mysqlPod, mysqlContainer, err := findRunningPod(ctx, cs, namespace, "mysql")
@@ -82,14 +88,28 @@ func Teardown(ctx context.Context, cs kubernetes.Interface, cfg *rest.Config, na
 	}
 	res.DroppedTable = true
 
-	// 2. rm the PVC dirs — jobs-manager pod mounts the shared PVC.
-	jmPod, jmContainer, err := findRunningPod(ctx, cs, namespace, "jobs-manager")
+	// 2. rm the PVC dirs from an ephemeral stage-identity pod (see the
+	// doc note above + #259). The pod owns the staging files it
+	// deletes, so this works on hostPath and CSI.
+	podOpts.Namespace = namespace
+	podName, err := CreateStagePod(ctx, cs, podOpts)
 	if err != nil {
- return res, fmt.Errorf("locating jobs-manager pod: %w", err)
+ return res, fmt.Errorf("creating teardown pod: %w", err)
+	}
+	// Always clean up the teardown pod — even if the wait/exec fails or
+	// the parent ctx is cancelled. Fresh ctx so the delete still reaches
+	// the API. Mirrors push.Stage's deferred cleanup.
+	defer func() {
+ delCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
+ _ = DeleteStagePod(delCtx, cs, namespace, podName)
+	}()
+	if _, err := WaitForStagePodReady(ctx, cs, namespace, podName); err != nil {
+ return res, fmt.Errorf("waiting for teardown pod: %w", err)
 	}
 	stderr.Reset()
 	rmCmd := append([]string{"rm", "-rf"}, plan.PVCPaths...)
-	if err := exec.Exec(ctx, namespace, jmPod, jmContainer, rmCmd, nil, nil, &stderr); err != nil {
+	if err := exec.Exec(ctx, namespace, podName, "stage", rmCmd, nil, nil, &stderr); err != nil {
  return res, fmt.Errorf("removing PVC paths: %w%s", err, stderrSuffix(&stderr))
 	}
 	res.RemovedPaths = plan.PVCPaths

internal/push/teardown_test.go

@@ -1,6 +1,15 @@
 package push
 
-import "testing"
+import (
+	"context"
+	"strings"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
+)
 
 // TestPlanTeardown pins the artifact set `dataset rm` targets: the
 // MySQL table in IngestionDatabase + both PVC dirs (final dest +
@@ -28,3 +37,81 @@ func TestPlanTeardown(t *testing.T) {
  }
 	}
 }
+
+// TestTeardown_RemovesViaStageIdentityPod is the regression test for
+// tracebloc/client#259: `dataset rm` must NOT run the file `rm` inside
+// the long-lived jobs-manager pod (a non-root uid that cannot delete
+// the uid-65532-owned staging files). It must run it in a short-lived
+// pod that mirrors the stage pod's identity (uid 65532 + fsGroup 65532),
+// which owns the staging files and so can delete them on any volume type
+// (hostPath included, where fsGroup is a no-op).
+func TestTeardown_RemovesViaStageIdentityPod(t *testing.T) {
+	// A running "mysql" pod must exist so step 1 (DROP TABLE) can locate
+	// it. The teardown pod is created by Teardown itself and marked Ready
+	// by the reactor (shared with stage_test.go).
+	cs := fake.NewClientset(&corev1.Pod{
+ ObjectMeta: metav1.ObjectMeta{Name: "mysql-0", Namespace: "tracebloc"},
+ Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "mysql"}}},
+ Status: corev1.PodStatus{Phase: corev1.PodRunning},
+	})
+	readyOnNextGet(cs)
+	fe := &fakeExecutor{}
+
+	plan := PlanTeardown("reg_train")
+	res, err := Teardown(context.Background(), cs, fe, "tracebloc", plan, PodSpecOptions{
+ Namespace: "tracebloc",
+ PVCClaimName: "client-pvc",
+ PVCMountPath: "/data/shared",
+ Table: "reg_train",
+	})
+	if err != nil {
+ t.Fatalf("Teardown: %v", err)
+	}
+	if !res.DroppedTable {
+ t.Error("DroppedTable = false, want true")
+	}
+
+	// The rm is the LAST Exec call. It must target the ephemeral stage
+	// pod, NOT a jobs-manager pod — that's the #259 fix.
+	if !strings.HasPrefix(fe.gotPod, "tracebloc-stage-") {
+ t.Errorf("rm ran in pod %q, want the ephemeral stage pod (tracebloc-stage-*); "+
+ "running it in the jobs-manager pod is the #259 bug", fe.gotPod)
+	}
+	if strings.Contains(fe.gotPod, "jobs-manager") {
+ t.Errorf("rm ran in the jobs-manager pod (%q) — the #259 regression", fe.gotPod)
+	}
+	if fe.gotContainer != "stage" {
+ t.Errorf("rm container = %q, want stage", fe.gotContainer)
+	}
+	wantCmd := "rm -rf " + strings.Join(plan.PVCPaths, " ")
+	if got := strings.Join(fe.gotCmd, " "); got != wantCmd {
+ t.Errorf("rm cmd = %q, want %q", got, wantCmd)
+	}
+
+	// The teardown pod must run as the stage uid (65532) + fsGroup so it
+	// OWNS the staging files it deletes. Inspect the created Pod via the
+	// fake clientset action log (the pod is deleted before the test ends).
+	var sc *corev1.PodSecurityContext
+	for _, action := range cs.Actions() {
+ if action.GetVerb() == "create" && action.GetResource().Resource == "pods" {
+ sc = action.(k8stesting.CreateAction).GetObject().(*corev1.Pod).Spec.SecurityContext
+ break
+ }
+	}
+	if sc == nil {
+ t.Fatal("no Pod create observed — teardown did not spawn an ephemeral pod")
+	}
+	if sc.RunAsUser == nil || *sc.RunAsUser != 65532 {
+ t.Errorf("teardown pod RunAsUser = %v, want 65532", sc.RunAsUser)
+	}
+	if sc.FSGroup == nil || *sc.FSGroup != 65532 {
+ t.Errorf("teardown pod FSGroup = %v, want 65532", sc.FSGroup)
+	}
+
+	// No leaked teardown pods after a successful run.
+	pods, _ := cs.CoreV1().Pods("tracebloc").List(context.Background(),
+ metav1.ListOptions{LabelSelector: StagePodManagedByLabel + "=" + StagePodManagedByValue})
+	if len(pods.Items) != 0 {
+ t.Errorf("Teardown leaked %d stage pod(s)", len(pods.Items))
+	}
+}