Skip to content

fix(kubernetes): make MinIO report its health and fail loudly on bucket setup - #7272

Open
corneliusroemer-agent wants to merge 5 commits into
mainfrom
fix/minio-startup-probe-and-bucket-bootstrap
Open

fix(kubernetes): make MinIO report its health and fail loudly on bucket setup#7272
corneliusroemer-agent wants to merge 5 commits into
mainfrom
fix/minio-startup-probe-and-bucket-bootstrap

Conversation

@corneliusroemer-agent

@corneliusroemer-agent corneliusroemer-agent commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Why

In run 33881830926 28 integration tests failed in one chromium job because the MinIO bucket did not exist. The pod reported 1/1 Running with 0 restarts, and wait_for_pods_to_be_ready.py passed.

Two things combine to hide it. The bucket is created by a postStart hook that backgrounds itself with ( … ) &, so nothing can fail if it goes wrong and kubelet discards its output; and its entire readiness strategy is sleep 10, so if MinIO is not serving ten seconds after the container starts, mc alias set fails and mc mb never runs. That run was on a heavily loaded runner — postgres took 33.9 s to pull onto the same node. MinIO also declares no probes at all, so it is Ready the instant the process starts, which is why a bucketless MinIO sailed through the wait step.

What this changes

The hook now runs in the foreground under set -e, polling until MinIO answers instead of sleeping a fixed ten seconds, and gives up after 120 s. A failure now kills the container and restarts it, which is both self-healing and visible as a restart count. MinIO also gets a readiness probe on its health endpoint, so the pod stops claiming to be Ready before it is.

Things worth knowing

postStart blocking is deliberate and safe here: kubelet does not report the container as Running until the hook returns, but the MinIO process itself is already running and serving, so the hook's own poll is what waits. There is no deadlock, and the probes only start once the hook has proved the API answers.

This was a separate minio/mc sidecar container until #3978 replaced it with the backgrounded hook. I kept the hook rather than going back to a sidecar or moving to a post-install Job, because a Job brings Helm hook semantics and Argo CD sync-phase behaviour for what is a dev- and CI-only path (s3.enabled and runDevelopmentS3).

I used /minio/health/live, which is what MinIO's own chart does.

I deliberately did not add a liveness or startup probe. Readiness is the field wait_for_pods_to_be_ready.py actually reads. During postStart the status still says the container is starting, and liveness, readiness and startup probes are all inert.

That has a nice consequence for this PR: because the hook now runs in the foreground, the container cannot be reported Running — and therefore cannot be Ready — until the bucket exists. It also explains the original bug precisely. With ( … ) & the hook returned in milliseconds, so the container went Running immediately, and with no readiness probe a running container is Ready by definition. The pod was Ready while sleep 10; mc mb was still running detached, or had failed.

The image pin is part of the same fix

I originally left image: minio/minio:latest for a follow-up, and that was the wrong call once the hook fails fast: bucket problems now become container restarts, and with a latest tag Kubernetes defaults imagePullPolicy to Always, so every restart re-pulls from Docker Hub — on the loaded runner this PR is about.

Pinning is the whole fix, with no new field: Kubernetes defaults the pull policy to IfNotPresent for any tag that is not latest. And it is behaviour-neutral today — latest and RELEASE.2025-09-07T16-13-09Z are the same digest, sha256:14cea49..., which is also the digest CI has been running. MinIO's community image is not moving anyway, so a pin costs nothing in updates.

🚀 Preview: https://fix-minio-startup-probe-a.loculus.org

…et setup

The bucket is created by a postStart hook that backgrounds itself with `&`,
so nothing can observe whether it worked, and MinIO declares no probes, so
the pod is Ready the moment the container starts. A failed bucket creation
therefore leaves a Ready MinIO with no bucket and no trace anywhere.

Run the hook in the foreground with `set -e` and replace the fixed `sleep 10`
with a bounded retry, so a failure kills and restarts the container instead
of vanishing. Add liveness and readiness probes on MinIO's health endpoint.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WiCjBAPT6cG6qHr6r5dqzA
@claude claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 8, 2026
Readiness is what fixes the bug: it is the field
wait_for_pods_to_be_ready.py reads, and its absence is why a MinIO with no
bucket counted as ready.

Liveness is not worth its risk here. Whether kubelet begins probing before
the postStart hook returns is not something I could verify offline, and the
two probes fail very differently if it does: a readiness probe that cannot
reach MinIO only delays Ready, while a liveness probe kills the container
after 30s, on exactly the loaded nodes this change exists to survive.
Restarting a single-replica ephemeral standin also recovers nothing that
the postStart retry does not already handle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WiCjBAPT6cG6qHr6r5dqzA
@corneliusroemer-agent corneliusroemer-agent added the preview Triggers a deployment to argocd label Sep 8, 2026
Comment thread kubernetes/loculus/templates/minio-deployment.yaml Outdated
@corneliusroemer
corneliusroemer marked this pull request as ready for review September 8, 2026 20:38
Copilot AI lite review requested due to automatic review settings September 8, 2026 20:38
@claude

claude Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @corneliusroemer's task in 3m 48s —— View job


Review: fix(kubernetes) MinIO health + bucket bootstrap

  • Gather context (diff, service, values, wait_for_pods_to_be_ready.py)
  • Review the manifest change
  • Post inline findings

