diff --git a/internal/cli/dataset_rm.go b/internal/cli/dataset_rm.go index a7c398f7..ecc83f6a 100644 --- a/internal/cli/dataset_rm.go +++ b/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)} } diff --git a/internal/push/teardown.go b/internal/push/teardown.go index 5d30ccf5..1c270feb 100644 --- a/internal/push/teardown.go +++ b/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/ are written by the stage pod as +// uid 65532 (and the ingestor's SharedRoot/
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 diff --git a/internal/push/teardown_test.go b/internal/push/teardown_test.go index a3e94444..d7275d7b 100644 --- a/internal/push/teardown_test.go +++ b/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)) + } +}