Skip to content

feat: set ownerReference on ModelMetadata CRs for automatic GC on pod deletion#507

Open
omerap12 wants to merge 7 commits into
ai-dynamo:mainfrom
omerap12:modelmetadatas-gc
Open

feat: set ownerReference on ModelMetadata CRs for automatic GC on pod deletion#507
omerap12 wants to merge 7 commits into
ai-dynamo:mainfrom
omerap12:modelmetadatas-gc

Conversation

@omerap12

@omerap12 omerap12 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Overview

This PR associates Kubernetes-backed ModelMetadata records with the Pod that
published them. When a client provides a complete same-namespace Pod identity,
the Kubernetes metadata backend adds a Pod ownerReference, allowing Kubernetes
to garbage-collect the metadata immediately when that Pod is deleted.

The change applies to every engine and metadata type that uses the shared
PublishMetadata path, including weight and artifact metadata.

What changed

  • Added optional pod_name, pod_uid, and pod_namespace fields to
    PublishMetadataRequest.
  • Updated the Python client to populate those fields from POD_NAME, POD_UID,
    and POD_NAMESPACE.
  • Updated the Kubernetes backend to:
    • create Pod-owned ModelMetadata CRs for complete same-namespace identities;
    • patch existing CRs when Pod identity becomes available on a later publish;
    • reject partial or cross-namespace identities for ownership without rejecting
      metadata publication.
  • Preserved the existing ModelMetadata -> tensor ConfigMap ownership chain,
    so deleting a Pod collects both the CR and its tensor metadata.
  • Added Downward API wiring to the checked-in vLLM, SGLang, Dynamo, and CI
    manifests.
  • Updated the deployment, metadata, and Kubernetes P2P example documentation.

Compatibility

The protobuf fields are additive. MX 0.4.0 and other older clients omit them and
continue publishing metadata without a Pod owner reference. Missing, partial,
or cross-namespace identity also preserves this fallback behavior. Redis,
in-memory, and non-Kubernetes environments ignore the Pod fields and continue
using the existing heartbeat/reaper lifecycle.

Validation

Local and CI-equivalent checks

  • Rust tests: 184 passed
  • Workspace Clippy: passed
  • Python client, vLLM, and SGLang tests: 100 passed, 8 skipped
  • Go binding tests: passed
  • YAML validation: passed
  • Post-origin/main merge focused client/SGLang tests: 44 passed

Kubernetes E2E

Tested in namespace zheng on a B200 node with the Kubernetes metadata backend,
an isolated server built from this PR, the shared model PVC, and
Qwen/Qwen2.5-0.5B. Source and target ran on the same node; this validates the
complete Kubernetes lifecycle and NIXL/UCX P2P path, but is not a cross-node RDMA
benchmark.

Engine Runtime Source behavior Target P2P result Inference
vLLM 0.23.0 Cold-loaded from disk and published Ready metadata 267 tensors / 0.99 GB in 0.062s (128 Gbps) /v1/models succeeded; completion returned Paris
SGLang 0.5.13.post1 Cold-loaded from disk and published Ready metadata 170 tensors / 0.99 GB in 0.321s (24.6 Gbps) /v1/models succeeded; completion returned Paris

For both engines:

  • source and target published separate Ready ModelMetadata CRs;
  • each CR referenced the correct publishing Pod name and UID;
  • deleting the client Deployments reduced the matching CR count to zero through
    Kubernetes owner garbage collection;
  • no ModelMetadata CRs were manually deleted.

The live E2E run exercised weight metadata. Artifact metadata uses the same
PublishMetadata ownership path, but artifact transfer was not run as a separate
E2E case in this validation.

@copy-pr-bot

copy-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Metadata publishing now carries optional Kubernetes pod name and UID values from environment variables through Python and gRPC APIs into server backends. The Kubernetes backend uses both values to set ModelMetadata owner references; other backends retain compatible signatures without using them.

Changes

Kubernetes Pod Metadata Propagation

