Skip to content

feat(api): propagate SnapshotJob PodSnapshot metadata - #129

Open
julienmancuso wants to merge 1 commit into
mainfrom
jsm/snapshotjob-metadata
Open

feat(api): propagate SnapshotJob PodSnapshot metadata#129
julienmancuso wants to merge 1 commit into
mainfrom
jsm/snapshotjob-metadata

Conversation

@julienmancuso

@julienmancuso julienmancuso commented Aug 28, 2026

Copy link
Copy Markdown

Summary

  • add caller-owned labels and annotations to SnapshotJob.spec.podSnapshotTemplate
  • validate propagated metadata and reserve SnapshotJob ownership labels before the source Job starts
  • copy metadata to the generated PodSnapshot and include it in safe adoption checks

Part of ai-dynamo/dynamo#13919.

Validation

  • GOCACHE=/private/tmp/snapshot-metadata-go-cache go test ./... in api/
  • GOCACHE=/private/tmp/snapshot-metadata-go-cache go test ./... in operator/
  • GOCACHE=/private/tmp/snapshot-metadata-go-cache go vet ./... in api/ and operator/
  • generated API and CRDs with repository-pinned controller-gen v0.19.0

make lint could not run locally because the installed golangci-lint binary was built with Go 1.24, below this repository's Go 1.26.6 module version.

Summary by CodeRabbit

  • New Features

    • SnapshotJob pod templates now support custom labels and annotations.
    • Custom metadata is copied to generated PodSnapshots.
    • Controller-owned labels are protected from overrides.
  • Bug Fixes

    • Invalid labels or annotations are rejected before workload creation.
    • Existing PodSnapshots with mismatched metadata are detected and handled safely.

Signed-off-by: Julien Mancuso <jmancuso@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: a1bf7594-d0c1-4828-ae6c-866c5ac6f133

📥 Commits

Reviewing files that changed from the base of the PR and between c1206a4 and 594c56e.

⛔ Files ignored due to path filters (2)
  • api/v1alpha1/zz_generated.deepcopy.go is excluded by !**/zz_generated*.go
  • charts/snapshot/crds/nvidia.com_snapshotjobs.yaml is excluded by !charts/snapshot/crds/**
📒 Files selected for processing (8)
  • api/v1alpha1/crds/nvidia.com_snapshotjobs.yaml
  • api/v1alpha1/snapshotjob_types.go
  • api/v1alpha1/snapshotjob_types_test.go
  • operator/internal/controller/snapshotjob_job.go
  • operator/internal/controller/snapshotjob_job_test.go
  • operator/internal/controller/snapshotjob_podsnapshot.go
  • operator/internal/controller/snapshotjob_podsnapshot_test.go
  • operator/internal/controller/snapshotjob_validation.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

SnapshotJob pod templates now accept labels and annotations. The controller validates, clones, and propagates this metadata to PodSnapshot objects while preserving controller-owned labels and checking metadata during adoption.

Changes

SnapshotJob metadata propagation

Layer / File(s) Summary
Metadata API contract
api/v1alpha1/crds/nvidia.com_snapshotjobs.yaml, api/v1alpha1/snapshotjob_types.go, api/v1alpha1/snapshotjob_types_test.go
PodSnapshotTemplate now exposes optional labels and annotations. The CRD rejects controller-owned labels. Deep-copy tests verify map independence.
Template metadata validation
operator/internal/controller/snapshotjob_validation.go, operator/internal/controller/snapshotjob_job.go, operator/internal/controller/snapshotjob_job_test.go
The controller validates metadata keys and values, rejects reserved labels, aggregates validation errors, and stops Job construction when validation fails.
PodSnapshot propagation and matching
operator/internal/controller/snapshotjob_podsnapshot.go, operator/internal/controller/snapshotjob_podsnapshot_test.go
The controller clones template metadata, overlays ownership labels, and requires existing PodSnapshot objects to contain the expected source and metadata. Tests cover propagation, copy isolation, reserved labels, and name conflicts.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to 594c5

This change propagates validated SnapshotJob metadata to generated PodSnapshots; no actionable merge-blocking risk remains based on the supplied evidence.

Sequence Diagram(s)

sequenceDiagram
  participant SnapshotJob
  participant Controller
  participant PodSnapshot
  SnapshotJob->>Controller: Provide template labels and annotations
  Controller->>Controller: Validate and clone metadata
  Controller->>PodSnapshot: Create metadata with ownership labels
  PodSnapshot-->>Controller: Return existing metadata
  Controller->>Controller: Match expected metadata
Loading
🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (6 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Breaking Api Changes ✅ Passed No breaking API change matches the custom check. The PR only adds optional PodSnapshotTemplate.Metadata, PodSnapshotTemplateMetadata.Labels, and Annotations, each with json:"...,omitempty" and…
Rbac Least Privilege ✅ Passed No failure condition is present. The pull-request diff changes CRD schemas, API types, generated deepcopy code, and SnapshotJob controller logic; it changes no RBAC marker or Helm RBAC manifest. Exist…
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses the required conventional commit prefix feat, is 53 characters long, and accurately describes the metadata propagation changes.
Full details: Docstring Coverage

Explanation

Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 7 files. (1 skipped: 1 unsupported.)

Full details: Breaking Api Changes

Explanation

No breaking API change matches the custom check. The PR only adds optional PodSnapshotTemplate.Metadata, PodSnapshotTemplateMetadata.Labels, and Annotations, each with json:"...,omitempty" and // +optional. No existing exported field was removed, renamed, retagged, or had its Go type changed. PodSnapshotSpec and PodSnapshotContentSpec immutability markers are unchanged (self == oldSelf).

Full details: Rbac Least Privilege

Explanation

No failure condition is present. The pull-request diff changes CRD schemas, API types, generated deepcopy code, and SnapshotJob controller logic; it changes no RBAC marker or Helm RBAC manifest. Existing kubebuilder markers and Helm RBAC templates use explicit resources and verbs. No wildcard grant requires justification.

  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

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