fix(keycloak): readiness could not fail on the database, and the measured budget
Two corrections found while verifying the first CI run, not from the plan. THE DEFAULT THAT MADE THE READINESS PROBE TOOTHLESS. Keycloak's DATABASE health check is the one row of upstream's "Available Checks" table whose "Requires Metrics" column says yes; Cluster, Graceful shutdown and Keycloak Initialized say no. So with metrics-enabled=false - this chart's default until now - /health/ready answers 200 while the database is gone, and the pod keeps taking logins it cannot serve. The probe path was right and the aggregate behind it contained nothing external, which is the urlaubsverwaltung defect (#218) in a new hiding place: not a health group this time, but an option named for observability, where nobody looks when reviewing a probe. keycloak.metrics.enabled therefore defaults to TRUE, and its comment argues about readiness rather than about scraping. The price is stated at the value: /metrics then answers on the management port, unauthenticated, carrying realm names, client ids and login counters - on a port that is ClusterIP-only and in no Ingress. Switching it off is now documented as a decision about whether readiness can fail at all. The CI fixture keeps it on and says why, so a green run really does touch the database. THE STARTUP BUDGET NOW CARRIES ITS MEASUREMENT. From the install-test of PR #87 on the Gitea docker runner (k3s v1.35.5, two replicas, empty database, image pulled during the run): 71.8 seconds from `helm install` to BOTH pods Ready - first at ~70.8s, second one second later, 0 restarts. The 600s budget stays, with the reason for the headroom written down: everything in that run was the cheap case, and a first install against a real database serializes the second replica behind the first's Liquibase migration lock instead of letting it arrive a second later. AND AN OVERCLAIM OF MY OWN, REMOVED. ci/keycloak/test-values.yaml said a green run proves jdbc-ping found the second replica. It does not: readiness reports one node's own health, so a node that never found its peer is Ready, healthy and alone - and two such nodes are two islands that each believe they are the cluster. What the test does rule out is the louder half, a replica that cannot start or cannot share the database. The Infinispan view has to be read out of the log, which is what step 2 of NOTES.txt tells the operator to do, now with the note that no probe and no install-test replaces that step. Measurement point 4 of the plan therefore stays open. Part of #83
This commit is contained in:
1 parent
4c1cff1744
commit
320257c742
6 files changed
+113
-40
No files matched your search
@@ -52,11 +52,14 @@ FIRST INSTALL - AND STEP 4 IS MANDATORY, NOT ADVICE
|
||||
creates the entire schema through Liquibase, and with {{ $replicas }} replica(s)
|
||||
the second waits on the first's migration lock.
|
||||
|
||||
2. Check that the cluster really formed - this is what the second replica is
|
||||
for, and a silent failure here leaves two independent islands:
|
||||
2. Check that the cluster really formed. THIS STEP IS NOT OPTIONAL AND NO
|
||||
PROBE REPLACES IT: a node that never found its peer is Ready, healthy and
|
||||
alone, and two such nodes are two islands that each believe they are the
|
||||
cluster. The install-test cannot catch it either.
|
||||
kubectl -n {{ .Release.Namespace }} logs deployment/{{ $deploy }} | grep -i "ISPN000094\|view accepted\|members"
|
||||
Look for a view with {{ $replicas }} members. One member per pod means
|
||||
jdbc-ping did not find the peer.
|
||||
jdbc-ping did not find the peer - check that both pods can reach each
|
||||
other on port {{ $settings.ports.jgroups | int64 }} and that they really share one database.
|
||||
|
||||
3. Log in at {{ if $adminHostname }}{{ include "keycloak.adminUrl" . }}/admin{{ else if .Values.ingress.enabled }}{{ include "keycloak.frontendUrl" . }}/admin{{ else }}the in-cluster address, port-forwarded{{ end }}
|
||||
with the bootstrap credentials from the vault item above.
|
||||
@@ -140,6 +143,14 @@ TRAPS WORTH KNOWING BEFORE YOU NEED THEM
|
||||
unset. Read its comment in values.yaml before setting it: upstream also
|
||||
warns about cache invalidation between differently-named clusters.{{ end }}
|
||||
|
||||
- READINESS ONLY CHECKS THE DATABASE WHILE metrics-enabled IS TRUE. Upstream's
|
||||
table of health checks has a "Requires Metrics" column and the DATABASE
|
||||
check is the row that says yes. So with keycloak.metrics.enabled false,
|
||||
/health/ready answers 200 with the database gone and the pod keeps taking
|
||||
logins it cannot serve. This release has it {{ if $settings.metrics.enabled }}ON, which is the chart default
|
||||
and the reason the default is what it is.{{ else }}OFF, so the readiness probe
|
||||
cannot currently detect a database outage.{{ end }}
|
||||
|
||||
- THERE IS NO ConfigMap IN THIS CHART, so no checksum/config annotation. That
|
||||
is not an omission: Keycloak is configured entirely through options and
|
||||
KC_* variables, so there is no config file whose change could fail to
|
||||
|
||||
@@ -180,7 +180,7 @@ Usage: {{ $settings := fromYaml (include "keycloak.settings" .) }}
|
||||
"database" "keycloak"
|
||||
"pool" (dict "initialSize" 2 "minSize" 2 "maxSize" 20))
|
||||
"bootstrapAdmin" (dict "enabled" true)
|
||||
"metrics" (dict "enabled" false)
|
||||
"metrics" (dict "enabled" true)
|
||||
"features" (list)
|
||||
"cache" (dict "clusterName" nil)
|
||||
"ports" (dict "http" 8080 "management" 9000 "jgroups" 7800)
|
||||
@@ -392,11 +392,25 @@ away, which is also why the wrong path would look entirely plausible in a diff.
|
||||
{{- end -}}
|
||||
|
||||
{{/*
|
||||
READINESS: /health/ready - which DOES include the database, and must. A node
|
||||
that cannot reach its database answers every login with an error, so no Service
|
||||
READINESS: /health/ready - which includes the database, and must. A node that
|
||||
cannot reach its database answers every login with an error, so no Service
|
||||
should route to it. This is the other half of guideline 12.3: a dependency-free
|
||||
liveness path is only correct when readiness really does hang on the
|
||||
dependency.
|
||||
|
||||
AND THE PATH ALONE DOES NOT GUARANTEE THAT, which is the part worth reading
|
||||
before anybody "cleans up" keycloak.metrics.enabled. Upstream's table of
|
||||
available checks carries a "Requires Metrics" column, and the DATABASE check -
|
||||
"Returns the status of the database connection pool" - is the row that says
|
||||
YES; Cluster, Graceful shutdown and Keycloak Initialized say no. So with
|
||||
metrics-enabled=false this endpoint still answers 200 while the database is
|
||||
gone, and the pod keeps taking logins it cannot serve.
|
||||
|
||||
That is exactly the urlaubsverwaltung shape (#218): the probe path was right
|
||||
and the aggregate behind it contained nothing external. Here the dependency
|
||||
hides behind an option whose name says observability, which is why
|
||||
keycloak.metrics.enabled defaults to TRUE and why its comment in values.yaml
|
||||
argues about readiness rather than about scraping.
|
||||
*/}}
|
||||
{{- define "keycloak.readinessProbe" -}}
|
||||
{{- $defaults := dict
|
||||
|
||||
Reference in new issue
Block a user