Skip to content

fix(inference): warn when modelCache.claimName is silently ignored#966

Merged
Defilan merged 1 commit into
defilantech:mainfrom
joryirving:fix/model-cache-claimname-ignored-warning
Jul 3, 2026
Merged

fix(inference): warn when modelCache.claimName is silently ignored#966
Defilan merged 1 commit into
defilantech:mainfrom
joryirving:fix/model-cache-claimname-ignored-warning

Conversation

@joryirving

Copy link
Copy Markdown
Collaborator

What

Emit a ModelCacheClaimIgnored warning event when spec.modelCache.claimName is set but the cache path is inactive — caching disabled on the operator (chart modelCache.enabled: false / --model-cache-path unset) or the model has no effective cache key (local file:// source, or a remote model not yet fingerprinted).

Why

Follow-up from the #960 review: in these cases the pod silently fell back to an emptyDir and re-downloaded the model on every restart, with no event or condition. #960 already warned for the pvc://-source conflict; this covers the symmetric case.

Fixes #965

How

  • The check mirrors the useCache gate in constructDeployment; it lives in a warnIgnoredModelCacheClaim helper next to the other claimName logic (extracted rather than inlined because the extra branches pushed Reconcile over the gocyclo limit). Event messages say why the claim was ignored and that the pod gets an ephemeral emptyDir.
  • Envtest reconcile tests with a FakeRecorder assert every ModelCacheClaimIgnored variant — including the pre-existing pvc:// case, which was previously unasserted — plus a no-warning case when caching is active.
  • docs/MODEL-CACHE.md: updated the ignored-claim bullet and added a note that claimName is mutable and the old claim is not garbage-collected.
  • Skipped the ModelCachePVCNotFound condition-reason nice-to-have: the Degraded reason is hardcoded in the shared updateStatusWithSchedulingInfo switch, so a specific reason means plumbing a new parameter through it — happy to do as a separate PR if wanted.

AI assistance: authored by Claude (Fable 5) operating under the maintainer's direction; reviewed before submitting.

Checklist

  • Tests added/updated
  • make test passes locally (go test ./internal/controller/... with envtest assets, 506/506)
  • make lint passes locally (golangci-lint run ./internal/... ./api/..., 0 issues)
  • Commit messages follow conventional commits
  • All commits are signed off (git commit -s) per DCO
  • AI assistance (if any) is disclosed above, per CONTRIBUTING.md
  • Documentation updated (if user-facing change)

spec.modelCache.claimName only takes effect on the download-into-cache
path. When that path is inactive -- caching disabled on the operator
(chart modelCache.enabled=false / --model-cache-path unset) or a model
without an effective cache key (local file:// source, or a remote model
not yet fingerprinted) -- the pod silently fell back to an emptyDir and
re-downloaded the model on every restart, with no event or condition.

Emit the same ModelCacheClaimIgnored warning event already used for the
pvc://-source conflict, saying why the claim was ignored and that the
pod gets an ephemeral emptyDir instead. The check lives in a
warnIgnoredModelCacheClaim helper next to the other claimName logic
(extracted rather than inlined to keep Reconcile under the gocyclo
limit). Envtest reconcile tests now assert every ModelCacheClaimIgnored
variant (the pvc:// case was previously unasserted) plus the no-warning
happy path. Also document that claimName is mutable and the old claim
is not garbage-collected.

AI assistance: authored by Claude (Fable 5) operating under the
maintainer's direction; reviewed before submitting.

Fixes defilantech#965

Signed-off-by: Jory Irving <jory.irving@stackadapt.com>
@joryirving joryirving requested a review from Defilan as a code owner July 3, 2026 14:16
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Defilan Defilan 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.

Review

Approve. This closes the silent-drop gap I flagged on #960 (filed as #965): when spec.modelCache.claimName is set but caching is off, the user now gets a ModelCacheClaimIgnored warning instead of a silent emptyDir.

Verified:

  • warnIgnoredModelCacheClaim fires the existing ModelCacheClaimIgnored reason across all three ignore cases (pvc:// source, ModelCachePath == "", empty cache key). The switch conditions are the exact negation of the real useCache gate (effectiveModelCacheKey(model) != "" && r.ModelCachePath != ""), so the warning mirrors the actual storage path chosen.
  • Wired unconditionally in Reconcile before reconcileDeployment (not dead code), and it back-fills the previously-inline pvc:// warning.
  • No false positive on the happy path (claimName set + caching on), asserted by a dedicated NotTo(ContainElement(...)) test; the envtest covers all four cases with a FakeRecorder.
  • Tightly scoped (helper extraction + call-site swap + tests + a docs bullet), DCO signed.

One cosmetic note: the test/doc reference #928 rather than the silent-drop follow-up #965 this resolves; I'll close #965 pointing here.

Thanks for picking this up.

@Defilan Defilan merged commit d49cd22 into defilantech:main Jul 3, 2026
23 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 3, 2026
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.

[BUG] InferenceService: spec.modelCache.claimName silently ignored when caching is off

2 participants