Repository navigation
feat(server): protect Canary-managed model services - #39
Open
primowenjoy wants to merge 2 commits into
Open
primowenjoy wants to merge 2 commits into
primowenjoy wants to merge 2 commits into
Conversation
3 tasks done
huskcass
reviewed
Oct 29, 2026
huskcass
left a comment
Collaborator
There was a problem hiding this comment.
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 throughfinishUninstallFailed, 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:
handleSLOrunsrefuseCanaryWritefor 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-namespacelabel, it returnsNamespace: "", and the 409 message readsLLMCanary /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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Canary arm LLMServices appear beside independent model deployments, but independently upgrading, deleting or editing their SLO can disrupt the experiment. This change returns
managedByownership on list/status and rejects arm install-overwrite, apply/force, rollback, uninstall, autoscale and SLO operations with HTTP 409 andCANARY_MANAGED_SERVICEplus 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-systemlayer 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.