feat(controller): opt-in Prometheus scrape annotations on inference pods

[brandonliuli]

Upstream LLMKube proposal — inference-pod metrics discoverable by annotation scrapers

Problem

LLMKube runtimes expose Prometheus metrics on the inference container's app port
(llama.cpp llamacpp:*, vLLM vllm:*, TGI tgi_*) at /metrics. But making a
cluster-wide metrics agent scrape them today requires either:

• the inferencePodMonitor (needs prometheus-operator, which many clusters don't run), or
• hand-adding prometheus.io/* annotations to every InferenceService's spec.podAnnotations.

There's also a real footgun: the inference container's metrics port is named
http (the app port), which does not match the port-name regexes most
annotation-scrape configs use (metrics|http-metrics|metrics-port). So even
annotation-based discovery misses it unless prometheus.io/port is set explicitly.

Net: no zero-config path to inference metrics without prometheus-operator.

Proposal — two shapes (seeking maintainer preference)

Both extend the existing operator-default pattern on InferenceServiceReconciler
(cf. DefaultFSGroup, ModelCacheMode) and the merge point buildPodAnnotations
in internal/controller/deployment_builder.go. In both, a user's explicit
spec.podAnnotations value always wins.

Option A — opt-in scrape-annotation emission (reference PR implements this)

A --emit-scrape-annotations flag (chart metrics.emitScrapeAnnotations, default
off). When on, the operator adds prometheus.io/scrape=true, path=/metrics, and
port=<resolved spec.endpoint.port> to every inference pod.

• Pro: the operator knows the endpoint port, so prometheus.io/port is always
  correct — this dissolves the http-port-name footgun at the source, across all
  runtimes. Zero per-CR boilerplate.
• Con: opinionated toward prometheus.io-annotation scraping (though it's a
  no-op unless explicitly enabled).

Option B — generic operator-level default pod annotations

A --default-pod-annotations key=val,... flag (chart defaultPodAnnotations) that
seeds buildPodAnnotations at lowest precedence. The operator sets nothing metrics-
specific; the platform supplies the prometheus.io/* values (including a port).

• Pro: fully generic, reusable for any default annotation, not just metrics.
• Con: the port is static config — wrong for a fleet where InferenceServices
  use different endpoint.port values; the platform has to know/guess the port. Does
  not solve the port-name footgun on its own.

Recommendation

Option A — it fixes the actual discovery problem (the port) at the one place that
authoritatively knows the port, and stays a no-op until enabled. Option B is a nice
generic primitive but leaves the port problem to the caller. (They're not mutually
exclusive; A could later sit on top of a generic B.)

Reference PR

Implements Option A: reconciler field + --emit-scrape-annotations flag + chart
value/schema/arg + table-driven unit test (TestBuildPodAnnotations: emit off,
explicit port, default-8080 fallback, user-port-wins). go build, go vet, gofmt,
and the targeted test are green locally (envtest-gated tests need setup-envtest
binaries, unrelated).

---

AI-assisted: this change and text were drafted with an AI coding agent (Claude) and
are being reviewed, tested, and submitted by a human who takes DCO accountability for
the result.

[brandonliuli]

Design discussion + in-depth problem writeup (both options): #1367

[Defilan]

brandonliuli, welcome to LLMKube, and thanks for two solid PRs on your first day! The two-option framing here is genuinely useful and Option A is the right call. The opt-in wiring is clean end to end (chart to flag to reconciler to buildPodAnnotations), correctly off by default, and using a reconciler flag instead of a CRD field is the right pattern (no CRD regen needed). DCO and the PR-body AI disclosure both meet our policy.

One blocker, and it's small: the new endpointPort() helper reinvents port resolution and gets it wrong, which undercuts the PR's own thesis. The real container port is resolved in constructDeployment (deployment_builder.go:251-255) as ContainerPort -> Endpoint.Port -> backend.DefaultPort(), and DefaultPort() is runtime-specific: 80 for TGI, 8000 for vLLM, 30000 for SGLang, 8998 for personaplex, and only 8080 for llama.cpp/generic. endpointPort() (deployment_builder.go:310-315) ignores spec.containerPort and hardcodes 8080, so the zero-config path emits a wrong prometheus.io/port for every non-llama.cpp runtime, and mis-annotates any service using spec.containerPort, the exact footgun this PR exists to fix.

The fix is elegant: the fully-resolved port local already exists at deployment_builder.go:251 and is in scope at the buildPodAnnotations call (:342). Thread it through, buildPodAnnotations(isvc, r.EmitScrapeAnnotations, port), and delete endpointPort(). Please also add test cases for (a) a non-llama.cpp backend default and (b) spec.containerPort set, both currently produce a wrong annotation, so they'll pin the fix. And a note in values.yaml that this is an alternative to the PodMonitor, not to run alongside it against the same Prometheus (double-scrape). Happy to merge once the port comes from the resolved value.

[codecov]

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/main.go	0.00%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

[brandonliuli]

Thanks for the thorough review, @Defilan — you're right, and it undercut the PR's own thesis. Fixed in b0ffb6e:

The fix. buildPodAnnotations now takes the fully-resolved port that constructDeployment already computes (spec.containerPort → spec.endpoint.port → backend.DefaultPort()) and endpointPort() is deleted. So prometheus.io/port always equals the container's actual ContainerPort — correct for every runtime and honoring spec.containerPort.

Tests. Added constructDeployment specs that pin exactly the cases you called out, plus a couple of guards:

• vLLM (non-llama.cpp) → 8000 (was wrongly 8080)
• spec.containerPort set → wins over the runtime default
• spec.endpoint.port when containerPort unset
• llama.cpp default → 8080 (unchanged)
• invariant: the annotation always equals the exposed ContainerPort
• reconciler flag off → no scrape annotations

The existing buildPodAnnotations unit test is narrowed to its now-smaller job (format the given port; user value wins), with a regression guard that the port is emitted verbatim, never a hardcoded 8080.

Docs. values.yaml now notes this is an alternative to a PodMonitor/ServiceMonitor — don't run both against the same Prometheus or you double-scrape.

Full controller suite is green (676 specs; the 6 new ones: 6 Passed | 0 Failed). Ready for another look whenever you have a moment.

[Defilan]

This is exactly right, thank you — and the fix is better than a patch would have been. I verified it rather than taking the summary's word for it:

• buildPodAnnotations now takes the fully-resolved port, the call site passes the same value constructDeployment uses for the container, and endpointPort() is gone entirely (0 occurrences on head). Since you introduced it in commit 1 and removed it in commit 2, it nets out as never having existed — clean.
• The tests pin the cases that were actually broken: vLLM 8000, SGLang 30000, spec.containerPort winning, llama.cpp 8080 unchanged, and the invariant that the annotation equals the exposed ContainerPort. That last one is the one that keeps this from regressing.
• The values.yaml note that this is an alternative to a PodMonitor rather than a companion is a good catch to write down; double-scraped counters are a miserable thing to debug after the fact.

Approving. It'll merge once CI finishes (15 green, rest still running).

Second PR in two days, and you turned a real design discussion into a better implementation than the one you opened with. Genuinely glad to have you here — thanks for the care.