test: real-Ceph quorum-survival e2e harness (#70, #71) - #82
Open
senolcolak wants to merge 11 commits into
Open
senolcolak wants to merge 11 commits into
senolcolak wants to merge 11 commits into
Conversation
Separate pure unit tests from envtest integration tests and remove the non-hermetic, source-mutating test workflow. - Gate controller envtest suites behind the `envtest` build tag; the unit layer (webhook defaulter/validator specs, fixture builders) now runs with plain `go test` and no Kubernetes API server or KUBEBUILDER_ASSETS. - Convert the webhook suite to pure unit tests: the specs call Default/Validate directly, so the envtest + webhook-server bootstrap that nothing exercised is removed in favor of a minimal RunSpecs entrypoint with RandomizeAllSpecs enabled. - Replace the captured mon-deployment.yaml (+ secret/configmap) fixtures with typed Go builders in test/builders, reproducing only the fields the controller consumes, with functional options and a contract test. - Add make targets that do not mutate source or clone Rook over the network: test-unit (race, shuffle), test-unit-repeat (20x for order dependence), test-envtest (explicit assets, envtest tag), test-all; redefine `test`. RandomizeAllSpecs is left off the controller suite pending #78 (a pre-existing finalizer-cleanup order dependence surfaced by this work). Package-level -shuffle=on is kept. Refs #67
… the strip path The mon fixture omitted --mon-host and --mon-initial-members, so the controller's modifyContainers strip branches for those args were never exercised by the envtest suite. The source mons this builder replaces carry both. Add them to the fixture. docs: reconcile the testing-foundation design doc with what shipped — mark #76 as planned (PR 4, not delivered), and correct the #66 job list to the jobs that actually exist in ci.yaml (generate/verify/envtest/helm/vuln + REUSE from reuse.yaml) instead of the aspirational names (crd-validate, race, etc.).
…ontract test Address PR #79 review: the chown initContainer had no Args, so the controller's modifyContainers rewrite of a /var/lib/ceph/mon/ceph- path arg was never exercised for that container. Restore the real Rook chown args (chown -R ceph:ceph over the ceph dirs, including the mon data dir) and add a contract-test assertion that the initContainer carries a rewritable mon path. Also correct the Makefile 'test' comment: the suite does not clone Rook, but the envtest CRDs come from contrib/k8s/3rdparty/ (populated by 'make deps', a one-time network clone) and 'make env' downloads kube binaries — so it is reproducible, not fully network-free.
Add the mandatory pull-request CI baseline before the expensive system-test
work in later issues.
- .github/workflows/ci.yaml: fast required checks, all SHA-pinned, least
privilege (contents: read), concurrency-cancel on superseded runs, and a
15-minute per-job timeout:
- generated files up to date (make gen + make imports must be a no-op)
- fmt / vet / lint / unit (-race -shuffle=on) / build
- envtest integration (against the pinned Rook CRD, no network)
- helm lint + template
- govulncheck
Go version comes from go.mod, envtest/K8s version from the Makefile, so CI
uses the versions the repository declares. All Go tooling runs via `go tool`.
- .github/workflows/nightly.yaml: the 20x determinism repeat, deferred off the
required gate to keep fast CI under target.
- Vendor the pinned Rook CRD (contrib/k8s/3rdparty/rook.yaml, Rook v1.18.6,
verified byte-for-byte against upstream) so CRD/envtest jobs need no network;
un-ignore just that file and add its Apache-2.0 REUSE annotation.
- docs/ci/required-checks.md: the checks branch protection must require and the
recommended main-branch settings (supports #77).
Builds on #67 (the hermetic make targets this workflow calls).
The helm job relied on the runner's preinstalled, unpinned Helm. Add azure/setup-helm (SHA-pinned) with a pinned Helm version so the check is reproducible. Correct the 'no network / hermetic' claims in ci.yaml and required-checks.md: the envtest job downloads kube-apiserver/etcd via setup-envtest from a Google-hosted bucket on a cache miss, and govulncheck queries vuln.go.dev. CI is reproducible (pinned versions), not fully network-free; say so. Also note the REUSE check comes from reuse.yaml, not ci.yaml.
Adds fast pure-unit coverage for the safety-critical reconciler branches that were previously reachable only through the slow envtest suite, fixes the teardown race that forced RandomizeAllSpecs off (#78), and corrects three webhook validation bugs surfaced by the new tests. Coverage: - Extract pure helpers allocateMonID and determinePublicAddressFor from the reconciler methods and unit-test every branch (mon-ID collision/exhaustion; all Service types + unallocated-IP/IPv6-only/no-ingress/unknown-type errors). Behavior-preserving: methods now delegate to these free functions. - docs/testing/traceability.md maps each reconcile transition/error branch to its covering spec, marking honest gaps for the deferred deletion-cascade and observedGeneration work. - Makefile: run ./pkg/controller/... in the unit layer (untagged pure tests), and add a report-only test-cover target that merges the unit and envtest coverage profiles (COVER_MIN gates once a floor is measured). - namespaceCleanUp force-strips finalizers tolerating both NotFound and Conflict and re-lists fresh each pass, converging inside a single Eventually that requires source and target namespaces empty in the same pass (source first, so the deleted RemoteArbiter stops repopulating the target). - Re-enable RandomizeAllSpecs. The "should succeed" spec waits 2 min: the reconciler has no watch on the target Deployment and only re-observes via the 1-min CheckInterval requeue, so the shared 30s timeout was intrinsically racy. Webhook fixes (regression-guarded): - Two field.Invalid calls reported the wrong field's value as BadValue (cephCluster.namespace reported .Name; monIdPrefix reported remoteCluster.Name). - The NodePort nodeIp branch reported Service.Type as BadValue and, on an unparseable IP, fell through to the Is4 check and appended a second spurious error. Fixed to one error naming the NodeIP. Closes #68 Refs #78
…ion works createArbiterService never set Spec.Type on the created Service, so a spec requesting service.type=NodePort (or LoadBalancer) silently got a ClusterIP Service. determinePublicAddressFor then picked the ClusterIP branch and baked an unroutable in-cluster address into the monitor's --public-addr and the monmap, with no error and no condition set. Set the created Service's type from spec.service.type, and make the NodePort branch of determinePublicAddressFor error on an empty node IP (mirroring the ClusterIP/LoadBalancer unallocated-address branches) instead of returning an empty address. Add a pure-unit regression row proving the empty-NodeIP guard. Correct docs/testing/traceability.md: drop stale line anchors (function names only), promote error-branch rows that were mislabeled 'partial' to covered after verifying the specs assert the specific condition + Error state, and record the Service-type propagation coverage honestly.
One k3d cluster, two namespaces (rook-ceph source, external-arbiter target): one flat pod network so the arbiter mon and source mons route without cross-cluster L3. Real Rook Ceph, real arbiter ceph-mon joining real Paxos quorum, kill a source mon, assert quorum_status retains an ext- member. Records the honest footprint (~16GB RAM, ~12min) and the explicit non-goals: no cross-cluster networking (#69), no NodePort/LB nodeIp path, no data-path recovery. Design only; scripts follow a separate approval.
Brings up a throwaway k3d cluster with cert-manager, Rook Ceph v1.18.6 (mon.count=3, loopback-PVC OSDs), and the operator via helm, then deploys an arbiter mon into a second namespace and kills a source mon to assert quorum survives because the arbiter votes. - test/e2e/quorum/: numbered steps 00..80 + 99-teardown, run.sh, lib.sh - manifests/: source CephCluster, installer RBAC, RemoteCluster/RemoteArbiter - Makefile: test-e2e-quorum, test-e2e-quorum-teardown (not in `make test`) One cluster / two namespaces keeps mons on a flat routable network, proving quorum survival without cross-cluster L3 (#69). NEGATIVE_CONTROL=1 kills a second mon to show quorum can be lost, making the assertion non-vacuous.
Review cycle 1 fixes: - OSDs: k3d local-path can't do volumeMode:Block; add loopdev DaemonSet (15-osd-loopdev) feeding Rook useAllNodes/useAllDevices instead of a storageClassDeviceSets Block PVC that never provisions. - Arbiter Deployment check: match on ceph.cobaltcore.sap.com/lookup label, not app=rook-ceph-mon (which the arbiter is never labelled with). - CSI: disable ROOK_CSI/ROOK_USE_CSI_OPERATOR in the operator configmap so bring-up doesn't block on absent csi-operator Driver CRDs. - Survival assertion: record killed mon ids and assert each is absent from quorum at assertion time, closing a >=3 false-pass when Rook re-creates the victim. - ceph calls: bound with --connect-timeout + host timeout/gtimeout so a lost-quorum cluster fails fast; negative control treats that as PASS. - 70-kill-mon: portable read loop (macOS bash 3.2 has no mapfile). - 20-cephcluster: guard the mon-count retry loop against pipefail. - docs/README: loopdev OSD approach, arbiter-operator namespaces, and the negative control framed as required for the full proof.
Review cycle 2 fixes: - loopdev DaemonSet: busybox losetup has no -j/-f --show, so reuse detection was dead code and 'losetup -f' then attach raced across the 3 node pods (shared host /dev) → CrashLoop. Switch to rook/ceph (util-linux, already pulled) and attach atomically with 'losetup -f --show' + EBUSY retry. - negative control: a bare non-zero ceph exit no longer counts as 'quorum lost' (toolbox-unready/kubectl blips would false-pass the vacuity guard); require the toolbox reachable first and drop HEALTH_ERR from the loss pattern (HEALTH_ERR != quorum loss). - assertions: baseline now requires exactly 4 mons and survival exactly 3 (was >=), so an extra or Rook-recreated mon fails loudly instead of masking the property. - run.sh: preflight kubectl >= 1.23 for 'wait --for=jsonpath'. - docs: correct the stale CephCluster YAML (useAllNodes/useAllDevices, ceph:v19), CR namespaces (arbiter-operator), and Predicate 2 snippet to match the shipped scripts; note KEEP_CLUSTER re-run non-idempotency.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The negative-control quorum-loss assertion can still false-pass on unrelated ceph -s failures and the harness preflight/docs need small consistency fixes to prevent avoidable confusion and incorrect outcomes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a real-Ceph quorum-survival end-to-end harness under test/e2e/quorum/ to validate the operator’s core value proposition (arbiter mon is a voting quorum member and keeps quorum after a source mon loss), and also strengthens the repo’s testing/CI foundation by separating unit vs envtest layers, introducing typed fixture builders, adding targeted regression/unit tests for safety-critical logic, and wiring required CI workflows.
Changes:
- Add k3d + Rook + operator + arbiter “quorum-survival” e2e harness with fault injection and assertions (plus design/docs).
- Refactor tests into layered targets (
test-unit,test-envtest) with deterministic settings; replace captured YAML fixtures with typed Go builders. - Improve controller/webhook correctness and testability (extract pure functions, fix validation BadValue reporting, propagate Service type), and add CI + branch-protection documentation.
File summaries
| File | Description |
|---|---|
| test/e2e/quorum/run.sh | Top-level orchestrator for the quorum-survival e2e run. |
| test/e2e/quorum/README.md | Harness usage and step-by-step documentation. |
| test/e2e/quorum/lib.sh | Shared helpers/utilities for e2e scripts (tool checks, Ceph exec helpers). |
| test/e2e/quorum/00-k3d-up.sh | Creates/reuses the k3d cluster used by the harness. |
| test/e2e/quorum/10-rook-install.sh | Installs cert-manager + pinned Rook components and disables CSI. |
| test/e2e/quorum/15-osd-loopdev.sh | Attaches loopback block devices via DaemonSet for OSDs. |
| test/e2e/quorum/20-cephcluster.sh | Applies CephCluster and waits for readiness/mon count. |
| test/e2e/quorum/30-operator-deploy.sh | Builds/imports the operator image and installs it via Helm. |
| test/e2e/quorum/40-remote-kubeconfig.sh | Creates target RBAC and stores a kubeconfig Secret for “remote” access. |
| test/e2e/quorum/50-apply-arbiter.sh | Applies RemoteCluster/RemoteArbiter and waits for Ready + Deployment existence. |
| test/e2e/quorum/60-assert-baseline.sh | Asserts baseline quorum state (arbiter voting, 4 mons). |
| test/e2e/quorum/70-kill-mon.sh | Fault injection step: scale down one (or two) source mon Deployments. |
| test/e2e/quorum/80-assert-survival.sh | Asserts quorum survival (or loss for negative control) after kill. |
| test/e2e/quorum/99-teardown.sh | Harness teardown (k3d delete), gated by KEEP_CLUSTER. |
| test/e2e/quorum/manifests/source-cephcluster.yaml | CephCluster manifest tuned for the harness topology/resources. |
| test/e2e/quorum/manifests/remote-cluster.yaml | RemoteCluster manifest for the harness. |
| test/e2e/quorum/manifests/remote-arbiter.yaml | RemoteArbiter manifest for the harness (service type set explicitly). |
| test/e2e/quorum/manifests/osd-loopdev-daemonset.yaml | Privileged DS to create/attach per-node loop devices. |
| test/e2e/quorum/manifests/arbiter-installer-rbac.yaml | Target-namespace RBAC required by the operator’s remote client. |
| test/builders/monitor.go | New typed fixture builders replacing captured YAML for controller/envtest. |
| test/builders/monitor_test.go | Contract tests for builder fixtures to prevent silent drift. |
| pkg/controller/remotearbiter_controller.go | Extracts/test-enables core logic and fixes Service type propagation/public address selection. |
| pkg/controller/allocation_test.go | Unit tests for extracted mon-ID allocation + public address selection. |
| pkg/controller/suite_test.go | Envtest suite: build-tagging, determinism improvements, fixture builder adoption, cleanup hardening. |
| pkg/controller/remotecluster_controller_test.go | Teardown cleanup made Eventually-based to remove order dependence. |
| pkg/controller/remotearbiter_controller_test.go | Teardown cleanup hardening and flake fix by extending wait beyond requeue interval. |
| pkg/webhook/v1alpha1/suite_test.go | Converts webhook tests to pure unit suite (no envtest/APIServer). |
| pkg/webhook/v1alpha1/remotearbiter_webhook.go | Fixes misleading validation BadValue reporting and NodeIP error handling. |
| pkg/webhook/v1alpha1/remotearbiter_webhook_regression_test.go | Regression tests for correct BadValue and single-error behavior. |
| Makefile | Adds layered test targets, coverage merging target, and e2e harness entrypoints. |
| .github/workflows/ci.yaml | Adds required per-PR CI baseline jobs (gen/verify/envtest/helm/vuln). |
| .github/workflows/nightly.yaml | Adds nightly determinism repeat job for unit tests. |
| docs/ci/required-checks.md | Documents required checks and recommended branch protection settings. |
| docs/testing/traceability.md | Traceability table mapping reconcile branches to tests. |
| docs/superpowers/specs/2026-09-03-testing-foundation-design.md | Design doc capturing testing foundation scope/decisions. |
| docs/superpowers/specs/2026-09-03-quorum-survival-harness-design.md | Design doc for the quorum-survival harness and its limitations. |
| REUSE.toml | Adds licensing annotation for vendored Rook CRD manifest. |
| .gitignore | Ignores 3rdparty directory contents while retaining pinned rook.yaml. |
| contrib/k8s/test/mon-deployment.yaml | Removes captured YAML fixture (replaced by typed builders). |
| contrib/k8s/test/env-var-secret.yaml | Removes captured YAML fixture (replaced by typed builders). |
| contrib/k8s/test/keyring-secret.yaml | Removes captured YAML fixture (replaced by typed builders). |
| contrib/k8s/test/override-configmap.yaml | Removes captured YAML fixture (replaced by typed builders). |
Review details
- Files reviewed: 41/43 changed files
- Comments generated: 3
- 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 on lines
+21
to
+28
| if ! out="$(ceph_exec ceph -s 2>&1)"; then | ||
| log "PASS negative control: ceph could not reach quorum (timed out) — quorum lost" | ||
| exit 0 | ||
| fi | ||
| if echo "${out}" | grep -qiE 'no quorum|quorum .* down|mon .* down|[0-9]+/[0-9]+ mons down'; then | ||
| log "PASS negative control: ceph reports lost quorum" | ||
| exit 0 | ||
| fi |
Comment on lines
+17
to
+22
| # `kubectl wait --for=jsonpath=` (used by steps 20/50) needs kubectl >= 1.23. | ||
| # Fail early with a clear message rather than a cryptic mid-run flag error. | ||
| kube_minor="$(kubectl version --client -o json 2>/dev/null \ | ||
| | jq -r '.clientVersion.minor | gsub("[^0-9]";"")' 2>/dev/null || echo 0)" | ||
| [ "${kube_minor:-0}" -ge 23 ] 2>/dev/null \ | ||
| || die "kubectl >= 1.23 required for 'wait --for=jsonpath' (got minor '${kube_minor}')" |
| | `50-apply-arbiter` | `RemoteCluster` + `RemoteArbiter`, wait `state=Ready`, assert arbiter Deployment (lookup label) | | ||
| | `60-assert-baseline` | 4 mons in quorum, `ext-*` arbiter already voting | | ||
| | `70-kill-mon` | scale one source mon Deployment to 0, record its ceph mon id | | ||
| | `80-assert-survival` | victim absent from quorum, ≥3 mons, arbiter still voting | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a real-Ceph quorum-survival e2e harness that proves the operator's reason to exist: deploy an arbiter
ceph-mon, kill a source mon, and show quorum survives because the arbiter votes. Realises issues #70 (quorum) and #71 (fault injection). The branch also carries the testing-foundation and CI work it builds on (#66, #67, #68).The harness (
test/e2e/quorum/)One k3d cluster (1 server + 2 agents), three namespaces:
rook-ceph— source Rook Ceph v1.18.6 (mon.count=3, loop-backed OSDs)arbiter-operator— the operator, the CRs, and the kubeconfig Secretexternal-arbiter— where the arbiter mon Deployment lands (spec.namespace)With 3 source mons + arbiter = 4 mons, killing one leaves 3/4 = majority. The harness asserts the arbiter (
ext-*) is voting before the kill (baseline), the killed mon is absent and exactly 3 mons remain after (survival), and — viaNEGATIVE_CONTROL=1— that killing a second mon drops to 2/4 and loses quorum (proving the arbiter's vote is load-bearing, not the assertion being vacuous).Kept out of the fast envtest/unit lane; gate behind an explicit CI label. Design + honest limitations (does NOT prove cross-cluster L3 / #69):
docs/superpowers/specs/2026-09-03-quorum-survival-harness-design.md.What else is on the branch
fix(controller): propagate Service type so NodePort/LB address selection worksReview
Two full review→fix cycles were run over the harness (correctness, portability/robustness on macOS bash 3.2, and proof-validity lenses). Fixes: loop-device attach made atomic + race-safe on util-linux (
losetup -f --show+ retry), negative control no longer false-passes on a bare non-zero ceph exit, exact mon-count assertions (==4 / ==3) so a recreated/extra mon fails loudly, kubectl-version preflight, and doc/script consistency.Test plan
shellcheck -x test/e2e/quorum/*.sh— cleanbash -non every script — clean