Layer / File(s) Summary
Pod metadata contracts and environment sources
modelexpress_common/proto/p2p.proto, modelexpress_common/src/envs.rs, modelexpress_client/python/modelexpress/p2p_pb2.py
The publish request and environment registries define pod_name and pod_uid, with regenerated protobuf descriptors.
Client-side pod context propagation
modelexpress_client/python/modelexpress/client.py, modelexpress_client/python/modelexpress/envs.py, modelexpress_client/python/modelexpress/metadata/*, modelexpress_client/python/tests/test_artifact_transfer.py
Python publishing reads pod environment values and forwards them through both publishing paths and the gRPC request.
RPC and state forwarding
modelexpress_server/src/p2p/backend.rs, modelexpress_server/src/p2p/service.rs, modelexpress_server/src/p2p/state.rs
The server RPC, state manager, and backend trait accept and forward pod identity values; affected tests match the expanded calls.
Backend ownership and compatibility updates
modelexpress_server/src/p2p/backend/kubernetes.rs, modelexpress_server/src/p2p/backend/memory.rs, modelexpress_server/src/p2p/backend/redis.rs
The Kubernetes backend conditionally adds pod owner references, while memory and Redis backends accept unused pod parameters.

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

Poem

I’m a rabbit with two pods in my hat,
Name and UID—imagine that!
Through clients and servers they hop,
Kubernetes gives metadata a crop.
Owner references now bloom where they stop.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding ownerReferences so ModelMetadata CRs are garbage-collected when the pod is deleted.

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
modelexpress_server/src/p2p/service.rs (1)

433-455: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Exercise the pod identity propagation contract end to end.

The updated test seams accept the new fields but either discard them, pass empty values, or ignore them in assertions, allowing propagation regressions to pass.

  • modelexpress_server/src/p2p/service.rs#L433-L455: send non-empty pod fields and assert the backend receives both.
  • modelexpress_client/python/tests/test_artifact_transfer.py#L1266-L1266: retain pod_name and pod_uid on _FakeMxClient for assertions.
  • modelexpress_server/src/p2p/state.rs#L462-L485: assert both values reach MetadataBackend.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@modelexpress_server/src/p2p/service.rs` around lines 433 - 455, Strengthen
the pod identity propagation tests: in
modelexpress_server/src/p2p/service.rs:433-455, update
test_publish_metadata_success to send non-empty pod name and UID values and
assert both are received by the backend; in
modelexpress_client/python/tests/test_artifact_transfer.py:1266, retain pod_name
and pod_uid on _FakeMxClient for assertions; in
modelexpress_server/src/p2p/state.rs:462-485, assert both values reach
MetadataBackend.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@modelexpress_server/src/p2p/backend/kubernetes.rs`:
- Around line 226-239: Update the publish/update flow surrounding
owner_references so an existing custom resource receives the Pod OwnerReference
whenever both pod name and UID are available, not only during initial creation.
Reuse the existing has_pod_name_and_uid condition and preserve None when either
value is absent, ensuring metadata is patched for previously unowned resources.

---

Outside diff comments:
In `@modelexpress_server/src/p2p/service.rs`:
- Around line 433-455: Strengthen the pod identity propagation tests: in
modelexpress_server/src/p2p/service.rs:433-455, update
test_publish_metadata_success to send non-empty pod name and UID values and
assert both are received by the backend; in
modelexpress_client/python/tests/test_artifact_transfer.py:1266, retain pod_name
and pod_uid on _FakeMxClient for assertions; in
modelexpress_server/src/p2p/state.rs:462-485, assert both values reach
MetadataBackend.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3b176d7c-e631-4922-aed9-79d81ecc1b87

📥 Commits

Reviewing files that changed from the base of the PR and between e1816bd and 0d01b02.

📒 Files selected for processing (14)
  • modelexpress_client/python/modelexpress/client.py
  • modelexpress_client/python/modelexpress/envs.py
  • modelexpress_client/python/modelexpress/metadata/k8s_service_client.py
  • modelexpress_client/python/modelexpress/metadata/publish.py
  • modelexpress_client/python/modelexpress/p2p_pb2.py
  • modelexpress_client/python/tests/test_artifact_transfer.py
  • modelexpress_common/proto/p2p.proto
  • modelexpress_common/src/envs.rs
  • modelexpress_server/src/p2p/backend.rs
  • modelexpress_server/src/p2p/backend/kubernetes.rs
  • modelexpress_server/src/p2p/backend/memory.rs
  • modelexpress_server/src/p2p/backend/redis.rs
  • modelexpress_server/src/p2p/service.rs
  • modelexpress_server/src/p2p/state.rs

Comment thread modelexpress_server/src/p2p/backend/kubernetes.rs Outdated
@zhengluo-nv zhengluo-nv self-assigned this Jul 15, 2026
omerap12 and others added 5 commits July 15, 2026 11:23
… deletion

Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Omer Aplatony <omerap12@gmail.com>
Signed-off-by: Zheng Luo <zheluo@nvidia.com>
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.48485% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
modelexpress_server/src/p2p/backend/kubernetes.rs 96.15% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@zhengluo-nv

Copy link
Copy Markdown
Contributor

/ok to test df84eee

@copy-pr-bot copy-pr-bot Bot temporarily deployed to automated-release July 15, 2026 21:24 Inactive
@copy-pr-bot copy-pr-bot Bot temporarily deployed to automated-release July 15, 2026 21:24 Inactive

@zhengluo-nv zhengluo-nv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not merge, waiting for final review

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants