Skip to content

fix(consensus): stop owning PVCs so data outlives the capsule - #1453

Merged
leninmehedy merged 2 commits into
mainfrom
fix/01449-consensus-pvc-no-owner
Oct 30, 2026
Merged

leninmehedy merged 2 commits into
mainfrom
fix/01449-consensus-pvc-no-owner

Conversation

@leninmehedy

Copy link
Copy Markdown
Member

Summary

  • No owner reference on PVCs. reconcilePVC no longer calls SetControllerReference, so deleting a ConsensusCapsule (or its Orbit) never garbage-collects consensus data.
  • Watch removed. Owns(PersistentVolumeClaim) is dropped, since it can't fire without an owner reference. PVC reconcile is create-only and PVCs are immutable, so nothing depended on it.
  • Reinstall reuses PVCs. Same namespace and node id picks up the existing claims, unchanged.
  • Docs. Added a "Data retention" note; data is removed only by an explicit kubectl delete pvc.

Notes

  • A deleted PVC no longer re-queues the capsule, matching the "remove data only on purpose" intent.
  • Pre-existing on main, not caused by this PR: TestRestart_ConsensusCapsule_PinsReachChildBuild fails, and task lint reports a typecheck error (LedgerId) in test/integration/restart/restart_test.go.
  • solo-weaver can drop its ReleaseConsensusPVCs step once operators ship this.

Test plan

  • go test ./internal/resources/consensus/ (asserts no owner refs; capsule delete keeps PVC; reinstall reuses it)
  • Integration tests in CI

Closes #1449

Drop the controller owner reference on consensus PVCs and the Owns(PVC) watch. Deleting a ConsensusCapsule no longer garbage-collects its PVCs; reinstall reuses them. Add tests and a data-retention note.

Closes #1449

Signed-off-by: Lenin Mehedy <lenin.mehedy@hashgraph.com>
@leninmehedy
leninmehedy requested review from a team as code owners October 29, 2026 23:38
@leninmehedy
leninmehedy requested review from alex-au and a balanced review from Copilot October 29, 2026 23:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

An unresolved active finding remains, flagging that the SurvivesCapsuleDelete test assertion fails to exercise its named behavior because the fake client skips owner-reference garbage collection, so it should instead assert the surviving PVC has no owner reference.

1 open issue
What changed in this PR

This PR implements issue #1449: it stops the consensus operator from owning its PersistentVolumeClaims so that consensus node data (state, streams, logs, etc.) survives deletion of a ConsensusCapsule or its parent Orbit, and is only ever removed by an explicit delete. Previously reconcilePVC set the capsule as the controller owner, which caused Kubernetes garbage collection to delete the node's data whenever the capsule was deleted — contradicting the product stance that this data is precious. The change also makes fresh and reinstalled nodes behave identically (reinstalled PVCs had already lost their owner reference, so GC behavior diverged).

The change is consistent with the surrounding code: PVCs are reconciled create-only (PVCs are immutable), the operator locates them by name/label (GetPVCNames/CommonLabels), the StatefulSet mounts them by ClaimName rather than via volumeClaimTemplates, and the removed Owns(PVC) watch was gated on GenerationChangedPredicate — which never fires for immutable PVCs — so dropping it has no functional effect.

Changes:

  • Remove SetControllerReference(capsule, ...) (and the now-unused controllerutil import) from reconcilePVC, leaving an explanatory comment.
  • Drop the Owns(&corev1.PersistentVolumeClaim{}, genChanged) watch from SetupWithManager.
  • Add tests asserting PVCs have no owner references and are reused on reinstall, plus a "Data retention" note in the example docs.
File Description
internal/​resources/​consensus/​pvc.go Removes owner-reference assignment and its import; data now outlives the capsule.
internal/​controller/​consensuscapsule_controller.go Drops the PVC Owns watch, which could no longer fire without an owner reference.
internal/​resources/​consensus/​consensus_test.go Adds owner-reference and survive/reuse assertions; the delete-survival assertion is weak under the fake client (see comment).
docs/​example/​README.md Documents that PVC data outlives the capsule and is removed only via explicit kubectl delete pvc.

I left one optional test-quality comment noting that the SurvivesCapsuleDelete assertion doesn't actually exercise garbage collection (the controller-runtime fake client doesn't GC owner references), so it would pass even if the owner reference were reintroduced. The actual guarantee is covered by the no-owner-reference assertion in TestPVCSubReconciler.

Review effort: Balanced

- restart integration test: use the spec.network shape from the Orbit refactor so the package compiles
- restart unit test: give the Orbit fixture ledgerId+chainId so reconcile passes the consensus enforcement
- assert the surviving PVC has no owner reference (fake client does not garbage-collect)
- bump github.com/klauspost/compress to v1.20.2 (zstd advisory)

Closes #1449

Signed-off-by: Lenin Mehedy <lenin.mehedy@hashgraph.com>

@brunodam brunodam 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.

LGTM

@leninmehedy
leninmehedy merged commit ea594f4 into main Oct 30, 2026
18 checks passed
@leninmehedy
leninmehedy deleted the fix/01449-consensus-pvc-no-owner branch October 30, 2026 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

consensus: stop the operator owning PVCs so data outlives the capsule

4 participants