Skip to content

fix(controller): clean retained workspace namespace metadata - #433

Open
Kevin Cho (chokevin) wants to merge 15 commits into
mainfrom
chokevin-clean-workspace-metadata
Open

Kevin Cho (chokevin) wants to merge 15 commits into
mainfrom
chokevin-clean-workspace-metadata

Conversation

@chokevin

@chokevin Kevin Cho (chokevin) commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Deleting or retargeting a workspace could remove namespace routing metadata or execution identity before accepted work on retained external queues finished. Cleanup now checks unfinished Kueue Workloads and source Jobs, RayJobs, RayServices, and Kueue-managed Deployments before removing required routing labels or unchanged execution accounts. Retargeting keeps the old resolved target until draining completes.

Route discovery resolves live LocalQueues and current/admitted ClusterQueues, verifies recorded ClusterQueue UID continuity when available, and compares live namespace selectors against the cleaned labels. It does not depend on populated status.queue. External queues are never mutated or deleted. Missing queues and selector-independent routes do not introduce a drain wait; unreadable source APIs, queues, and selectors fail closed. Optional RayJob/RayService APIs are checked independently and may be absent.

  • Batch source ownership is matched by kind, UID, and name. RayJob completion matches Kueue: only deployment Complete or Failed is terminal; pending retries retain routing.
  • Long-running RayServices retain required routing until deleted, even if a derived Workload reports Finished.
  • Deployments use the Pod template's Kueue managed/queue labels, not just root labels. Required routes remain protected before Pod Workloads exist, at zero replicas, while paused/deleting, and after individual Pod Workloads finish. Delete the Deployment before completing workspace deletion or retargeting.
  • Both Helm and Kustomize add read-only source discovery for Jobs, RayJobs, RayServices, and Deployments.

Researcher authorization remains independent of execution drain. Deletion revokes owned researcher bindings before waiting. Retargeting reconciles the current researcher subject in the old namespace; cluster-wide authorization removes subject-specific bindings. Renaming the ServiceAccount, changing its client ID, or removing workload identity revokes obsolete accounts before granting access, even during drain. Failed or finalizer-delayed account deletion removes researcher bindings until revocation is confirmed. Recovery restores a missing current-researcher binding only in the retained, owned resolved namespace, never abandoned/foreign namespaces or cluster-wide mode.

Once draining completes, stale RBAC, identity, managed queue resources, and namespace metadata are removed normally. Regression coverage includes repeated reconciliation, failure/recovery, identity deletion finalizers, source-to-Workload lifecycles, long-running sources surviving Workload completion, and both deletion and retargeting.

The branch includes a normal merge of origin/main at 0d5ad5ad; no rebase or force-push of the PR branch was used. GitHub Copilot assisted with implementation, tests, and this description; the diff, evidence, and validation claims were checked before submission.

Related issue

N/A — verified TauGrid v0.4.5 live controller defect found during release validation.

Validation

PASS: cd controllers/tau-core && make test
      go test -race -count=1 ./...
PASS: cd controllers/tau-core && make lint
      go vet ./... and gofmt check
PASS: cd controllers/tau-core && make build
      Production build preserves runtime.godebugDefault=ms_tlsmldsacert=0.
PASS: cd controllers/tau-core && go test -race -v -count=1 -run '^TestWorkspacePreservesRoutingForJobBeforeWorkloadCreation$/Deployment' ./internal/controller
      Deletion and retarget cases passed.
PASS: helm lint charts/tau-core-controller
PASS: helm unittest charts/tau-core-controller
      14 tests across 5 suites passed.
PASS: helm template test charts/tau-core-controller --namespace taugrid-system >/dev/null
PASS: git diff --check
PASS: PYTHONDONTWRITEBYTECODE=1 python3 scripts/check-license-headers.py
      880 source files checked.
PASS: PYTHONDONTWRITEBYTECODE=1 python3 site/scripts/check_content.py site/content/en/docs
      35 capability pages checked.
EXPECTED FAIL: the same Deployment lifecycle command before the production fix
      Deletion completed and retarget advanced before source drain; both now pass.
Earlier follow-ups also reproduced identity recovery, Job, RayJob, and RayService lifecycle failures before fixing them.

Validation ran locally on macOS with Helm 4.2.0 and helm-unittest 1.1.1. No cluster resources were mutated. The full documentation-site build was not run; the content check passed.

Latest 17-second evidence shows the actual Deployment discovery/RBAC diff and captured failing-baseline/passing-fix deletion and retarget regressions. These are local fake-client controller tests, not live-cluster validation; detailed logs are retained separately.

deployment-source-drain.mp4

Earlier RayService source discovery evidence:

rayservice-source-drain.mp4

Earlier RayJob discovery and terminal-state evidence:

rayjob-source-drain.mp4

Earlier binding restoration and source Job evidence:

retarget-recovery-source-jobs.mp4

Earlier obsolete-identity revocation evidence:

retarget-identity-revocation.mp4

Compatibility and operational impact

No public API, CLI, CRD, configuration, image, or dependency changes. Deploy the updated controller RBAC: both Helm and Kustomize grant read-only get/list/watch for batch/jobs, apps/deployments, ray.io/rayjobs, and ray.io/rayservices. Unreadable discovery blocks cleanup, except absent optional Ray APIs.

Deletion or retargeting can remain pending while accepted work drains or routing evidence is unavailable, including legacy workspaces with empty queue status and sources awaiting Workload creation. Long-running RayServices and managed Deployments must be deleted before their required namespace routes can be cleared; finishing individual Workloads or scaling a Deployment to zero is not sufficient. Unmanaged Deployments do not introduce a source drain wait. The old namespace retains required routing and unchanged execution identity; researcher authorization is handled independently.

Explicit identity revocation takes precedence over retention. Pending work using a removed account may no longer create Pods and must be cancelled or replaced; keep identity unchanged until draining finishes for uninterrupted execution. Failed/incomplete identity deletion temporarily removes researcher bindings; recovery restores current-researcher access in the retained resolved namespace. The workspace reference documents these behaviors.

Checklist

  • The change is focused and does not include unrelated edits.
  • Tests cover the changed behavior, or I explained why tests are not needed.
  • Documentation and examples are updated when public behavior changes.
  • Generated files are updated from their source and included in this pull request.
  • New dependencies are publicly available and their license and provenance were reviewed.
  • The diff contains no secrets, private endpoints, customer data, or Microsoft-internal information.
  • Fork-based CI can validate the change without repository secrets, or maintainer-only validation is clearly identified.

Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go Outdated
Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
Comment thread controllers/tau-core/internal/controller/workspace_sync.go
Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
Comment thread controllers/tau-core/internal/controller/workspace_sync.go Outdated
@chokevin

Copy link
Copy Markdown
Contributor Author

Review evidence for a9236f8: a 17-second terminal recording of the actual cleanup-order diff and captured regression output before and after the fix. The unchanged identity survives the drain; renamed, changed-client-ID, and removed identities are revoked before old-namespace researcher access is reassigned. Failed or finalizer-blocked deletions revoke researcher bindings and report drift. These are local fake-client controller tests, not a live-cluster recording.

retarget-identity-revocation.mp4

Comment thread controllers/tau-core/internal/controller/workspace_sync.go
Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
@chokevin

Copy link
Copy Markdown
Contributor Author

Review follow-up in 1be0f57: restore the current researcher binding in the retained resolved namespace after obsolete identity revocation recovers, while its Workload remains unfinished. Include unfinished source Jobs awaiting Kueue Workload creation in deletion and retarget routing checks; match derived Workloads by Job UID and name, and grant the controller read-only Job discovery in Helm and Kustomize. The 18-second recording shows the actual diffs and captured failing-baseline/passing-fix regressions. Evidence uses local fake-client controller tests, not a live cluster. Controller race tests, lint and build; Helm lint, all 14 chart unit tests and template rendering; docs content, toolchain pins and license checks passed.

retarget-recovery-source-jobs.mp4

Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
@chokevin

Copy link
Copy Markdown
Contributor Author

RayJob follow-up in f6c59da: source discovery now includes queue-labeled RayJobs awaiting Kueue Workload creation, preserving namespace routing and unchanged execution identity during deletion and retargeting. Terminal detection matches Kueue: only deployment Complete/Failed is finished, so a failed driver with a pending retry still retains its route. Helm and Kustomize grant read-only RayJob discovery; a missing optional API is tolerated, but forbidden/unreadable API responses fail closed. The 16-second recording shows the actual discovery diff and the failing-baseline/passing-fix deletion and retarget regressions. These are local fake-client tests, not a live-cluster recording. Full controller race tests/lint, all 14 Helm tests/lint/rendering, documentation content and license checks passed.

rayjob-source-drain.mp4

Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
@chokevin

Copy link
Copy Markdown
Contributor Author

RayService follow-up in 325aa53: source discovery now preserves required namespace routing and unchanged execution identity before Kueue creates a Workload. RayServices have no terminal job state, so the route remains protected until the service is deleted. Helm and Kustomize include read-only discovery permissions. This 16-second recording shows the actual code/config diff and captured failing-baseline/passing-fix deletion and retarget regressions. These are local fake-client controller tests, not live-cluster validation; detailed logs are retained separately.

rayservice-source-drain.mp4

Comment thread controllers/tau-core/internal/controller/workspace_cleanup.go
@chokevin

Copy link
Copy Markdown
Contributor Author

Deployment source discovery fixed in 9283d7d. The drain guard now reads Kueue managed/queue labels from Deployment Pod templates before Pod Workloads exist. Live managed Deployments retain required routing and unchanged execution identity until deleted, including at zero replicas or after a Pod Workload finishes. Both RBAC manifests add read-only apps/deployments discovery. This 17-second recording shows the actual code/config diff and captured failing-baseline/passing-fix deletion and retarget cases. Evidence uses local fake-client controller tests, not a live cluster. Controller race tests, lint, build, Helm validation, documentation content, and license checks passed.

deployment-source-drain.mp4

@feiskyer Pengfei Ni (feiskyer) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants