diff --git a/charts/etcd-operator/files/manager-role-rules.yaml b/charts/etcd-operator/files/manager-role-rules.yaml index 4ad4b945..11c60323 100644 --- a/charts/etcd-operator/files/manager-role-rules.yaml +++ b/charts/etcd-operator/files/manager-role-rules.yaml @@ -74,6 +74,8 @@ verbs: - get - list + - patch + - update - watch - apiGroups: - etcd-operator.cozystack.io diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 32ab7632..4d481869 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -75,9 +75,10 @@ type EtcdClusterReconciler struct { ClusterDomain string } -//+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdclusters,verbs=get;list;watch +//+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdclusters,verbs=get;list;watch;update;patch //+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdclusters/status,verbs=get;update;patch //+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdclusters/finalizers,verbs=update +//+kubebuilder:rbac:groups="",resources=persistentvolumeclaims,verbs=get;list;watch;delete //+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdmembers,verbs=get;list;watch;create;delete //+kubebuilder:rbac:groups=etcd-operator.cozystack.io,resources=etcdmembers/status,verbs=get;update;patch //+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch @@ -102,11 +103,23 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, err } - // If the cluster is being deleted, don't keep reconciling it. Owned - // resources are cascaded out via owner refs; recreating a Service for a - // Terminating cluster races against the GC and pollutes logs. + // If the cluster is being deleted, stop reconciling it and run cleanup. + // Owner-referenced resources cascade out on their own; data volumes do + // not (they carry no owner reference — see deleteMemberPVC) and are + // reclaimed here, before the finalizer is released. if !cluster.DeletionTimestamp.IsZero() { - return ctrl.Result{}, nil + return r.handleClusterDeletion(ctx, cluster) + } + + // Claim the cleanup finalizer before anything else creates state worth + // cleaning up. + if controllerutil.AddFinalizer(cluster, ClusterFinalizer) { + if err := r.Update(ctx, cluster); err != nil { + if errors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, err + } } // Terminal-config gate. spec.tls.{client,peer}.certManager requires @@ -1309,6 +1322,19 @@ func (r *EtcdClusterReconciler) scaleDown( if err := r.Delete(ctx, &victim); err != nil && !errors.IsNotFound(err) { return ctrl.Result{}, err } + // One of the two places the operator reclaims a data volume. Deleting + // the member above runs MemberRemove through its finalizer, so by the + // time this volume is dropped its contents are already replicated on + // the remaining peers — this is the deliberate shrink the user asked + // for, not an accident, so the storage goes back to the pool. + // + // Note this is skipped for the 1→0 pause handled above: that member is + // parked dormant, CR and volume intact, and resume mounts it again. + if victim.Spec.Storage.Medium != lll.StorageMediumMemory { + if err := deleteMemberPVC(ctx, r.Client, victim.Namespace, victim.Name); err != nil { + return ctrl.Result{}, err + } + } return ctrl.Result{RequeueAfter: 10 * time.Second}, nil } @@ -1951,6 +1977,74 @@ func (r *EtcdClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { // that happens we sit in DeadlineExceeded. // // We never auto-pivot during bootstrap, and never silently in steady state. +// handleClusterDeletion reclaims what k8s garbage collection will not, then +// releases the finalizer. +// +// Members, Pods and Services are owner-referenced to the cluster and cascade +// out on their own. Data volumes deliberately are not (see deleteMemberPVC): +// they must survive a member disappearing for reasons that say nothing about +// the data's value. Deleting the EtcdCluster is not one of those reasons — it +// is the user declaring the cluster finished — so this is where the volumes +// are reclaimed, explicitly. +// +// The finalizer is held until they are actually gone rather than merely asked +// to go. A volume stays Terminating while a Pod still mounts it, so releasing +// early would let the EtcdCluster vanish with storage still allocated behind +// it, and a namespace delete would look complete while PVCs were still +// draining. Holding the finalizer makes "the EtcdCluster is gone" mean "its +// storage is gone too". +func (r *EtcdClusterReconciler) handleClusterDeletion( + ctx context.Context, + cluster *lll.EtcdCluster, +) (ctrl.Result, error) { + log := log.FromContext(ctx) + + if !controllerutil.ContainsFinalizer(cluster, ClusterFinalizer) { + return ctrl.Result{}, nil + } + + pvcs := &corev1.PersistentVolumeClaimList{} + if err := r.List(ctx, pvcs, + client.InNamespace(cluster.Namespace), + client.MatchingLabels{LabelCluster: cluster.Name}, + ); err != nil { + return ctrl.Result{}, fmt.Errorf("list data volumes of deleted cluster: %w", err) + } + + remaining := 0 + for i := range pvcs.Items { + pvc := &pvcs.Items[i] + if !pvc.DeletionTimestamp.IsZero() { + // Already draining — most likely still mounted by a Pod that is + // itself being GC'd. Wait it out rather than declaring the + // cluster gone while its storage is not. + remaining++ + continue + } + if err := r.Delete(ctx, pvc); err != nil { + if errors.IsNotFound(err) { + continue + } + return ctrl.Result{}, fmt.Errorf("delete data volume %q: %w", pvc.Name, err) + } + log.Info("reclaiming data volume of deleted cluster", "pvc", pvc.Name) + remaining++ + } + + if remaining > 0 { + return ctrl.Result{RequeueAfter: 5 * time.Second}, nil + } + + controllerutil.RemoveFinalizer(cluster, ClusterFinalizer) + if err := r.Update(ctx, cluster); err != nil { + if errors.IsConflict(err) { + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, err + } + return ctrl.Result{}, nil +} + func (r *EtcdClusterReconciler) handleDeadlineExceeded( ctx context.Context, cluster *lll.EtcdCluster, diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index dcf8bc38..7fe916cc 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -36,6 +36,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" lll "github.com/cozystack/etcd-operator/api/v1alpha2" ) @@ -961,6 +962,240 @@ func TestScaleUp_WaitsForInFlightDeletion(t *testing.T) { } } +// scaleDownFixture builds an n-member cluster ready to shrink by one, with a +// data volume per member. +func scaleDownFixture(t *testing.T, replicas int32, members int) (*lll.EtcdCluster, []client.Object) { + t.Helper() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: lll.EtcdClusterSpec{ + Replicas: ptrInt32(replicas), Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + Status: lll.EtcdClusterStatus{ + ClusterToken: "ns-test-x", ClusterID: "deadbeef", + Observed: &lll.ObservedClusterSpec{ + Replicas: replicas, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, + }, + } + objs := []client.Object{cluster} + now := metav1.Now() + for i := 0; i < members; i++ { + name := fmt.Sprintf("test-%d", i) + objs = append(objs, &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, Namespace: "ns", Labels: memberLabels("test", name), + CreationTimestamp: metav1.NewTime(now.Add(time.Duration(i) * time.Minute)), + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", InitialCluster: "x", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + Status: lll.EtcdMemberStatus{ + PodName: name, MemberID: fmt.Sprintf("id-%d", i), PVCName: memberPVCName(name), + Conditions: []metav1.Condition{{ + Type: lll.MemberReady, Status: metav1.ConditionTrue, Reason: "PodReady", LastTransitionTime: now, + }}, + }, + }) + objs = append(objs, &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: memberPVCName(name), Namespace: "ns", Labels: memberLabels("test", name), + }, + }) + } + return cluster, objs +} + +// TestClusterDeletion_ReclaimsVolumesBeforeReleasingFinalizer: deleting the +// EtcdCluster is the user declaring the cluster finished, so its storage goes +// with it. Volumes carry no owner reference (that is what keeps a member +// disappearing from destroying data), so nothing reclaims them implicitly — +// the finalizer does, explicitly, and is held until they are actually gone. +func TestClusterDeletion_ReclaimsVolumesBeforeReleasingFinalizer(t *testing.T) { + ctx := context.Background() + now := metav1.Now() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "ns", + DeletionTimestamp: &now, + Finalizers: []string{ClusterFinalizer}, + }, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(3)}, + } + objs := []client.Object{cluster} + for _, name := range []string{"test-a", "test-b"} { + objs = append(objs, &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: memberPVCName(name), Namespace: "ns", Labels: memberLabels("test", name), + }, + }) + } + c, _ := newTestClient(t, objs...) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} + + // First pass: volumes are asked to go, finalizer still held. + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.RequeueAfter == 0 { + t.Fatalf("expected a requeue while volumes drain; got %+v", res) + } + for _, name := range []string{"test-a", "test-b"} { + if err := c.Get(ctx, types.NamespacedName{Name: memberPVCName(name), Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); !apierrors.IsNotFound(err) { + t.Fatalf("volume of %s should have been reclaimed; got %v", name, err) + } + } + + // Second pass: nothing left, so the cluster is allowed to go. + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile (second pass): %v", err) + } + got := &lll.EtcdCluster{} + if err := c.Get(ctx, types.NamespacedName{Name: "test", Namespace: "ns"}, got); err == nil { + if controllerutil.ContainsFinalizer(got, ClusterFinalizer) { + t.Fatalf("finalizer must be released once the volumes are gone") + } + } else if !apierrors.IsNotFound(err) { + t.Fatalf("Get cluster: %v", err) + } +} + +// TestClusterDeletion_HoldsFinalizerWhileAVolumeDrains: a volume stays +// Terminating while a Pod still mounts it. Releasing the finalizer then would +// let the EtcdCluster vanish with storage still allocated behind it — and a +// namespace delete would look complete while PVCs were still draining. +func TestClusterDeletion_HoldsFinalizerWhileAVolumeDrains(t *testing.T) { + ctx := context.Background() + now := metav1.Now() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "ns", + DeletionTimestamp: &now, + Finalizers: []string{ClusterFinalizer}, + }, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(1)}, + } + draining := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: memberPVCName("test-a"), Namespace: "ns", Labels: memberLabels("test", "test-a"), + DeletionTimestamp: &now, + Finalizers: []string{"kubernetes.io/pvc-protection"}, + }, + } + c, _ := newTestClient(t, cluster, draining) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} + + res, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}) + if err != nil { + t.Fatalf("Reconcile: %v", err) + } + if res.RequeueAfter == 0 { + t.Fatalf("expected a requeue while the volume drains; got %+v", res) + } + got := mustGet(t, c, "test", "ns", &lll.EtcdCluster{}) + if !controllerutil.ContainsFinalizer(got, ClusterFinalizer) { + t.Fatalf("finalizer must be held until the volume is actually gone") + } +} + +// TestClusterDeletion_LeavesAnotherClustersVolumes: the reclaim is scoped by +// the cluster label. Two clusters in one namespace is ordinary, and deleting +// one must not touch the other's data. +func TestClusterDeletion_LeavesAnotherClustersVolumes(t *testing.T) { + ctx := context.Background() + now := metav1.Now() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "ns", + DeletionTimestamp: &now, + Finalizers: []string{ClusterFinalizer}, + }, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(1)}, + } + mine := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: memberPVCName("test-a"), Namespace: "ns", Labels: memberLabels("test", "test-a"), + }, + } + theirs := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: memberPVCName("other-a"), Namespace: "ns", Labels: memberLabels("other", "other-a"), + }, + } + c, _ := newTestClient(t, cluster, mine, theirs) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if err := c.Get(ctx, types.NamespacedName{Name: memberPVCName("other-a"), Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); err != nil { + t.Fatalf("another cluster's volume must be untouched; got %v", err) + } +} + +// TestScaleDown_ReclaimsTheVictimsVolume: shrinking a cluster is a deliberate +// instruction, and the departing member's contents are already replicated on +// the peers that remain (its finalizer runs MemberRemove first). Leaving the +// volume behind would quietly accumulate storage nobody asked to keep, so this +// is one of the two paths that reclaims it — explicitly, not by cascade. +func TestScaleDown_ReclaimsTheVictimsVolume(t *testing.T) { + ctx := context.Background() + _, objs := scaleDownFixture(t, 2, 3) + c, _ := newTestClient(t, objs...) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(fe)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + // Victim selection is newest-first, so test-2 goes. + if err := c.Get(ctx, types.NamespacedName{Name: memberPVCName("test-2"), Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); !apierrors.IsNotFound(err) { + t.Fatalf("victim's volume should have been reclaimed; got %v", err) + } + // The survivors keep theirs. + for _, name := range []string{"test-0", "test-1"} { + if err := c.Get(ctx, types.NamespacedName{Name: memberPVCName(name), Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); err != nil { + t.Fatalf("surviving member %s lost its volume: %v", name, err) + } + } +} + +// TestScaleDown_PauseKeepsTheVolume: scaling to zero parks the last member +// dormant rather than deleting it — the whole point being that the data +// survives until the cluster is resumed. The reclaim path must not fire here. +func TestScaleDown_PauseKeepsTheVolume(t *testing.T) { + ctx := context.Background() + _, objs := scaleDownFixture(t, 0, 1) + c, _ := newTestClient(t, objs...) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(fe)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } + + if err := c.Get(ctx, types.NamespacedName{Name: memberPVCName("test-0"), Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); err != nil { + t.Fatalf("paused cluster must keep its data volume; got %v", err) + } + member := &lll.EtcdMember{} + if err := c.Get(ctx, types.NamespacedName{Name: "test-0", Namespace: "ns"}, member); err != nil { + t.Fatalf("Get member: %v", err) + } + if !member.Spec.Dormant { + t.Fatalf("last member should have been parked dormant, not deleted") + } +} + // TestScaleDown_PicksMostRecentlyCreatedVictim: with apiserver-assigned // names there is no ordinal to scale down by; victim selection sorts by // CreationTimestamp (newest first). This naturally retires the most @@ -1727,7 +1962,9 @@ func TestDeadlineExceeded_NoChurnWhenSteady(t *testing.T) { now := metav1.Now() past := metav1.NewTime(now.Add(-time.Hour)) cluster := &lll.EtcdCluster{ - ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + // A settled cluster already carries the cleanup finalizer; it is + // claimed once on the first reconcile, not on every pass. + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns", Finalizers: []string{ClusterFinalizer}}, Spec: lll.EtcdClusterSpec{ Replicas: ptrInt32(3), Version: "3.5.17", @@ -2374,7 +2611,9 @@ func TestClusterUpdateStatus_NoChurnInSteadyState(t *testing.T) { ctx := context.Background() now := metav1.Now() cluster := &lll.EtcdCluster{ - ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + // A settled cluster already carries the cleanup finalizer; it is + // claimed once on the first reconcile, not on every pass. + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns", Finalizers: []string{ClusterFinalizer}}, Spec: lll.EtcdClusterSpec{ Replicas: ptrInt32(3), Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, }, diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index c87c69ab..7ebb0498 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -116,6 +116,12 @@ func (r *EtcdMemberReconciler) Reconcile(ctx context.Context, req ctrl.Request) log.Error(err, "failed to delete pod for dormant member") return ctrl.Result{}, err } + // The volume is intact but nothing mounts it while paused. Marking + // it detached is what makes "paused cluster" distinguishable from + // "leftover volume" in a plain `kubectl get pvc -o custom-columns`. + if err := r.setPVCStatus(ctx, member, PVCStatusDetached); err != nil { + log.Error(err, "failed to mark PVC detached") + } // Persist the cleared PodName + a Paused condition. updateStatus() // is for the running flow (reads pod, derives Ready); for dormant // we know the answer directly. Idempotent — setMemberCondition @@ -357,19 +363,24 @@ func (r *EtcdMemberReconciler) ensurePVC(ctx context.Context, member *lll.EtcdMe return nil } - pvcName := "data-" + member.Name + pvcName := memberPVCName(member.Name) pvc := &corev1.PersistentVolumeClaim{} err := r.Get(ctx, types.NamespacedName{Namespace: member.Namespace, Name: pvcName}, pvc) if err == nil { - // The PVC stays owned by this EtcdMember across pause/resume - // (the cluster controller flips Spec.Dormant rather than - // deleting the member CR), so ownership never moves. If the - // PVC's controller-owner doesn't match this member's UID, it - // belongs to something else and we must refuse — silently - // inheriting another member's data dir would crashloop the - // pod when etcd notices a removed memberID in the WAL. - if !pvcOwnedBy(pvc, member) { - return fmt.Errorf("PVC %q is owned by a different EtcdMember; awaiting GC before reuse", pvcName) + // The volume stays with this EtcdMember across pause/resume (the + // cluster controller flips Spec.Dormant rather than deleting the + // member CR), so the marker never moves. A volume carrying a + // different member's marker belongs to something else and must be + // refused — silently inheriting another member's data dir would + // crash-loop the pod when etcd notices a removed memberID in the WAL. + if !pvcBelongsTo(pvc, member) { + return fmt.Errorf("PVC %q belongs to a different EtcdMember; awaiting cleanup before reuse", pvcName) + } + // Migrate volumes that predate the annotation: stamp the marker and + // drop the controller owner reference, which is what made a deleted + // member take its data with it. + if err := r.adoptLegacyPVC(ctx, pvc, member); err != nil { + return err } member.Status.PVCName = pvcName return nil @@ -378,11 +389,16 @@ func (r *EtcdMemberReconciler) ensurePVC(ctx context.Context, member *lll.EtcdMe return err } - // The PVC is operator-created and operator-owned, so it carries the - // cluster's additionalMetadata like every other child object (backup - // tooling and cost-allocation selectors target PVCs specifically). + // The PVC is operator-created, so it carries the cluster's + // additionalMetadata like every other child object (backup tooling and + // cost-allocation selectors target PVCs specifically). pvcLabels, pvcAnnotations := applyAdditionalMetadata( memberLabels(member.Spec.ClusterName, member.Name), nil, member.Spec.AdditionalMetadata) + if pvcAnnotations == nil { + pvcAnnotations = map[string]string{} + } + pvcAnnotations[AnnPVCMemberUID] = string(member.UID) + pvcAnnotations[AnnPVCStatus] = PVCStatusInitializing pvc = &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: pvcName, @@ -400,9 +416,9 @@ func (r *EtcdMemberReconciler) ensurePVC(ctx context.Context, member *lll.EtcdMe }, }, } - if err := controllerutil.SetControllerReference(member, pvc, r.Scheme); err != nil { - return err - } + // No controller owner reference on purpose: the volume must outlive + // anything that removes its EtcdMember without meaning to discard the + // data. See AnnPVCMemberUID and deleteMemberPVC. if err := r.Create(ctx, pvc); err != nil { return err } @@ -410,19 +426,69 @@ func (r *EtcdMemberReconciler) ensurePVC(ctx context.Context, member *lll.EtcdMe return nil } -// pvcOwnedBy returns true only if the PVC carries an EtcdMember owner -// reference whose UID matches this member — i.e. we created it. PVCs with -// no owner refs, or with owner refs pointing at anything else, are refused. -func pvcOwnedBy(pvc *corev1.PersistentVolumeClaim, member *lll.EtcdMember) bool { +// adoptLegacyPVC brings a pre-existing volume onto the annotation-based +// ownership model: stamp the member-UID marker, and strip the controller +// owner reference so the volume no longer cascade-deletes with its member. +// +// Runs against volumes created by an older operator build and against those +// stamped by the in-place migration tool. Idempotent, and a no-op once a +// volume has been migrated. +func (r *EtcdMemberReconciler) adoptLegacyPVC( + ctx context.Context, + pvc *corev1.PersistentVolumeClaim, + member *lll.EtcdMember, +) error { + kept := make([]metav1.OwnerReference, 0, len(pvc.OwnerReferences)) for _, o := range pvc.OwnerReferences { if o.Kind == "EtcdMember" && o.UID == member.UID { - return true + continue } + kept = append(kept, o) } - return false + _, marked := pvc.Annotations[AnnPVCMemberUID] + if marked && len(kept) == len(pvc.OwnerReferences) { + return nil + } + + original := pvc.DeepCopy() + if pvc.Annotations == nil { + pvc.Annotations = map[string]string{} + } + pvc.Annotations[AnnPVCMemberUID] = string(member.UID) + pvc.OwnerReferences = kept + if err := r.Patch(ctx, pvc, client.MergeFrom(original)); err != nil { + return fmt.Errorf("migrate PVC %q to annotation ownership: %w", pvc.Name, err) + } + return nil } -// podOwnedBy mirrors pvcOwnedBy: true only when the Pod's owner refs +// setPVCStatus records the volume's lifecycle marker (AnnPVCStatus). Purely +// observational — see the constant's doc comment. Best-effort: a failure here +// must never block the member's reconcile, so callers log and continue. +func (r *EtcdMemberReconciler) setPVCStatus(ctx context.Context, member *lll.EtcdMember, status string) error { + if member.Spec.Storage.Medium == lll.StorageMediumMemory { + return nil + } + pvc := &corev1.PersistentVolumeClaim{} + name := types.NamespacedName{Namespace: member.Namespace, Name: memberPVCName(member.Name)} + if err := r.Get(ctx, name, pvc); err != nil { + if errors.IsNotFound(err) { + return nil + } + return err + } + if pvc.Annotations[AnnPVCStatus] == status { + return nil + } + original := pvc.DeepCopy() + if pvc.Annotations == nil { + pvc.Annotations = map[string]string{} + } + pvc.Annotations[AnnPVCStatus] = status + return r.Patch(ctx, pvc, client.MergeFrom(original)) +} + +// podOwnedBy: true only when the Pod's owner refs // point at this EtcdMember by UID. Less load-bearing than the PVC // check (Pod corruption is recoverable; replacing one is cheap), but // adopting a leftover Pod from a prior cluster generation would leave @@ -1064,6 +1130,14 @@ func (r *EtcdMemberReconciler) updateStatus(ctx context.Context, member *lll.Etc if err := r.Delete(ctx, member); err != nil { return ctrl.Result{}, err } + // One of the two places the operator reclaims a data volume: + // this member's data dir is broken enough to crash-loop it, and + // the quorum gate above proved the rest of the cluster is + // healthy, so its contents are both unusable and redundant. The + // replacement joins with an empty volume and syncs from peers. + if err := deleteMemberPVC(ctx, r.Client, member.Namespace, member.Name); err != nil { + return ctrl.Result{}, err + } return ctrl.Result{}, nil } if setMemberCondition(member, lll.MemberReady, metav1.ConditionFalse, "PodNotReady", @@ -1135,6 +1209,18 @@ func (r *EtcdMemberReconciler) updateStatus(ctx context.Context, member *lll.Etc } } + // Track the volume's lifecycle marker alongside member readiness: a + // member that is serving means its volume is in use, anything else means + // it is still filling. Best-effort — never fail a reconcile over an + // observability annotation. + pvcStatus := PVCStatusInitializing + if ready { + pvcStatus = PVCStatusReady + } + if err := r.setPVCStatus(ctx, member, pvcStatus); err != nil { + log.Error(err, "failed to update PVC status annotation") + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } diff --git a/controllers/etcdmember_controller_test.go b/controllers/etcdmember_controller_test.go index 56096f8a..6fbd6a15 100644 --- a/controllers/etcdmember_controller_test.go +++ b/controllers/etcdmember_controller_test.go @@ -359,6 +359,159 @@ func TestEnsurePVC_AcceptsOwnPVC(t *testing.T) { } } +// TestEnsurePVC_CreatesVolumeWithoutOwnerReference pins the ownership model +// that keeps etcd data alive through events nobody intended as "discard the +// data". +// +// A controller owner reference does two jobs at once: it says "this is mine" +// and it says "delete me with my owner". For a data volume the second job is +// a liability — every route that removes an EtcdMember (a stray kubectl +// delete, a GitOps prune, a CRD replacement, a chart rollback that drops the +// CRs) takes the volume with it, silently and irreversibly. Identity moves to +// an annotation so recognition survives without the coupling. +func TestEnsurePVC_CreatesVolumeWithoutOwnerReference(t *testing.T) { + ctx := context.Background() + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("member-uid")}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test", + }, + } + c, _ := newTestClient(t, member) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + + if err := r.ensurePVC(ctx, member); err != nil { + t.Fatalf("ensurePVC: %v", err) + } + + pvc := mustGet(t, c, "data-test-0", "ns", &corev1.PersistentVolumeClaim{}) + if len(pvc.OwnerReferences) != 0 { + t.Fatalf("data volume must carry no owner reference; got %+v", pvc.OwnerReferences) + } + if got := pvc.Annotations[AnnPVCMemberUID]; got != "member-uid" { + t.Fatalf("member-uid annotation = %q, want %q", got, "member-uid") + } + if got := pvc.Annotations[AnnPVCStatus]; got != PVCStatusInitializing { + t.Fatalf("fresh volume status = %q, want %q", got, PVCStatusInitializing) + } + // The annotation must be enough to recognise the volume on the next pass. + if !pvcBelongsTo(pvc, member) { + t.Fatalf("member must recognise its own volume by annotation") + } +} + +// TestEnsurePVC_MigratesLegacyOwnerReference covers the upgrade path: volumes +// created by an older build — and those stamped by the in-place migration +// tool — carry a controller owner reference and no annotation. Leaving that +// reference in place would keep exactly the cascade this change removes, so +// the first reconcile after the upgrade must strip it and stamp the marker. +func TestEnsurePVC_MigratesLegacyOwnerReference(t *testing.T) { + ctx := context.Background() + uid := types.UID("legacy-uid") + yes := true + legacy := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-test-0", Namespace: "ns", + OwnerReferences: []metav1.OwnerReference{{ + APIVersion: "etcd-operator.cozystack.io/v1alpha2", + Kind: "EtcdMember", + Name: "test-0", + UID: uid, + Controller: &yes, + BlockOwnerDeletion: &yes, + }}, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("1Gi")}, + }, + }, + } + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: uid}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test", + }, + } + c, _ := newTestClient(t, member, legacy) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + + if err := r.ensurePVC(ctx, member); err != nil { + t.Fatalf("ensurePVC on legacy volume: %v", err) + } + + pvc := mustGet(t, c, "data-test-0", "ns", &corev1.PersistentVolumeClaim{}) + if len(pvc.OwnerReferences) != 0 { + t.Fatalf("legacy owner reference must be stripped; got %+v", pvc.OwnerReferences) + } + if got := pvc.Annotations[AnnPVCMemberUID]; got != string(uid) { + t.Fatalf("member-uid annotation = %q, want %q", got, uid) + } + if member.Status.PVCName != "data-test-0" { + t.Fatalf("PVCName not recorded: %q", member.Status.PVCName) + } +} + +// TestMemberDeletion_LeavesTheVolumeBehind is the regression this whole change +// exists for: an EtcdMember going away must not take its data with it. +// +// The finalizer removes the member from etcd and lets the CR go, and the +// volume stays — no owner reference to cascade through, and nothing in the +// deletion path reclaims it. Recovering or discarding what is left is then a +// human's call. +func TestMemberDeletion_LeavesTheVolumeBehind(t *testing.T) { + ctx := context.Background() + now := metav1.Now() + uid := types.UID("doomed-uid") + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-0", Namespace: "ns", UID: uid, + Labels: memberLabels("test", "test-0"), + DeletionTimestamp: &now, + Finalizers: []string{MemberFinalizer}, + }, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test", + }, + Status: lll.EtcdMemberStatus{PVCName: "data-test-0", MemberID: "abc"}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-test-0", Namespace: "ns", + Annotations: map[string]string{AnnPVCMemberUID: string(uid)}, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce}, + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{corev1.ResourceStorage: resource.MustParse("1Gi")}, + }, + }, + } + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Status: lll.EtcdClusterStatus{ClusterID: "deadbeef", ReadyMembers: 3}, + } + c, _ := newTestClient(t, member, pvc, cluster) + fe := newFakeEtcd(0xdeadbeef) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(fe)} + + if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: "test-0", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile of deleting member: %v", err) + } + + got := &corev1.PersistentVolumeClaim{} + if err := c.Get(ctx, types.NamespacedName{Name: "data-test-0", Namespace: "ns"}, got); err != nil { + t.Fatalf("data volume must survive its member's deletion; got %v", err) + } + if got.DeletionTimestamp != nil { + t.Fatalf("data volume must not be marked for deletion by the member's finalizer") + } +} + // TestEnsurePVC_AppliesStorageClassName covers the wiring of // spec.storage.storageClassName onto the created PVC. The propagation // is what makes per-cluster StorageClass overrides actually take effect @@ -974,6 +1127,77 @@ func TestUpdateStatus_ReplacesStuckMember(t *testing.T) { } } +// TestUpdateStatus_ReplacingStuckMemberReclaimsItsVolume: the second of the +// two paths that deliberately destroy data. The member crash-loops on its own +// data dir while the quorum gate proves the rest of the cluster is healthy, so +// its contents are both unusable and already replicated — the replacement is +// meant to start clean and sync from peers. Leaving the volume would also +// strand it: replacements get fresh apiserver-assigned names, so nothing would +// ever mount it again. +func TestUpdateStatus_ReplacingStuckMemberReclaimsItsVolume(t *testing.T) { + ctx := context.Background() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(3)}, + } + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-1", Namespace: "ns", UID: types.UID("stuck-uid"), Labels: memberLabels("test", "test-1")}, + Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test"}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-test-1", Namespace: "ns", + Annotations: map[string]string{AnnPVCMemberUID: "stuck-uid"}, + }, + } + c, _ := newTestClient(t, cluster, member, pvc, crashLoopPod("test-1", "ns")) + clusterWithReady(t, c, "test", "ns", 2) // 2/3 ready → quorum without test-1 + + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + if _, err := r.updateStatus(ctx, member); err != nil { + t.Fatalf("updateStatus: %v", err) + } + + if err := c.Get(ctx, types.NamespacedName{Name: "data-test-1", Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); !apierrors.IsNotFound(err) { + t.Fatalf("replaced member's volume should have been reclaimed; got %v", err) + } +} + +// TestUpdateStatus_KeepingStuckMemberKeepsItsVolume is the mirror image: when +// the quorum gate refuses the replacement, neither the member nor its data may +// be touched. A cluster-wide outage looks exactly like this on every member at +// once, and reclaiming there would turn an outage into data loss. +func TestUpdateStatus_KeepingStuckMemberKeepsItsVolume(t *testing.T) { + ctx := context.Background() + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: lll.EtcdClusterSpec{Replicas: ptrInt32(3)}, + } + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-1", Namespace: "ns", UID: types.UID("stuck-uid"), Labels: memberLabels("test", "test-1")}, + Spec: lll.EtcdMemberSpec{ClusterName: "test", Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, InitialCluster: "x", ClusterToken: "test"}, + } + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "data-test-1", Namespace: "ns", + Annotations: map[string]string{AnnPVCMemberUID: "stuck-uid"}, + }, + } + c, _ := newTestClient(t, cluster, member, pvc, crashLoopPod("test-1", "ns")) + clusterWithReady(t, c, "test", "ns", 1) // 1/3 ready → no quorum without test-1 + + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + if _, err := r.updateStatus(ctx, member); err != nil { + t.Fatalf("updateStatus: %v", err) + } + + if err := c.Get(ctx, types.NamespacedName{Name: "data-test-1", Namespace: "ns"}, + &corev1.PersistentVolumeClaim{}); err != nil { + t.Fatalf("volume must survive when the replacement is refused; got %v", err) + } +} + // TestUpdateStatus_KeepsStuckMemberWithoutQuorum: the same crash-looping member // is NOT deleted when the rest of the cluster lacks quorum — self-heal must // never cascade a cluster-wide outage into mass deletion. @@ -1832,11 +2056,16 @@ func TestReconcile_DormantMemberDeletesPod(t *testing.T) { } else if !apierrors.IsNotFound(err) { t.Fatalf("unexpected error fetching Pod: %v", err) } - // PVC must still exist with the EtcdMember as its owner-controller — - // nothing reparented anything. + // PVC must still exist, still marked as this member's — nothing + // reparented anything. gotPVC := mustGet(t, c, "data-test-saved1", "ns", &corev1.PersistentVolumeClaim{}) - if !pvcOwnedBy(gotPVC, dormant) { - t.Fatalf("PVC owner-controller must still be the EtcdMember; got %+v", gotPVC.OwnerReferences) + if !pvcBelongsTo(gotPVC, dormant) { + t.Fatalf("PVC must still be marked as the EtcdMember's; got annotations %+v owner %+v", + gotPVC.Annotations, gotPVC.OwnerReferences) + } + // A paused member's volume is intact but unmounted: detached. + if got := gotPVC.Annotations[AnnPVCStatus]; got != PVCStatusDetached { + t.Fatalf("paused member's PVC should be marked %q; got %q", PVCStatusDetached, got) } // Status.PodName cleared so /status reflects reality. gotMember := mustGet(t, c, "test-saved1", "ns", &lll.EtcdMember{}) @@ -1901,10 +2130,11 @@ func TestReconcile_WakeFromDormantCreatesPod(t *testing.T) { if gotPod.Name != "test-saved1" { t.Fatalf("expected Pod test-saved1 to exist after wake") } - // PVC must still exist with the same owner. + // PVC must still exist, still marked as the woken member's. gotPVC := mustGet(t, c, "data-test-saved1", "ns", &corev1.PersistentVolumeClaim{}) - if !pvcOwnedBy(gotPVC, woken) { - t.Fatalf("PVC owner-controller must still be the woken EtcdMember; got %+v", gotPVC.OwnerReferences) + if !pvcBelongsTo(gotPVC, woken) { + t.Fatalf("PVC must still be marked as the woken EtcdMember's; got annotations %+v owner %+v", + gotPVC.Annotations, gotPVC.OwnerReferences) } } diff --git a/controllers/helpers.go b/controllers/helpers.go index 087038a7..629d096a 100644 --- a/controllers/helpers.go +++ b/controllers/helpers.go @@ -1,13 +1,16 @@ package controllers import ( + "context" "fmt" "path" "regexp" "strings" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" lll "github.com/cozystack/etcd-operator/api/v1alpha2" ) @@ -34,6 +37,16 @@ const ( // graceful removal from the etcd cluster before deletion. MemberFinalizer = "etcd-operator.cozystack.io/member-cleanup" + // ClusterFinalizer is placed on EtcdCluster resources so deleting one + // also reclaims its data volumes. Those volumes carry no owner + // reference (see AnnPVCMemberUID), which is what keeps a member + // disappearing from destroying data — but a deleted EtcdCluster is the + // user declaring the cluster finished, so its storage must go too. The + // finalizer is held until the volumes are actually gone, not merely + // asked to go, so "the EtcdCluster is gone" implies "its storage is + // gone". + ClusterFinalizer = "etcd-operator.cozystack.io/cluster-cleanup" + // ReservedAnnotationPrefix namespaces the operator-interpreted // annotations below. additionalMetadata must never be able to set a // key under this prefix (applyAdditionalMetadata strips it): the @@ -43,6 +56,39 @@ const ( // turn data-dir-subpath into a user-controllable path into --data-dir. ReservedAnnotationPrefix = "etcd-operator.cozystack.io/" + // AnnPVCMemberUID records the UID of the EtcdMember a data PVC was + // created for. It is how the member controller recognises its own + // volume, replacing the controller owner reference that used to serve + // that purpose. + // + // The distinction matters because an owner reference does two jobs at + // once: it identifies the owner AND it makes the object cascade-delete + // with it. For a data volume the second job is a liability — every + // route that removes an EtcdMember (an operator's kubectl delete, a + // GitOps prune, a CRD replacement, a Helm rollback that drops the CRs) + // silently takes the data with it, whether or not anyone intended to + // discard it. Identity via annotation keeps the recognition and drops + // the coupling: volumes are deleted only where the operator explicitly + // decides to (see deleteMemberPVC). + AnnPVCMemberUID = ReservedAnnotationPrefix + "member-uid" + + // AnnPVCStatus mirrors the lifecycle marker CloudNativePG puts on its + // instance volumes (cnpg.io/pvcStatus). It is observability, not + // control flow: nothing branches on it, but it answers "is this volume + // in use, still filling, or left behind?" with a kubectl get -o custom-columns + // instead of a cross-reference against the member list. + AnnPVCStatus = ReservedAnnotationPrefix + "pvc-status" + + // PVCStatusInitializing — the volume exists, its member has not reported + // Ready yet (fresh member joining, or a restore still populating it). + PVCStatusInitializing = "initializing" + // PVCStatusReady — the volume is mounted by a member that is Ready. + PVCStatusReady = "ready" + // PVCStatusDetached — the volume is intact but no Pod is using it: a + // paused (dormant) member, or a volume whose member is gone. Detached + // volumes are never reclaimed automatically. + PVCStatusDetached = "detached" + // AnnHeadlessServiceName overrides the headless Service name a member's // DNS identity keys off: its Pod subdomain and every peer/client URL // the operator constructs for it. Absent ⇒ the cluster's own name @@ -572,6 +618,61 @@ func setMemberCondition(member *lll.EtcdMember, condType string, status metav1.C return true } +// memberPVCName is the data volume name for a member. Kept in one place +// because both controllers derive it: the member controller to create and +// recognise the volume, the cluster controller to reclaim it. +func memberPVCName(memberName string) string { + return "data-" + memberName +} + +// pvcBelongsTo reports whether a data PVC was created for this member. +// +// Primary check is the member-UID annotation. The owner-reference fallback +// covers volumes created before the ownership model changed, and volumes +// stamped by the in-place migration tool — both carry a controller owner ref +// and no annotation until ensurePVC migrates them. +// +// A volume with neither marker is refused rather than adopted: silently +// inheriting another member's data dir crash-loops the pod when etcd finds a +// removed member ID in the WAL. +func pvcBelongsTo(pvc *corev1.PersistentVolumeClaim, member *lll.EtcdMember) bool { + if uid, ok := pvc.Annotations[AnnPVCMemberUID]; ok { + return uid == string(member.UID) + } + for _, o := range pvc.OwnerReferences { + if o.Kind == "EtcdMember" && o.UID == member.UID { + return true + } + } + return false +} + +// deleteMemberPVC reclaims a member's data volume. +// +// This is the ONLY path that destroys etcd data, and it is called exclusively +// from places where the operator itself decided to retire a member and the +// data is either replicated elsewhere or already unusable: +// +// - scale-down, where the removed member's contents live on the remaining +// peers (MemberRemove has already run via the finalizer), and +// - self-heal replacement of a member whose data dir is broken enough to +// crash-loop it, gated on the rest of the cluster holding quorum. +// +// Deliberately NOT wired to EtcdMember deletion in general. A member CR can +// disappear for reasons that say nothing about the data's value — a stray +// kubectl delete, a GitOps prune, a CRD replacement, a chart rollback — and +// none of those should reach the volume. What is left behind is visible as a +// detached PVC (AnnPVCStatus) and reclaimed by a human. +func deleteMemberPVC(ctx context.Context, c client.Client, namespace, memberName string) error { + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: memberPVCName(memberName), Namespace: namespace}, + } + if err := c.Delete(ctx, pvc); err != nil && !errors.IsNotFound(err) { + return fmt.Errorf("delete PVC %q: %w", pvc.Name, err) + } + return nil +} + // setCondition inserts or updates a condition, preserving LastTransitionTime // when the status has not changed. func setCondition(conditions *[]metav1.Condition, c metav1.Condition) { diff --git a/docs/concepts.md b/docs/concepts.md index 6cd535ba..fa4e36b6 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -10,7 +10,7 @@ Two custom resources, one of them user-facing. **`EtcdCluster`** — the user-facing object. It captures cluster-wide intent: replica count, etcd version, per-member storage size, a progress deadline. This is the only resource users normally touch. -**`EtcdMember`** — one per etcd member. Created and deleted by the cluster controller. Each `EtcdMember` owns its Pod and PVC. Users should not create or edit these directly. +**`EtcdMember`** — one per etcd member. Created and deleted by the cluster controller. Each `EtcdMember` owns its Pod; its data PVC is deliberately **not** owned by it (see [data volume lifecycle](#data-volume-lifecycle)). Users should not create or edit these directly. There is **no StatefulSet**. Each member's Pod and PVC are reconciled independently by the member controller. The motivation is protocol awareness: scale-up adds a member as a learner first and only promotes once it's caught up; scale-down runs `MemberRemove` via a finalizer before reclaiming the Pod; pod restarts reuse the existing data dir and rejoin with the same etcd-side member ID. None of these flows fit StatefulSet's "all replicas are one fungible workload" model. @@ -98,7 +98,7 @@ If the seed's pod hasn't been created yet (between Create and Pod-up), the contr ### Pause (1→0) -When the cluster controller's `scaleDown` observes `desired==0 && len(running)==1`, it Patches `spec.dormant=true` on the surviving member. The CR is **not** deleted. On the next reconcile of that member, the member controller observes `spec.dormant=true` and runs `ensurePodAbsent` — deletes the Pod, clears `status.podName`, surfaces `Ready=False/Paused`. The PVC is not touched. It keeps its existing owner-ref to the `EtcdMember`, which still exists. So nothing reparents, nothing cascade-deletes. +When the cluster controller's `scaleDown` observes `desired==0 && len(running)==1`, it Patches `spec.dormant=true` on the surviving member. The CR is **not** deleted. On the next reconcile of that member, the member controller observes `spec.dormant=true` and runs `ensurePodAbsent` — deletes the Pod, clears `status.podName`, surfaces `Ready=False/Paused`. The volume is not touched — it keeps its member-UID marker and is flagged `pvc-status: detached` while nothing mounts it. So nothing reparents, and nothing is reclaimed. Intermediate steps of a multi-member descent (3→2, 2→1) are normal scale-downs: pick newest, Delete CR, finalizer runs `MemberRemove`. Only the final 1→0 step flips dormant. @@ -121,6 +121,37 @@ An earlier iteration of this feature deleted the CR, reparented the PVC to the ` The single exception is the steady-state call to `updateStatus`, which receives the full active set (including dormant). `updateStatus`'s Paused branch uses `findDormantMember(members)` to name the parked PVC in the `Available=False/Paused` message. Stripping the dormant member at that call site would silently fall back to the fresh-zero "no data has been written" message even on real dormant clusters. The asymmetry is deliberate and the call site is commented. +## Data volume lifecycle + +A member's data PVC carries **no owner reference**. Nothing in Kubernetes garbage-collects it: not deleting the `EtcdMember`, not deleting the `EtcdCluster`, not removing the CRDs. Reclaiming is the operator's own decision, taken in three specific places — and nowhere else. + +This is a deliberate departure from the usual "child object owned by its CR" pattern, because an owner reference does two jobs at once — it marks the owner, and it makes the object cascade-delete with that owner. The first is wanted, the second is a liability for a volume holding the only copy of a database. Every route that removes an `EtcdMember` would take the data with it: a stray `kubectl delete`, a GitOps prune, a CRD replacement during an upgrade, a chart rollback that drops the CRs. None of those say anything about whether the data still matters, and all of them are irreversible against a `Delete`-reclaim StorageClass. + +So identity and lifetime are separated: + +- **Identity** is the `etcd-operator.cozystack.io/member-uid` annotation. The member controller mounts a volume only when the annotation matches its own UID; anything else is refused rather than adopted (inheriting a foreign data dir crash-loops the pod once etcd finds a removed member ID in the WAL). Volumes from older operator versions, and those stamped by the migration tool, carry a controller owner reference instead — the first reconcile after upgrading migrates them: annotation stamped, owner reference stripped. +- **Lifetime** is the operator's explicit decision. Three paths delete a volume, each one a place where the operator itself decided the data is finished: + - **scale-down** — the departing member's contents live on the remaining peers (its finalizer ran `MemberRemove` first). The shrink was asked for, so the storage goes back to the pool. + - **crash-loop replacement** — the member cannot boot on its own data dir, and the quorum gate proved the rest of the cluster is healthy. The replacement starts clean and syncs from peers. + - **cluster deletion** — deleting the `EtcdCluster` is the user declaring the cluster finished, so its storage goes with it. This runs from the cluster's own finalizer (`etcd-operator.cozystack.io/cluster-cleanup`), which is held until the volumes are actually gone, not merely asked to go: a volume stays `Terminating` while a Pod still mounts it, and releasing early would let the `EtcdCluster` disappear with storage still allocated behind it — or let a namespace delete look complete while PVCs were still draining. + +Everything else leaves the volume in place. A paused (`spec.replicas: 0`) cluster keeps its member CR and its volume; a member deleted by anything other than the paths above — a stray `kubectl delete`, a GitOps prune, a CRD replacement, a chart rollback — leaves a volume behind that no future member will mount, because replacements get fresh apiserver-assigned names. + +Leftover volumes are visible rather than silent. Each carries `etcd-operator.cozystack.io/pvc-status`, mirroring the marker CloudNativePG puts on its instance volumes: + +| Value | Meaning | +|---|---| +| `initializing` | The volume exists; its member has not reported `Ready` yet (joining, or a restore still populating it). | +| `ready` | Mounted by a member that is serving. | +| `detached` | Intact, nothing mounting it: a paused member, or a volume whose member is gone. | + +```sh +kubectl get pvc -l etcd-operator.cozystack.io/cluster= -n \ + -o custom-columns='NAME:.metadata.name,STATUS:.metadata.annotations.etcd-operator\.cozystack\.io/pvc-status' +``` + +Nothing branches on this annotation — it is observability. Reclaiming a detached volume is a human's call. + ## Storage Each member's data dir is configured via `spec.storage`, a struct with `size`, `medium`, and an optional `storageClassName`. The medium chooses between a PVC and a tmpfs `emptyDir`; the locking pattern protects size and medium just like `replicas` and `version` — a mid-flight flip is locked out until the current target is reached or the deadline expires. @@ -149,7 +180,7 @@ On every reconcile of a memory-backed member the controller stamps `Status.PodUI - Pod present, UID matches → steady state. - Pod absent (or UID differs) with a previously recorded UID → loss confirmed. -The member controller self-deletes the `EtcdMember`. The existing finalizer runs `MemberRemove` against quorum-reachable peers and the Pod / PVC owner-refs handle the rest of GC. The cluster controller's normal `current < desired` arm then scales up: a fresh `EtcdMember` is created with a new `GenerateName` and a new etcd-side member ID. There is no in-place "rejoin with empty data dir" — that path would require lying to raft. +The member controller self-deletes the `EtcdMember`. The existing finalizer runs `MemberRemove` against quorum-reachable peers and the Pod's owner reference handles its GC; the memory-backed member has no volume to reclaim. The cluster controller's normal `current < desired` arm then scales up: a fresh `EtcdMember` is created with a new `GenerateName` and a new etcd-side member ID. There is no in-place "rejoin with empty data dir" — that path would require lying to raft. If quorum is already lost across multiple simultaneous failures, `MemberRemove` will fail and the dying members stay in `Terminating` until quorum returns. That is the correct outcome: the cluster is dead and the user has to recreate it. The operator does not try to be clever about restoring a quorum from inconsistent half-states. @@ -163,7 +194,7 @@ The member controller detects this and replaces the member: - **Trigger.** The etcd container is not ready and has restarted at least `dataLossRestartThreshold` (5) times. `OOMKilled` is excluded (whether it's the current or the last termination) — that's a resource problem re-creating the member would not fix — and a Pod that is itself being deleted (drain/eviction/manual restart) is never treated as stuck. - **Quorum gate.** The operator deletes the member only when the *rest* of the cluster still has quorum, so a cluster-wide outage (many members crashing at once) never cascades into mass deletion. The count is read from `Status.ReadyMembers`, which the cluster controller maintains and which can lag; if the stuck member is still counted ready, the gate subtracts it. As a second line of defence the finalizer's `MemberRemove` is itself quorum-gated, so even a stale-high count cannot delete data below quorum. -- **Replacement.** Deleting the `EtcdMember` runs the finalizer's clean `MemberRemove`, the member-owned PVC is GC'd (discarding the corrupt data dir), and the cluster controller gap-fills a fresh `GenerateName` member with a current `--initial-cluster` and a **new** etcd member ID — not a same-ID rejoin. +- **Replacement.** Deleting the `EtcdMember` runs the finalizer's clean `MemberRemove`, the operator explicitly reclaims that member's volume (discarding the corrupt data dir — one of the two paths allowed to do so, see [data volume lifecycle](#data-volume-lifecycle)), and the cluster controller gap-fills a fresh `GenerateName` member with a current `--initial-cluster` and a **new** etcd member ID — not a same-ID rejoin. - **Latency.** `CrashLoopBackOff` caps its backoff at 5 minutes, so reaching 5 restarts takes on the order of **tens of minutes**, not the ~5s of the memory Pod-loss path. A deliberately-deleted-and-replaced member during this window is expected operator behavior, not a fault. A slow restore or slow learner join on the *replacement* can itself trip the threshold and be replaced again; this is quorum-gated and self-limiting, but expect it on a struggling cluster. ### What is missing from memory clusters today diff --git a/docs/installation.md b/docs/installation.md index d8c6ab4c..74bf401c 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -318,7 +318,7 @@ kubectl delete crd etcdclusters.etcd-operator.cozystack.io \ etcdsnapshots.etcd-operator.cozystack.io ``` -Deleting an `EtcdCluster` while it's running cascades through every owned resource: the operator's finalizer on each `EtcdMember` calls `MemberRemove` (when the cluster itself is also being deleted, the operator detects this and skips `MemberRemove` to avoid a deadlock — see `handleDeletion` in `controllers/etcdmember_controller.go`). Pods and PVCs are then GC'd via owner-refs. +Deleting an `EtcdCluster` while it's running cascades through every owned resource: the operator's finalizer on each `EtcdMember` calls `MemberRemove` (when the cluster itself is also being deleted, the operator detects this and skips `MemberRemove` to avoid a deadlock — see `handleDeletion` in `controllers/etcdmember_controller.go`). Pods are then GC'd via owner-refs. Data volumes carry no owner reference by design ([data volume lifecycle](concepts.md#data-volume-lifecycle)), so they are reclaimed explicitly by the cluster's own finalizer, which is held until they are actually gone — a deleted `EtcdCluster` therefore implies its storage is gone too, and a namespace delete does not complete while volumes are still draining. If the operator is uninstalled while `EtcdCluster` resources still exist, they're stranded — the finalizers won't run because no controller is reading the queue. Recovery is to either re-install the operator, or `kubectl patch ... --type=merge -p '{"metadata":{"finalizers":null}}'` on each `EtcdMember` (manual, leaves PVCs and Pods in place — clean them up by label). diff --git a/docs/operations.md b/docs/operations.md index 48649db8..fc0da235 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -57,7 +57,7 @@ kubectl patch etcdcluster.etcd-operator.cozystack.io -n --type=merge -p '{"spec":{"replicas":3}}' ``` -Picks the most-recently-created member as the victim (`CreationTimestamp` DESC, name DESC tiebreak). The finalizer calls `MemberRemove` against the remaining peers before the Pod and PVC are garbage-collected. No special seed-protection — the seed (the original bootstrap member) has no permanent special role and can be removed like any other member. +Picks the most-recently-created member as the victim (`CreationTimestamp` DESC, name DESC tiebreak). The finalizer calls `MemberRemove` against the remaining peers, the Pod is garbage-collected with its member, and the operator explicitly reclaims that member's data volume — its contents are already replicated on the peers that remain. No special seed-protection — the seed (the original bootstrap member) has no permanent special role and can be removed like any other member. ### Pause (scale to 0) @@ -66,7 +66,7 @@ kubectl patch etcdcluster.etcd-operator.cozystack.io -n --type=merge -p '{"spec":{"replicas":0}}' ``` -For an N>1 cluster this is a staged descent: each intermediate step (`MemberRemove` + Pod/PVC GC) until one member remains, then a 1→0 "pause" — the surviving member's `spec.dormant` is patched to `true`. The Pod goes away; the PVC stays owned by the `EtcdMember`, which itself stays alive. `etcdctl` from outside is no longer reachable (no Pod) but the data is intact. +For an N>1 cluster this is a staged descent: each intermediate step (`MemberRemove`, Pod GC, volume reclaimed) until one member remains, then a 1→0 "pause" — the surviving member's `spec.dormant` is patched to `true`. The Pod goes away; the volume stays, marked `pvc-status: detached`, and its `EtcdMember` stays alive. `etcdctl` from outside is no longer reachable (no Pod) but the data is intact. Observable state once paused: @@ -161,7 +161,11 @@ kubectl get etcdmember,pvc -l etcd-operator.cozystack.io/cluster= -n .yaml ``` -The PVC GC step is important: re-creating before the prior PVCs are gone causes the new EtcdMember to refuse to adopt them (`pvcOwnedBy` UID check fails — see [concepts](concepts.md#api-model)). The operator's check is a safety feature; the right answer is to wait. +Deleting the cluster reclaims its data volumes too — the cluster finalizer removes them and is only released once they are gone (see [data volume lifecycle](concepts.md#data-volume-lifecycle)). So a recreated cluster starts clean; wait for the `EtcdCluster` to actually disappear before re-applying: + +```sh +kubectl get etcdcluster.etcd-operator.cozystack.io,pvc -l etcd-operator.cozystack.io/cluster= -n +``` ### `Available=False/DeadlineExceeded` @@ -488,7 +492,7 @@ If a non-bootstrap PVC member's etcd cannot start — classically because its da - **Detection**: the etcd container is not ready and has restarted at least 5 times (`dataLossRestartThreshold`), excluding `OOMKilled` (a resource problem, not a lost data dir — raising `spec.resources.limits.memory` is the fix there, not replacement). A Pod that is being deleted (drain, eviction, manual restart) is never treated as stuck. - **Quorum gate**: the operator deletes the member only when the *rest* of the cluster still has quorum, so a cluster-wide outage never cascades into mass deletion. The gate reads `Status.ReadyMembers` (maintained by the cluster controller, and possibly lagging) and subtracts the stuck member if it is still counted; the finalizer's `MemberRemove` is independently quorum-gated as a backstop. -- **Replacement**: the `EtcdMember` CR is deleted → finalizer `MemberRemove` → the member-owned `data-` PVC is GC'd (discarding the corrupt data dir) → the cluster controller gap-fills a fresh `GenerateName` member with a current `--initial-cluster` and a **new** etcd member ID. +- **Replacement**: the `EtcdMember` CR is deleted → finalizer `MemberRemove` → the operator explicitly reclaims `data-` (discarding the corrupt data dir) → the cluster controller gap-fills a fresh `GenerateName` member with a current `--initial-cluster` and a **new** etcd member ID. **Detection latency is much longer than the Pod-loss path.** `CrashLoopBackOff` caps backoff at 5 minutes, so reaching 5 restarts takes **tens of minutes**, not ~5 s. Budget for that before concluding the operator is misbehaving — a member that vanishes and is replaced by a fresh-named one after a long crash-loop is the operator working as designed, not flapping. Note also that a replacement which is itself slow to come up (slow restore, slow learner join) can trip the same threshold and be replaced again; this is quorum-gated and harmless to the cluster, but expect repeated replacement on a genuinely unhealthy member.