Skip to content

[OEP] Propose optional Dragonfly model downloads - #1205

Merged
heymrbox merged 1 commit into
mainfrom
heymrbox-oep-0012-dragonfly-review
Oct 8, 2026
Merged

heymrbox merged 1 commit into
mainfrom
heymrbox-oep-0012-dragonfly-review

Conversation

@heymrbox

@heymrbox heymrbox commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

What this PR does

Adds OEP-0012 proposing optional Dragonfly transport for PerNode Hugging Face model downloads.
Defines immutable snapshot identity, credential partitioning, durable ownership and recovery, endpoint normalization, verification, fallback, observability, and rollout requirements.
Dragonfly remains opt-in. Default direct downloads and existing CRD APIs remain unchanged. This PR contains the proposal, not the implementation.

Why we need it

Concurrent downloads across nodes repeatedly fetch identical model files from the origin. Optional peer-to-peer distribution can reduce origin traffic and improve rollout latency without changing the serving storage layout.
The proposal also establishes security and qualification requirements before alpha release, explicitly excluding unpatched Dragonfly client v1.5.7.

Fixes: N/A — no linked issue.

How to test

Documentation-only change. Validation completed:

  • Pre-commit checks, including spelling, YAML, Helm lint, and chart rendering.
  • OEP table-of-contents, references, metadata, and metrics consistency.
  • Existing TestDirectHFSkipConfigParsingCompletesOrdinaryDownload regression test.
    The OEP specifies future unit and end-to-end qualification tests. The full make test suite was not run.

Checklist

  • Tests added/updated (if applicable)
  • Docs updated (if applicable)
  • make test passes locally

Summary by CodeRabbit

  • Documentation
    • Added a proposal for optional Dragonfly transport for eligible Hugging Face model downloads. Direct/Xet downloads remain the default, and Dragonfly is not required to run OME.
    • The proposal outlines configuration, download eligibility, fallback behavior, security requirements, monitoring, and rollout criteria. Dragonfly support remains provisional and gated on client qualification.

@heymrbox
heymrbox requested a review from slin1237 as a code owner October 7, 2026 23:00
@github-actions github-actions Bot added documentation Documentation changes oep OME Enhancement Proposal labels Oct 7, 2026
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 96b0d251-eda3-48e5-98fe-5f26b292ecbc
📥 Commits

Reviewing files that changed from the base of the PR and between 3556453 and 627cfcb.

📒 Files selected for processing (2)
  • oeps/0012-optional-dragonfly-model-downloads/README.md
  • oeps/0012-optional-dragonfly-model-downloads/oep.yaml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

This PR adds OEP 0012, a provisional proposal for optional Dragonfly transport for Hugging Face PerNode model downloads. The proposal keeps direct downloads as the default and documents configuration, eligibility, fallback, security, and qualification requirements. It specifies no OME API or Dragonfly deployment changes.

Changes

Dragonfly transport proposal

Layer / File(s) Summary
Transport design and OEP metadata
oeps/0012-optional-dragonfly-model-downloads/README.md, oeps/0012-optional-dragonfly-model-downloads/oep.yaml
The OEP describes the proposed Dragonfly download design, including configuration, eligibility, failure handling, security, and qualification requirements. Its metadata marks the proposal as alpha and lists four transport metrics.

Priority: ⬇️ Low

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

Change: Other

Merge Risk: ⚪ Minimal · up to 627cf

This proposal keeps direct downloads as the default. No issue requiring resolution before merge was established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 627cf

The proposal does not enable new runtime behavior and retains direct downloads as the default. It establishes important integrity, credential-isolation, and recovery requirements, but their implementation and deployment qualification remain outstanding.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — If implemented and enabled, compromise of participating peers, origin-fetching roles, the control plane, or authorized socket callers could affect content and credentials entrusted to that Dragonfly confidentiality domain. Cache tags do not limit malicious access. The proposal requires separate deployments or reviewed isolation for mutually untrusted tenants; this PR does not activate that exposure.

Security Findings and Attack Paths

  • inferred — The deferred candidate does not substantiate acceptance of content without an authoritative digest. Conditional passing of an LFS digest to dfget is compatible with non-LFS Git blob verification, and the proposal requires checked metadata and authoritative final validation. This counterevidence does not resolve the recorded verification-receipt discrepancy or prove future implementation enforcement.

Trust Boundaries and Controls

  • observed — The proposal binds effective credentials and provenance to one Secret read, partitions cache tasks by endpoint and credential generation, and explicitly distinguishes partitioning from authorization. It requires controlled subprocess execution, credential redaction, origin certificate validation in every fetching role, and no bearer-token forwarding across origins. Token rotation does not revoke already cached bytes.

Resilience and Maintainability Implications

  • observed — The specified failure contract separates transport-integrity failures from ownership and filesystem safety failures. Direct fallback follows child termination and guarded cleanup, uses the same immutable request, and receives identical final verification. Cancellation, corrupt ownership state, or unsafe paths cannot initiate another writer; cleanup retains claims needed for authorized recovery.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: proposing optional Dragonfly transport for model downloads.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Reviewed this OEP in depth — no issues found.

Verified during review:

  • PR description follows the template with all required sections filled.
  • OEP number 0012 is unused; oep.yaml metadata (status, stage, metrics) is consistent with the README and with merged OEP precedent; the four metrics listed match the Observability table exactly.
  • The table of contents matches the document headings one-to-one.
  • In-repo references are accurate: the HFDownloadFunc callback shape matches directHfSource.download (pkg/modelagent/gopher_hf_source.go), the claim that removeInvalidFiles cannot remove files absent from a new manifest matches the implementation in pkg/modelagent/hf_snapshot_validation.go, the model_agent_download_transport_* metric names follow the existing model_agent_ prefix convention, and AlwaysDownload/ReuseIfExists/PerNode match the v1beta1 API enums.
  • External pins check out: client tag v1.5.7 resolves to ada71793… and server tag v2.5.2 to dbc47411… as stated; the v1.5.7 Hugging Face backend does unconditionally install NoVerifier for origin TLS; dfget v1.5.7 provides --hf-revision, DFGET_HF_TOKEN, DFGET_HF_BASE_URL, and --transfer-from-dfdaemon; and the native metrics do label vectors with the raw task tag, supporting the cardinality gate.

Severity counts: 🔴 0, 🟡 0, 🟣 0.

@heymrbox
heymrbox merged commit 64999d0 into main Oct 8, 2026
18 checks passed
@heymrbox
heymrbox deleted the heymrbox-oep-0012-dragonfly-review branch October 8, 2026 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation changes oep OME Enhancement Proposal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants