Skip to content

Retrieve complete acknowledged run artifact bundles - #32

Open
Kevin Cho (chokevin) wants to merge 5 commits into
mainfrom
chokevin-retrieve-published-artifacts
Open

Kevin Cho (chokevin) wants to merge 5 commits into
mainfrom
chokevin-retrieve-published-artifacts

Conversation

@chokevin

Copy link
Copy Markdown
Contributor

Summary

Temporary PVC-reader Pods made artifact retrieval depend on ad hoc cluster mutation and treated a shared directory as if it were a completion contract. This change adds a generation-bound bundle acknowledgement after staged publication, checkpoint indexing, and metrics offload, then makes tau run get <name> --destination DIR verify and download that complete bundle directly from Azure Blob storage.

The tau-core controller stamps trusted, non-secret Blob transport metadata on Tau-managed Jobs and RayJobs, so workspace users do not need cluster-scoped PV access. Retrieval fails closed on missing or mismatched acknowledgements, storage listing errors, untrusted endpoints, size mismatches, and destination collisions. This PR was prepared with AI assistance and the resulting implementation and tests were reviewed and run locally.

Related issue

N/A

Validation

PASS: cd cli && go test ./...
PASS: cd cli && make lint
PASS: cd cli && make build
PASS: cd controllers/tau-core && go test -race -count=1 ./...
PASS: cd controllers/tau-core && make lint && make build
PASS: cd core && go test ./... && go vet ./...
PASS: helm lint charts/tau-core-controller
PASS: helm template test charts/tau-core-controller --namespace taugrid-system
PASS: cd cli && go test -count=1 -run TestCompleteBundleLocalFixtureEnumeratesAndDownloadsWithoutKubernetes ./internal/artifactbundle
PASS: cd controllers/tau-core && go test -count=1 -run 'TestArtifactStore' ./internal/controller
PASS: git diff --check
BLOCKED: cd site && make check
  npm could not fetch node-releases@2.0.52: the internal feed returned 404 and registry.npmjs.org returned ENOTCONN. Documentation changes are Markdown-only.

Compatibility and operational impact

  • Adds tau run get --destination DIR for complete acknowledged bundles on /data-backed single-pod Jobs and RayJobs. Existing single-artifact and listing behavior remains available where practical.
  • Multi-node Indexed Jobs and custom PVC mount roots intentionally do not emit a complete bundle marker; an individual index cannot safely acknowledge Job-level completion.
  • Downloads refuse to replace existing destination files.
  • Adds tau-core read access to PVC/PV metadata and patch/update access to Jobs and RayJobs. The controller mutates only Tau-managed workloads carrying bundle and result-PVC metadata. RayJob watching is skipped when the CRD is absent.
  • Legacy workloads without the transport annotation fall back to read-only PVC/PV discovery.
  • No CRD schema changes, direct storage credentials, account keys, SAS tokens, or new dependencies are introduced. Rollback is a normal revert; the new annotations and durable .tau/bundles files are additive.

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.

Replace temporary PVC-reader pods with a generation-bound bundle contract and direct Azure Blob retrieval. Have tau-core stamp trusted non-secret transport metadata and fail closed across staged publication, metrics, and checkpoints.
Comment thread cli/internal/cli/run_get.go Outdated
Comment thread cli/internal/artifactbundle/retrieve.go
Comment thread cli/internal/cli/run_get.go Outdated
Comment thread cli/internal/jobrender/render.go Outdated
Comment thread cli/internal/cli/run_bundle.go
if outputDir == "." || strings.TrimSpace(resultPVC) == "" || !outputWritable {
return artifactbundle.Runtime{}, nil
}
if outputDir != "/data" && !strings.HasPrefix(outputDir, "/data/") {

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.

P1 storage.output: /data is accepted here. PVCRelativePath("/data") becomes an empty prefix, so run get --destination passes "" to Blob listing and downloads every object in the container. Require a strict descendant of /data and reject an empty bundle prefix before listing.

}
checkpointCheck := ""
if strings.TrimSpace(runtime.CheckpointIndex) != "" {
checkpointCheck = fmt.Sprintf(`if [ ! -f %s ] ||

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.

P1 A missing declared checkpoint is intentionally non-fatal in artifactindex (it exits 0), but this wrapper then exits 126 because no index exists, turning a successful workload into a failed one. Treat an absent index as “no bundle acknowledgement” without changing the payload exit status; keep 126 for malformed or mismatched existing indexes.

Make artifact manifest shell quoting explicit

rayJob := &unstructured.Unstructured{}
rayJob.SetGroupVersionKind(schema.GroupVersionKind{Group: "ray.io", Version: "v1", Kind: "RayJob"})
if _, err := mgr.GetRESTMapper().RESTMapping(rayJob.GroupVersionKind().GroupKind(), rayJob.GroupVersionKind().Version); err != nil {

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.

P1 Returning success on NoMatch permanently skips the RayJob controller when the CRD is installed after tau-core starts; setup is only called once, so later RayJobs never get artifact-store metadata until restart. Retry discovery/register after the CRD appears, or fail startup so setup is retried.

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