The diagnosis in the description holds up. I traced it: has_container_that_is_not_ready in .github/scripts/wait_for_pods_to_be_ready.py:46-51 reads only containerStatuses[].ready, and with no readiness probe a Running container is Ready by definition — so a bucketless MinIO passed the gate exactly as described. Moving the hook to the foreground is the right lever, because kubelet won't mark the container Running until it returns, which makes the bucket a hard precondition for Ready rather than a race.

The shell is correct. set -e is suppressed for the until condition, so a failing mc alias set retries rather than exiting, and attempt=$((attempt + 1)) always returns 0 — no accidental early exit. mc mb -p and mc anonymous set-json are idempotent, which is load-bearing now that restarts re-run the hook.

Three things, all inline:

  • Readiness probe can yank the Service endpoint (lines 71–77). The probe now gates loculus-minio-service endpoints, and MinIO requests only cpu: 10m. On the contended runner this PR targets, 30 s of missed probes turns a slow MinIO into connection-refused for the backend — a new failure mode. Suggested a higher failureThreshold.
  • Retry doesn't cover mc mb / mc anonymous (lines 95–96). A blip there costs a container restart plus, because of :latest, a Docker Hub re-pull. Cheap to fold into the same loop.
  • The 120 s bound is attempt-count, not wall-clock (lines 86–94). Low severity, but a hanging mc would leave the container in ContainerCreating with no restart, since kubelet enforces no postStart timeout — the one case this PR can't self-heal.

On the deferred latest pin: it's more load-bearing after this change than before, since fail-fast now converts bucket problems into restarts and each restart re-pulls. Fine to split out, worth doing soon.

No test changes needed — this is a dev/CI-only path (s3.enabled and runDevelopmentS3), and the integration suite is the test.

I could not run helm lint / helm template to confirm the render; both were blocked by sandbox permissions in this environment. The change is plain YAML under an existing containers[] entry, so I don't expect a render problem, but it's unverified.
· fix/minio-startup-probe-and-bucket-bootstrap

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The current hook is not idempotent on bucket creation (can restart-loop when the bucket already exists) and the readiness probe points at MinIO’s liveness endpoint rather than its readiness endpoint.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR improves the reliability and observability of the dev/CI MinIO deployment by ensuring the bucket setup is performed deterministically during container startup and by adding a Kubernetes readiness probe so the Pod doesn’t appear Ready prematurely.

Changes:

  • Added a MinIO readinessProbe so Kubernetes can gate readiness on MinIO’s health endpoint.
  • Reworked the postStart hook to run in the foreground with set -e, polling until MinIO responds (with a timeout) before creating/configuring the bucket.
File summaries
File Description
kubernetes/loculus/templates/minio-deployment.yaml Adds readiness probing and makes bucket creation/setup a blocking, fail-fast postStart step to prevent “Ready but unusable” MinIO pods
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread kubernetes/loculus/templates/minio-deployment.yaml
Comment thread kubernetes/loculus/templates/minio-deployment.yaml Outdated
Comment on lines +71 to +77
readinessProbe:
httpGet:
path: /minio/health/live
port: 9000
periodSeconds: 10
timeoutSeconds: 5
failureThreshold: 3

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The readiness probe now gates loculus-minio-service endpoints (minio-service.yaml:10-15 selects these same labels), so a probe flap doesn't just change what wait_for_pods_to_be_ready.py sees — it pulls MinIO out of the Service mid-test.

That matters because MinIO requests only cpu: 10m (values.yaml:3035-3040). On the same contended runners this PR is fixing for, a CPU-starved MinIO can miss a 5 s probe timeout; three misses (30 s) drop the endpoint and the backend gets connection refused instead of a slow response. That's a failure mode the old probe-less config didn't have.

Worth being more forgiving, since nothing here needs fast NotReady detection on a single dev replica:

Suggested change
readinessProbe:
httpGet:
path: /minio/health/live
port: 9000
periodSeconds: 10
timeoutSeconds: 5
failureThreshold: 3
readinessProbe:
httpGet:
path: /minio/health/live
port: 9000
periodSeconds: 10
timeoutSeconds: 5
failureThreshold: 6

Fix this →

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken, failureThreshold is now 6. Nothing here needs fast NotReady detection on a single dev replica, so the slower detection costs nothing and the flap risk you describe is real.

Comment thread kubernetes/loculus/templates/minio-deployment.yaml Outdated
Comment thread kubernetes/loculus/templates/minio-deployment.yaml
…etup

Pin the image to the release `latest` currently points at (same digest,
sha256:14cea49). That is worth more than reproducibility here: Kubernetes
defaults imagePullPolicy to Always for a `latest` tag and IfNotPresent for
any other, so pinning alone takes Docker Hub out of the container-restart
path, which this PR made the failure path.

Put the whole bootstrap inside the retry loop. Previously only the
connectivity check retried, so a transient `mc mb` or `mc anonymous` failure
exited under `set -e` and cost a full container restart. Both are idempotent,
so retrying them in place is free and the loop gets simpler rather than
more complex.

Spell out `--ignore-existing` instead of its short form `-p`; a reviewer read
`-p` as `--parents` and thought bucket creation was not idempotent.

Use `/minio/health/ready` rather than the liveness endpoint. In this
deployment the two are identical (readiness only additionally checks KMS and
etcd, neither configured), but the named-for-the-job endpoint is clearer.

Raise the readiness failureThreshold to 6. Nothing needs fast NotReady
detection on a single dev replica, and MinIO requests only 10m CPU, so on a
contended runner a missed probe should not pull it from the Service.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WiCjBAPT6cG6qHr6r5dqzA

@maverbiest maverbiest left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure preview Triggers a deployment to argocd

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants