[TimHunt12]

Three fixes for #211, from the last review round. Merging this into usage-metering adds them to #211.

1. The usage console fails closed (2c665d8)

The console shows everyone's usage, but with no issuer set it let every caller in as the same viewer, and that was the default whenever the NebariApp was off. An issuer without a client ID also started, accepting a token issued to any client in the realm.

• The server now needs an issuer and a client ID, or an explicit -dev (CONSOLE_DEV_MODE=true). Otherwise it refuses to start.
• The chart's new console.devMode (default false) sets dev mode, and the chart refuses to render the console with none of the NebariApp's sign-in, console.oidc or devMode. The new console.enabled (default true) can leave the console out.
• The dev values set devMode: true. The docs and the subchart README describe the new values.

2. OpenMeter works in the pack's namespace (22c8c35, replacing 52cabe5)

Packs need to let people choose namespaces, the pack's own included (see @dcmcand's comment on #211). 52cabe5 refused the pack's namespace; 22c8c35 fixes the two problems that layout had instead:

• A first helm install failed, because the ClickHouse password's pre-install hook ran before the chart created the pack's namespace. The password Job and its ServiceAccount, Role and RoleBinding are now ordinary resources, not hooks. ClickHouse and OpenMeter wait for the Secret (Kubernetes retries CreateContainerConfigError). The Job's name carries a hash of its spec, since a Job can't be changed after it's created. The Job only creates a missing Secret, so an upgrade keeps the existing password.
• keepDataOnUninstall kept nothing, because uninstalling deleted the namespace with the retained Postgres Cluster and volumes in it. With OpenMeter in the pack's namespace and keepDataOnUninstall on, the Namespace now carries helm.sh/resource-policy: keep and argocd.argoproj.io/sync-options: Delete=false.

openmeter.namespace is still required, because a subchart can't be handed the pack's computed namespace; the error names the pack's namespace. A separate namespace still works, and keeps OpenMeter's credentials away from the key manager until #215; the docs keep the label and Argo CD steps for it. The cross-namespace NetworkPolicy renders only when the namespaces differ.

3. 503, not 401, when Keycloak's signing keys can't be fetched (90f9cea, closes #217)

A token signed with a key the validator hadn't cached, while the JWKS couldn't be fetched (Keycloak down mid key rotation), was refused like a forged token: 401 "sign in again", which sends people round a sign-in loop that can't succeed. The shared validator now returns ErrKeysUnavailable in that case, including while the cooldown suppresses another fetch, and the console and the key manager both answer it with 503, like ErrNotReady.

Testing

• go vet and go test pass in shared, key-manager, usage-console/server and openmeter-setup. New tests cover the console's startup rules and the keys-unavailable case; the latter fails without the fix.
• Rendered with Helm 3.16.4 and 4.3.1: each console sign-in option and each namespace case renders or fails as described above. With openmeter.enabled off, the render is byte-identical to main, and helm lint passes.
• make usage-e2e passes on a local kind cluster at 90f9cea with OpenMeter in its own namespace, and at 22c8c35 with OpenMeter in the pack's namespace (OM_NAMESPACE=llm-operator-system). Each person's and team's requests reach OpenMeter, and the console (in dev mode) and OpenMeter's cost queries show the same totals. The console image refuses to start with no sign-in configured, and with an issuer but no client ID.
• With the whole chart on kind at 22c8c35:
  • a fresh helm install with OpenMeter in the pack's namespace, which didn't exist yet, succeeds;
  • helm uninstall keeps the namespace, the Postgres Cluster, the volumes and the password Secret;
  • reinstalling under the same name reads back a customer written before the uninstall;
  • with keepDataOnUninstall=false, uninstalling deletes the namespace;
  • upgrading from 6604308 (the hook version) keeps the existing password Secret.

🤖 Generated with Claude Code

[github-actions]

📄 Docs preview for usage-metering-review-fixes:
https://usage-metering-review-fixes.llm-serving-pack.pages.dev

[TimHunt12]

Thanks @dcmcand (copying my reply on #211 here so the change is on record with the code). Understood on both points: nothing merges without your review, and we don't need a fork.

On the namespace: I'll drop the "separate namespace only" change from this PR and make OpenMeter work in the pack's namespace, with an empty openmeter.namespace meaning the pack's namespace, as you asked originally. That means fixing the two problems Trent found in round 3: the first helm install failing because OpenMeter's pre-install hook runs before the pack's Namespace exists, and keepDataOnUninstall losing data when uninstall deletes that namespace. A separate namespace will still be an option, documented in architecture.mdx, for anyone who wants OpenMeter's Secrets out of the key manager's reach until #215. I'll update this PR and ping you when it passes e2e on kind.

[TimHunt12]

Per @aktech, @dcmcand is reviewing this one. Thanks, both.

[dcmcand]

Verdict: Request changes. The namespace and uninstall changes below need to land, plus two stale doc lines and a few inline items.

@TimHunt12 thanks for turning this around so quickly. Here's the way forward I'd like on the namespace, plus a change to how uninstall works.

Namespace: one helper both charts call

You're right that a parent can't hand a computed value to a subchart. But named templates are shared across a parent and its subcharts (the parent already calls the subchart's om.name in openmeterURL). The helper can't be used from the subchart today because it reads fullnameOverride and nameOverride, which the subchart can't see. If the helper reads only .Release and .Values.global, both charts can call it and get the same answer.

That also covers something the pack should have had from the start: an overridable namespace. So:

• global.namespace is the pack's namespace. Empty means <release>-nebari-llm-serving-system, which is what every install without a name override gets today.
• nebari-llm-serving.operatorNamespace reads only global.namespace and .Release.Name. fullnameOverride and nameOverride go back to naming resources only.
• The subchart's om.namespace uses namespace (that is, openmeter.namespace) if set, else the pack's helper when it's part of the pack (openmeter.inPack: true, set in the parent's values.yaml), else the release namespace. The subchart still installs on its own, the way dev/usage-e2e.sh does.
• openmeter.namespace becomes an optional override. Empty puts OpenMeter in the pack's namespace, and the fail goes away.

Here's a prototype against 22c8c35:

Prototype diff (3 files)

diff --git charts/nebari-llm-serving/charts/nebari-openmeter/templates/_helpers.tpl charts/nebari-llm-serving/charts/nebari-openmeter/templates/_helpers.tpl
index 986bc8d..70cd674 100644
--- charts/nebari-llm-serving/charts/nebari-openmeter/templates/_helpers.tpl
+++ charts/nebari-llm-serving/charts/nebari-openmeter/templates/_helpers.tpl
@@ -16,11 +16,18 @@ is the name plus a hash and must fit 63).
 {{- end -}}
 
 {{/*
-The namespace every resource of the subchart goes in: namespace, or the
-release namespace when it is empty.
+The namespace every resource of the subchart goes in: namespace; else, as
+part of the pack (inPack), the pack's namespace from the pack's own helper;
+else the release namespace.
 */}}
 {{- define "om.namespace" -}}
-{{- .Values.namespace | default .Release.Namespace -}}
+{{- if .Values.namespace -}}
+{{ .Values.namespace }}
+{{- else if .Values.inPack -}}
+{{ include "nebari-llm-serving.operatorNamespace" . }}
+{{- else -}}
+{{ .Release.Namespace }}
+{{- end -}}
 {{- end -}}
 
 {{- define "om.labels" -}}
diff --git charts/nebari-llm-serving/templates/_helpers.tpl charts/nebari-llm-serving/templates/_helpers.tpl
index 5a41297..f1135ef 100644
--- charts/nebari-llm-serving/templates/_helpers.tpl
+++ charts/nebari-llm-serving/templates/_helpers.tpl
@@ -49,10 +49,21 @@ app.kubernetes.io/instance: {{ .Release.Name }}
 {{- end }}
 
 {{/*
-Operator namespace (the namespace where the operator and key manager run)
+The pack's namespace: global.namespace, else <release>-nebari-llm-serving-system.
+It reads only .Release and .Values.global, which every chart in the release
+sees, so the bundled OpenMeter subchart calls this same helper. Not derived
+from fullnameOverride or nameOverride: those name resources, not the namespace.
 */}}
 {{- define "nebari-llm-serving.operatorNamespace" -}}
-{{- printf "%s-system" (include "nebari-llm-serving.fullname" .) | trunc 63 | trimSuffix "-" }}
+{{- with (.Values.global).namespace -}}
+{{ . }}
+{{- else -}}
+{{- $full := .Release.Name -}}
+{{- if not (contains "nebari-llm-serving" .Release.Name) -}}
+{{- $full = printf "%s-nebari-llm-serving" .Release.Name -}}
+{{- end -}}
+{{- printf "%s-system" ($full | trunc 63 | trimSuffix "-") | trunc 63 | trimSuffix "-" -}}
+{{- end -}}
 {{- end }}
 
 {{/*
@@ -70,15 +81,10 @@ openmeter.enabled.
 {{- end }}
 
 {{/*
-The bundled OpenMeter's namespace: openmeter.namespace, which the subchart
-installs into. Required: a subchart cannot be handed the pack's computed
-namespace, and the release namespace is wherever helm happened to run.
+The bundled OpenMeter's namespace: openmeter.namespace, else the pack's.
 */}}
 {{- define "nebari-llm-serving.openmeterNamespace" -}}
-{{- if not .Values.openmeter.namespace -}}
-{{- fail (printf "openmeter.enabled needs openmeter.namespace: the pack's namespace, %s, or another namespace that exists" (include "nebari-llm-serving.operatorNamespace" .)) -}}
-{{- end -}}
-{{- .Values.openmeter.namespace -}}
+{{- .Values.openmeter.namespace | default (include "nebari-llm-serving.operatorNamespace" .) -}}
 {{- end }}
 
 {{/*
diff --git charts/nebari-llm-serving/values.yaml charts/nebari-llm-serving/values.yaml
index 9b12391..cf0207e 100644
--- charts/nebari-llm-serving/values.yaml
+++ charts/nebari-llm-serving/values.yaml
@@ -1,3 +1,8 @@
+global:
+  # namespace - the pack's namespace, which the bundled OpenMeter shares unless
+  # openmeter.namespace is set. Empty: <release>-nebari-llm-serving-system.
+  namespace: ""
+
 # createNamespace controls whether the chart provisions the operator's own
 # namespace and labels it `nebari.dev/managed=true` (required by the
 # validating webhook). Set to false when something else owns the namespace
@@ -186,6 +191,8 @@ keyManager:
 # subchart's; see charts/nebari-openmeter/values.yaml.
 openmeter:
   enabled: false
+  # Set by the pack, not by you: tells the subchart to default to the pack namespace.
+  inPack: true
   # namespace - where the bundled OpenMeter runs; required with
   # openmeter.enabled. Either the pack's own (<fullname>-system), or another
   # namespace that exists. In the pack's namespace, the key-manager can read

I've only rendered it so far:

• With default values it's byte-identical to main, for release names r, nebari-llm-serving and my-nebari-llm-serving-prod, under Helm 3.19.0 and 4.3.0.
• With OpenMeter on and nothing else set, all 44 namespaced resources land in r-nebari-llm-serving-system.
• global.namespace=acme moves the pack and OpenMeter together.
• openmeter.namespace=usage-om moves only OpenMeter, and the collector's URL points across.
• fullnameOverride=llm-operator renames resources but leaves the namespace alone.
• The subchart on its own still installs into its release namespace.

We'll test it on a cluster before merge.

Along with the helper, we need:

• An Upgrading note. Anyone on 0.1.2 to 0.1.4 who set fullnameOverride or nameOverride has a namespace named after it (foo-system, or <release>-bar-system). They need to set global.namespace to that namespace before upgrading. Otherwise the upgrade renders a Namespace with a new name, and Helm deletes the old one (pkg/kube/client.go:436-455 at v3.19.0 deletes anything in the old manifest that's missing from the new one), taking their LLMModels, API keys and provider Secrets with it. Argo CD does the same with automated prune on.
• dev/usage-e2e.sh renders the collector and the cross-namespace policy with fullnameOverride=llm-operator (lines 92 and 112) and expects them in llm-operator-system. Both helm template calls need --set global.namespace=$NS.
• Docs. global.namespace in configuration.mdx and in the single-namespace section of architecture.mdx, and openmeter.namespace described as optional. These spots call the pack's namespace <fullname>-system, which is no longer true when an override is set: usage.mdx:111 and :131, configuration.mdx:131, values.yaml:190, and the subchart's values.yaml:164.

Uninstall: if the pack creates the namespace, uninstall removes it

Please drop the helm.sh/resource-policy: keep and Delete=false annotations from the pack's Namespace (namespace.yaml:21-27). Keeping the namespace keeps much more than OpenMeter's data:

• The API-key Secrets have no owner references, and only the LLMModel finalizer deletes them (llmmodel_controller.go:281-286). After a reinstall, the operator keeps their data and rebuilds the SecurityPolicy from them, so keys from before the uninstall work again.
• The LLMModel CRs survive, since the CRDs live in crds/. The model Deployments, Services and PVCs are owned by those CRs (llmmodel_controller.go:119,157,310-328), so the model pods keep running on their GPUs with no operator or key manager left.

On main, uninstall deletes the namespace and all of that with it, and we should keep that behavior. The rule is: if the pack created the namespace, uninstall removes it.

That means keepDataOnUninstall only keeps data when OpenMeter's namespace outlives the release: a separate namespace, or createNamespace: false with the namespace created by something else. In the default layout it can't keep anything. We need to say so clearly:

• Docs. The "Uninstall and reinstall" section of usage.mdx, the keepDataOnUninstall row in configuration.mdx, and the subchart README's uninstall section.
• A templates/NOTES.txt warning. Print it when OpenMeter is in a namespace the chart creates and keepDataOnUninstall is on. Templates can't emit a non-fatal warning (only fail and required, and both abort), and a fail would break the default install. Argo CD renders with helm template and never shows NOTES.txt, so the docs carry the message for Argo CD users.
• An Argo CD example. If the Application creates the namespace (CreateNamespace=true, with managedNamespaceMetadata setting nebari.dev/managed: "true") and the values set createNamespace: false, openmeter.enabled: true and openmeter.keepDataOnUninstall: true, deleting the Application should leave the namespace. Argo CD doesn't track a namespace it creates that way (controller/sync_namespace.go:38-39 at v3.5.6), so it never deletes it. The Postgres Cluster's keep annotation and the StatefulSets' whenDeleted: Retain then keep the data. Let's add that next to examples/argocd-application.yaml and confirm it on the cluster. One case needs testing in particular: an existing Application switched from createNamespace: true to false. The chart's Namespace already carries Argo CD's tracking annotation there, so I believe deleting the Application would still delete the namespace unless that annotation is removed first.
• Update the existing examples. examples/argocd-application.yaml has drifted:
  • It pins chart 0.1.2 and doesn't render against the current chart: frontend.keycloak.url is now required.
  • It sets CreateNamespace=true and managedNamespaceMetadata, but leaves the chart's createNamespace on. The chart's Namespace takes precedence (gitops-engine/pkg/sync/sync_context.go:1161-1176), so the managedNamespaceMetadata labels are ignored, and deleting the Application deletes the namespace.
  • The two examples should tell one story. In the existing one the chart owns the namespace, so I believe we can drop CreateNamespace=true and managedNamespaceMetadata there. In the new OpenMeter one, Argo CD owns it.
  • With global.namespace, the Application's destination.namespace and global.namespace have to match. Both examples should show that.

---

Key: 🛑 Blocker (must fix before merge) · 💡 Suggestion (improvement, not blocking) · ❓ Question (needs author input) · ❤️ Kudos

[dcmcand]

💡 Suggestion: The Job name hashes only the image and command, so a change to the rest of the pod template keeps the old name.
Hash the rendered pod template, the way the price-sync Job does at openmeter.yaml:185.
A Job's pod template is immutable. If a later chart changes resources or securityContext with the same setup image, helm upgrade fails with "field is immutable". The comment at line 12 says the name carries "a hash of the spec", which needs the same fix.

[dcmcand]

💡 Suggestion: No key-manager test covers the new ErrKeysUnavailable branch.
Add a middleware test case that expects 503 and Retry-After for ErrKeysUnavailable.
Removing || errors.Is(err, oidc.ErrKeysUnavailable) leaves go test ./internal/api/ green. Only the console's identity_test.go pins this mapping today.

[dcmcand]

💡 Suggestion: "Create a separate namespace first" reads as a step for every install, but the pack's namespace is allowed again.
Start the sentence with "To use a separate namespace, create it first".

[dcmcand]

❤️ Kudos: newIdentity refuses -dev together with an issuer, so a dev flag left on in a real deployment can't turn off sign-in.
The dev and issuer both set case in TestNewIdentityFailsClosed fails without this branch.

[dcmcand]

❤️ Kudos: console.enabled removes the console's Deployment, Service, NebariApp and NetworkPolicy together, so OpenMeter can run without a UI that needs sign-in.

[dcmcand]

🛑 Blocker: Line 146 still says the ClickHouse password Secret is "made by a hook".
Change "made by a hook" to "made by a Job".
The Secret still isn't part of the release, so the rest of that sentence holds.

[dcmcand]

🛑 Blocker: Line 83 still calls the ClickHouse password step a "hook".
Change "the ClickHouse password hook" to "the ClickHouse password Job".