From 8f10d202e62f02f17d327b8abe7d4f61fb79765a Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sun, 6 Sep 2026 21:41:14 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8:=20give=20the=20shipped=20manifes?= =?UTF-8?q?t=20probes=20and=20a=20stop=20grace=20period?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The manifest in this repository had no probes at all. A pod was sent traffic as soon as its container was running, whether or not the database it needs was reachable, and it was stopped with whatever grace period Kubernetes defaults to rather than one chosen against what this process actually spends shutting down. It now mounts both probes, at the endpoint that answers each question: readiness at /ready, which fails while a dependency is unreachable, and liveness at /health, which is a bare 200 because restarting a process whose database is down turns one outage into a crash loop. Both skip the rate limiter, which is why that had to land first. timeoutSeconds is 3, not the default 1. The handler allows its checks two seconds, so at the default a database answering in 1.2s would be recorded as a failed check while the handler was returning 200 - the probe would be failing on the orchestrator's stopwatch, not on its own. The comment beside that constant said the constraint was the polling period; the constraint is the per-check timeout, and it is now written down correctly. terminationGracePeriodSeconds is 30, against a shipped budget of 0 + 5 + 3. Raising drain means raising this too, in the same commit; the check that notices when somebody does not arrives two commits from here. replicas stays at 1, and the comment says why that makes the drain window worth nothing: there is nowhere to send the traffic this pod stops taking. Raising it needs one more change than the number - the volume is shared by every replica and the log path lives on it, so a second pod would append to the same rotating file. The reason not to raise it is not the one the review assumed: the claim was that the PVC is ReadWriteOnce, and it is not, it is ReadWriteMany on nfs-csi. There is no preStop hook. How long one should sleep depends on how fast the thing in front removes this instance, which the repository cannot know, and a manifest carrying both a preStop sleep and a drain window is the double-counting trap - the budget would be spent twice and the start-up line would report half of it. --- app/other/router/monitor.go | 9 ++++--- scripts/k8s/deploy.yml | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/app/other/router/monitor.go b/app/other/router/monitor.go index 5f36fa9f..e7737275 100644 --- a/app/other/router/monitor.go +++ b/app/other/router/monitor.go @@ -16,9 +16,12 @@ func init() { routerNoCheckRole = append(routerNoCheckRole, RegisterMonitorRouter) } -// readyTimeout bounds the whole probe. It has to stay under whatever period -// the orchestrator polls on, or a slow dependency turns a readiness check into -// a queue of readiness checks. +// readyTimeout bounds the whole probe. What constrains it is the orchestrator's +// per-check timeout rather than its polling period: Kubernetes allows a probe +// one second by default, so a dependency that answers in 1.2s is recorded as a +// failed check however promptly this handler returns. A manifest that mounts +// this probe has to raise timeoutSeconds above this value, and +// scripts/k8s/deploy.yml does. const readyTimeout = 2 * time.Second // HealthPath and ReadyPath are the two probe routes, relative to APIPrefix. diff --git a/scripts/k8s/deploy.yml b/scripts/k8s/deploy.yml index 44792278..cac85e8e 100644 --- a/scripts/k8s/deploy.yml +++ b/scripts/k8s/deploy.yml @@ -22,6 +22,11 @@ metadata: app: go-admin version: v1 spec: + # One replica, and the drain window below buys nothing at one replica: there + # is nowhere to send the traffic this pod stops taking. Raising it needs one + # more change than the number - the volume below is shared by every replica, + # and the log path in settings.yml lives on it, so a second pod would append + # to the same rotating file. replicas: 1 selector: matchLabels: @@ -39,6 +44,40 @@ spec: imagePullPolicy: IfNotPresent ports: - containerPort: 8000 + # Readiness answers "send me requests". It fails while the database or + # the cache is unreachable, so this pod stays out of the Service until + # the datastore settings.yml names is really there - which is a change + # from having no probe at all, where a pod with an unreachable database + # was still sent traffic. + # + # timeoutSeconds is 3 rather than the default 1 because the handler + # allows its checks 2 seconds (readyTimeout in + # app/other/router/monitor.go). At the default, a database that answers + # in 1.2s is recorded as a failed check while the handler is returning + # 200. + readinessProbe: + httpGet: + path: /api/v1/ready + port: 8000 + periodSeconds: 5 + timeoutSeconds: 3 + failureThreshold: 3 + # Liveness answers "restart me", which is a different question: a + # process whose database is unreachable does not want restarting, so + # this points at /health, which is a bare 200. Both probes skip the + # rate limiter - see exemptProbes in cmd/api/server.go - because a + # liveness probe that collects 429s under load gets the container + # restarted at the moment the deployment can least afford to lose it. + # + # initialDelaySeconds covers the migrations, which run before the + # listener opens. + livenessProbe: + httpGet: + path: /api/v1/health + port: 8000 + initialDelaySeconds: 15 + periodSeconds: 10 + failureThreshold: 3 volumeMounts: - name: go-admin mountPath: /temp @@ -47,6 +86,18 @@ spec: - name: go-admin-config mountPath: /config/ readOnly: true + # SIGKILL arrives when this is up, so it has to be longer than what the + # process spends shutting down: extend.shutdown's drain + server + + # cleanup, which settings.yml ships as 0 + 5 + 3. Raise drain here and + # this number has to follow, or the cleanup callbacks are cut off + # part-way through - checksilent's shutdown-budget-overruns-grace check + # is what notices. + # + # No preStop hook on purpose. A sleep there would be spent before the + # process is told anything, so BeginDraining never runs and /ready + # answers 200 for the whole of it - and it would be added to the budget + # above rather than replacing any of it. + terminationGracePeriodSeconds: 30 volumes: - name: go-admin persistentVolumeClaim: