Repository navigation
fix: restore the default Endpoint Picker image - #186
Conversation
dcmcand
left a comment
dcmcand
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The image fix is right and we need it: main still ships the removed llm-d-inference-scheduler:v0.8.0, so every new EPP pod fails to pull. I checked the compatibility claims against llm-d-router v0.9.0 and they hold. The default-image check needs two parser fixes before it can run against current main, and the branch needs a rebase.
🛑 Merge conflict in .github/workflows/test.yaml. main has gained jobs since this branch was cut. Please rebase onto main.
💡 Let the EPP's Role read the llm-d.ai inference group too.
- In llm-d-router v0.9.0,
InferenceObjectiveandInferenceModelRewritemoved tollm-d.ai, and the EPP prefers that group when its CRDs are installed (pkg/epp/server/controller_config.go:66-77at v0.9.0). - The Role in
inferencepool.go(around line 323) only grantsinference.networking.x-k8s.io. - On a cluster with the new CRDs installed, the informers would likely fail to sync again, the crash loop the comment above that rule describes. That's inferred from the source, not observed.
- Fix: add a rule for
llm-d.aiwith the same resources and verbs, and update that comment, which still says "v0.8.x".
💡 v0.9.0 is a fine pin for this fix. Moving to v0.10.0, v0.11.0 or v1.0.0 belongs in a separate PR.
What I verified (static reading at 8d4a2fc4, plus curl; the checker itself wasn't run):
llm-d.ai/v1alpha1is registered at v0.9.0 (apix/config/v1alpha1/zz_generated.register.go).max-score-pickerandsingle-profile-handlerare registered (cmd/epp/runner/runner.go:504,507).- Every flag the operator passes exists (
pkg/epp/server/options.go). llm-d-router-endpoint-picker:v0.9.0returns 200 to an anonymous manifest HEAD, and the old image returns 403.
| } | ||
| return strings.TrimSpace(image), nil | ||
| case map[string]any: | ||
| repository, err := requiredString(image, "repository", pathText) |
There was a problem hiding this comment.
🛑 On current main this errors before any image is checked, so the new CI job fails on every run.
main's values.yaml has openmeter.console.image: {tag: ""} and openmeter.setup.image: {tag: ""}, with no repository. The release workflow fills those tags in at release time. requiredString returns openmeter.console.image.repository is required, and main.go exits.
Please skip mappings that have no repository or have an empty tag (those are pinned at release time), and add a defaults_test.go case for that shape.
There was a problem hiding this comment.
Fixed in 9ca78cc. A mapping with an empty tag or no repository is now skipped and shows up as a SKIP line (SKIP openmeter.console.image: empty tag). A repository with no tag key at all is still an error, so an entry that leaves the key out still fails the check. TestParseDefaultImagesSkipsReleasePinnedMappings covers the {tag: ""} shape from main, and run locally against this branch's values.yaml the checker now passes 7 images and skips these two.
|
|
||
| name := image | ||
| manifestReference := "" | ||
| if at := strings.LastIndex(image, "@"); at > lastSlash { |
There was a problem hiding this comment.
🛑 A reference that has both a tag and a digest keeps the :tag in the repository path.
main has usage.collector.image.tag: "v1.0.0-beta.234@sha256:cdc3...". That builds ghcr.io/openmeterio/benthos-collector:v1.0.0-beta.234@sha256:..., which requests https://ghcr.io/v2/openmeterio/benthos-collector:v1.0.0-beta.234/manifests/sha256:.... That URL returns 404, while the same URL without :v1.0.0-beta.234 returns 200.
After splitting off the digest, please also strip a :tag that follows the last / from name, and add a test case for repo:tag@sha256:....
There was a problem hiding this comment.
Fixed in 9ca78cc. When a digest is present, a :tag after the last / is now dropped from the name, and a registry port such as localhost:5000 is left alone. I added tag and digest, registry port with tag and digest and registry port with digest cases to TestParseImageReference, and the real usage.collector.image reference went from HTTP 404 to OK.
| if parsed["kind"] != "EndpointPickerConfig" { | ||
| t.Errorf("expected parsed kind=EndpointPickerConfig, got %v", parsed["kind"]) | ||
| } | ||
| if parsed["apiVersion"] != "llm-d.ai/v1alpha1" { |
There was a problem hiding this comment.
❤️ Nice touch. This assertion pins the move to the non-deprecated config API, so a later revert to the deprecated inference.networking.x-k8s.io group fails a test instead of only logging a deprecation warning.
main's values.yaml now has openmeter.setup.image and openmeter.console.image with an empty tag and no repository. The release workflow pins those tags and the subchart supplies the repository, so the check cannot build a reference for them. The parser now returns such mappings with a SkipReason, and the CLI prints a SKIP line for each. A mapping that has a repository but no tag key is still rejected. A reference with both a tag and a digest kept the tag in the repository path, so usage.collector.image requested a manifest URL that returns 404. The tag before the digest is now dropped; a registry port stays.
llm-d-router v0.9.0 reads InferenceObjective and InferenceModelRewrite from llm-d.ai when those CRDs are installed, and from inference.networking.x-k8s.io otherwise (pkg/epp/server/controller_config.go). The EPP Role only granted the legacy group. It now also grants read access to the two llm-d.ai CRDs that v0.9.0 ships, matching the router's own Helm chart. The operator must already hold any permission it grants (nebari-dev#121), so the chart ClusterRole and dev/manifests/operator.yaml get the same rule. Without it the API server rejects the EPP Role as an escalation.
The operator now writes the built-in EPP config as llm-d.ai/v1alpha1. llm-d-inference-scheduler v0.8.0 loads its config with the gateway-api-inference-extension v1.5.0 loader, which only registers inference.networking.x-k8s.io, so an install that pins a mirror of the old image needs to move to the new one.
llm-d-router-endpoint-picker is the image name. The EPP deployment template the specs follow is in llm-d-router's routerlib chart (config/charts/routerlib/templates/_deployment.yaml at v0.9.0).
8d4a2fc to
fe9c609
Compare
|
I rebased onto main and kept both c70fe68 lets the EPP Role read fc9ce13 adds a short entry to the Upgrading page. The gateway-api-inference-extension v1.5.0 loader that
|
dcmcand
left a comment
dcmcand
left a comment
There was a problem hiding this comment.
Verdict: Approve. Every item from my last review is fixed, and one optional suggestion is inline.
💡 Suggestion: No test checks that the operator ClusterRoles hold every rule in buildEPPRole, so a new EPP rule can break the Role again. Please open a follow-up issue for a test that compares them. This predates the PR and does not block it.
| Previous item | Status | Evidence |
|---|---|---|
🛑 Merge conflict in test.yaml |
Fixed | Both jobs present, merges cleanly into main |
🛑 {tag: ""} mappings crash the checker |
Fixed in 9ca78cc |
Checker passes at fe9c609: 7 OK, 2 SKIP |
| 🛑 Tag kept in the path when a digest is present | Fixed in 9ca78cc |
usage.collector.image now returns OK |
💡 llm-d.ai rule for the EPP Role |
Fixed in c70fe68 |
Rule in the Role and both ClusterRoles, test fails without it |
| 💡 Keep the v0.9.0 pin | Followed | Pin unchanged |
Key: 🛑 Blocker (must fix before merge) · 💡 Suggestion (improvement, not blocking) · ❓ Question (needs author input) · ❤️ Kudos
| if tag, ok := image["tag"]; ok && isBlank(tag) { | ||
| return "empty tag" | ||
| } | ||
| if isBlank(image["repository"]) { |
There was a problem hiding this comment.
💡 Suggestion: skipReason skips any mapping without a usable repository, so a mistyped key passes CI as a SKIP line.
Drop the no repository branch and the sidecar.image case in TestParseDefaultImagesSkipsReleasePinnedMappings.
My last review asked for this branch, but it was never needed: both openmeter images have tag: "", so the empty tag branch already skips them. With it, repositry: x plus a real tag prints SKIP instead of failing.
| go test ./internal/controller/reconcilers -run TestGatewayAdmission -v | ||
| ``` | ||
|
|
||
| ### Upgrading from 0.1.4 or earlier: a mirrored Endpoint Picker image |
There was a problem hiding this comment.
❤️ Kudos: This section tells anyone running a mirror of the old image exactly which image to mirror before upgrading. It is accurate: v0.8.0 loads its config with the GIE v1.5.0 loader, which registers only inference.networking.x-k8s.io.
|
Thanks for this, @Keerthirajan58. Every new model on |
Summary
llm-d-inference-scheduler:v0.8.0default withllm-d-router-endpoint-picker:v0.9.0in Helm and the operator fallbackllm-d.ai/v1alpha1APIvalues.yamlinferenceobjectivesandinferencemodelrewritesinllm-d.ai, with the same rule in the operator ClusterRoleWhy
The old GHCR package no longer grants an anonymous pull token, so new and rescheduled EPP pods fail with
ImagePullBackOff. Updating the default restores the Endpoint Picker deployment. The CI check exercises the public Registry v2 Bearer flow and fails if any checked default becomes unavailable.The availability check discovers both scalar image values and
repository/tagmappings. It uses no Docker or GitHub package credentials, and reports missing manifests, access denial, rate limits, and registry failures separately. A reference with both a tag and a digest is checked by digest. Mappings with an empty tag or no repository, such asopenmeter.setup.imageandopenmeter.console.image, cannot be checked fromvalues.yamlalone (the release workflow pins those tags), so the check prints a SKIP line for them.llm-d-router v0.9.0 reads InferenceObjective and InferenceModelRewrite from
llm-d.aiwhen those CRDs are installed, so the EPP Role grants read access there as well as in the legacy group. The operator ClusterRole carries the same rule because Kubernetes rejects a Role that grants permissions its creator does not hold (#121).llm-d-inference-scheduler:v0.8.0cannot load thellm-d.ai/v1alpha1config, so an install that setsdefaults.epp.imageto a mirror of the old image needs to mirror the new one. The Upgrading page now says so.Verification
cd operator && make testwith the released AI Gateway v1.1.0 CRDs, as in CIcd operator && make lintcd operator && go test -race ./internal/imagecheck -count=1helm lint charts/nebari-llm-serving/with the chart's required valueshelm templateconfirmsLLM_DEFAULT_EPP_IMAGE=ghcr.io/llm-d/llm-d-router-endpoint-picker:v0.9.0and thellm-d.airule in the operator ClusterRoleactionlint .github/workflows/test.yamlnpm test,npm run build, andscripts/check-links.shllm-d.airesourcesKubernetes testing used envtest and rendered manifests rather than a live cluster.
Fixes #174