Skip to content

feat(server): protect Canary-managed model services - #39

Open
primowenjoy wants to merge 2 commits into
modelsphere:masterfrom
primowenjoy:feat/canary-arm-ownership
Open

primowenjoy wants to merge 2 commits into
modelsphere:masterfrom
primowenjoy:feat/canary-arm-ownership

Conversation

@primowenjoy

@primowenjoy primowenjoy commented Oct 29, 2026 •

Copy link
Copy Markdown

Canary arm LLMServices appear beside independent model deployments, but independently upgrading, deleting or editing their SLO can disrupt the experiment. This change returns managedBy ownership on list/status and rejects arm install-overwrite, apply/force, rollback, uninstall, autoscale and SLO operations with HTTP 409 and CANARY_MANAGED_SERVICE plus the parent identity.

Ownership comes from the live object. Incomplete or conflicting Canary metadata stays protected. Ordinary LLMService deletion uses UID/resourceVersion preconditions; failed discovery or missing LLMService read access blocks writes while preserving read fallback. Arm SLO reads are also refused because the release-name API cannot represent an inherited cross-namespace SLO correctly.

Canary read plans now merge the operator-owned canary-system layer last, so route, scaling and SLO values match the deployed policy instead of the unmodified input layers. A regression test covers final values and provenance.

Design: https://github.com/modelsphere/console-canary-plugin/blob/feat/llmservice-migration-pr/docs/design/llmcanary-llmservice-migration.md

Validation: go test -p 2 ./... passed with the CI-pinned model catalog fixture. Tests cover all blocked API operations, ownership inconsistencies, ordinary deletion identity and unavailable ownership reads. Console integration: modelsphere/console#108; canaryd: https://github.com/modelsphere/console-canary-plugin/pull/4; operator: https://github.com/modelsphere/llm-canary-operator/pull/8.

Manual CPU Mock validation reached Serving with two arms, completed 80/80 streaming Gateway requests, displayed token/TTFT metrics, and confirmed live HTTP 409 protection plus nested-target rejection. The browser retained ordinary-service upgrade/uninstall actions.

Kubernetes admission and SLO/configuration drift tracking remain separate follow-up work.

The disposable test experiment was stopped through the UI. Parent/child CRs, model/CART/proxy Pods and the experiment route were cleaned up; historical metrics and the production Mock service were retained.

@huskcass huskcass left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No blockers. go test -p 2 ./... passes locally at 30a42c7; go and gate are green, e2e was still pending when I looked.

Medium:

  • Uninstall (backend_llm.go, DeleteObserved): the resourceVersion precondition fails whenever the operator writes status between the Get and the Delete, which happens often on a service that's scaling or rolling. That conflict then goes through finishUninstallFailed, so the plan is marked failed and a failed uninstall is recorded even though nothing was deleted. On a conflict, restore the plan (as the NotFound branch does) and either re-Get, re-check ownership and retry once, or keep only the UID precondition and rely on the ownership re-check.
  • Failing closed now applies to helm too: a discovery error (cached for crdServedTTL, 30s) or a Forbidden on LLMServices blocks every install, apply, rollback, uninstall and SLO call, even for helm releases on clusters that never run Canary. That reverses the old "invisible CRD behaves like unserved" rule. The bundled chart always grants llmservices, so this mostly affects custom RBAC and API blips, but it should be a deliberate choice, noted in the PR or the README.

Low:

  • handleSLO runs refuseCanaryWrite for GET as well, so SLO reads for helm releases now 502 when LLMService access is forbidden. That contradicts "preserving read fallback". Only refuse arm reads when the object is actually seen, and skip the fail-closed branch for GET.
  • The ownership check in rollback, uninstall and SLO runs outside the apply lock, so an adoption between the check and the write isn't caught (apply is fine because it checks inside applyLocked). This is narrow, but worth a comment.
  • If the label-only fallback finds no canary-namespace label, it returns Namespace: "", and the 409 message reads LLMCanary /demo. Fall back to the object's namespace.

Verdict: the approach looks right. Fix the uninstall conflict handling before merge, and confirm the fail-closed behaviour for helm is intended.

— Review Golden

This branch has not been deployed

No deployments
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