Add support for VolumeClaimDeletePolicies for Elasticsearch clusters (#4050)

Approach inspired by kubernetes/enhancements#1915

Adds an new volumeClaimDeletePolicy to the Elasticsearch Spec on the cluster level (not per NodeSet)

apiVersion: elasticsearch.k8s.elastic.co/v1
kind: Elasticsearch
metadata:
  name: es
spec:
  version: 7.10.1
  volumeClaimDeletePolicy: DeleteOnScaledownAndClusterDeletion
  nodeSets:
  - name: default
    count: 2

Possible values are DeleteOnScaledownAndClusterDeletion (default), DeleteOnScaledownOnly.

RemoveOnScaledownAndClusterDeletion relies on an owner reference pointing to the Elasticsearch resource to garbage collect PVCs once the Elasticsearch cluster has been deleted (existing behaviour).

It also runs additional garbage collection to remove PVCs on each reconciliation that are no longer in use because either the whole node set has been removed or individual nodes have been scaled down (existing behaviour).

RemoveOnScaledownOnly means the PVCs are kept around after the cluster has been deleted. This is implemented by removing the owner reference. Removal of PVCs on scale down happens as before.

Switching from one to the other strategy is allowed and is implemented by avoiding the StatefulSet templating mechanism. This is mainly because the PVC template in StatefulSets are considered immutable and it would require StatefulSets to be recreated in order to change the PVC ownership. Instead the operator edits the PVCs after they have been created by the StatefulSet controller.

Author: pebrc

Makefile

@@ -460,6 +460,7 @@ e2e-local: LOCAL_E2E_CTX := /tmp/e2e-local.json
 e2e-local:
 	@go run test/e2e/cmd/main.go run \
 		--test-run-name=e2e \
+		--operator-image=$(OPERATOR_IMAGE) \
 		--test-context-out=$(LOCAL_E2E_CTX) \
 		--test-license=$(TEST_LICENSE) \
 		--test-license-pkey-path=$(TEST_LICENSE_PKEY_PATH) \

config/crds/all-crds.yaml

@@ -2387,6 +2387,14 @@ spec:
             version:
               description: Version of Elasticsearch.
               type: string
+            volumeClaimDeletePolicy:
+              description: VolumeClaimDeletePolicy sets the policy for handling deletion
+                of PersistentVolumeClaims for all NodeSets. Possible values are DeleteOnScaledownOnly
+                and DeleteOnScaledownAndClusterDeletion. Defaults to DeleteOnScaledownAndClusterDeletion.
+              enum:
+              - DeleteOnScaledownOnly
+              - DeleteOnScaledownAndClusterDeletion
+              type: string
           required:
           - nodeSets
           - version

config/crds/bases/elasticsearch.k8s.elastic.co_elasticsearches.yaml

@@ -4418,6 +4418,15 @@ spec:
               version:
                 description: Version of Elasticsearch.
                 type: string
+              volumeClaimDeletePolicy:
+                description: VolumeClaimDeletePolicy sets the policy for handling
+                  deletion of PersistentVolumeClaims for all NodeSets. Possible values
+                  are DeleteOnScaledownOnly and DeleteOnScaledownAndClusterDeletion.
+                  Defaults to DeleteOnScaledownAndClusterDeletion.
+                enum:
+                - DeleteOnScaledownOnly
+                - DeleteOnScaledownAndClusterDeletion
+                type: string
             required:
             - nodeSets
             - version

deploy/eck-operator/charts/eck-operator-crds/templates/all-crds.yaml

@@ -2411,6 +2411,14 @@ spec:
             version:
               description: Version of Elasticsearch.
               type: string
+            volumeClaimDeletePolicy:
+              description: VolumeClaimDeletePolicy sets the policy for handling deletion
+                of PersistentVolumeClaims for all NodeSets. Possible values are DeleteOnScaledownOnly
+                and DeleteOnScaledownAndClusterDeletion. Defaults to DeleteOnScaledownAndClusterDeletion.
+              enum:
+              - DeleteOnScaledownOnly
+              - DeleteOnScaledownAndClusterDeletion
+              type: string
           required:
           - nodeSets
           - version

docs/reference/api-docs.asciidoc

@@ -895,6 +895,7 @@ ElasticsearchSpec holds the specification of an Elasticsearch cluster.
 | *`secureSettings`* __xref:{anchor_prefix}-github-com-elastic-cloud-on-k8s-pkg-apis-common-v1-secretsource[$$SecretSource$$]__ | SecureSettings is a list of references to Kubernetes secrets containing sensitive configuration options for Elasticsearch.
 | *`serviceAccountName`* __string__ | ServiceAccountName is used to check access from the current resource to a resource (eg. a remote Elasticsearch cluster) in a different namespace. Can only be used if ECK is enforcing RBAC on references.
 | *`remoteClusters`* __xref:{anchor_prefix}-github-com-elastic-cloud-on-k8s-pkg-apis-elasticsearch-v1-remotecluster[$$RemoteCluster$$] array__ | RemoteClusters enables you to establish uni-directional connections to a remote Elasticsearch cluster.
+| *`volumeClaimDeletePolicy`* __xref:{anchor_prefix}-github-com-elastic-cloud-on-k8s-pkg-apis-elasticsearch-v1-volumeclaimdeletepolicy[$$VolumeClaimDeletePolicy$$]__ | VolumeClaimDeletePolicy sets the policy for handling deletion of PersistentVolumeClaims for all NodeSets. Possible values are DeleteOnScaledownOnly and DeleteOnScaledownAndClusterDeletion. Defaults to DeleteOnScaledownAndClusterDeletion.
 |===
 
 
@@ -1030,6 +1031,18 @@ UpdateStrategy specifies how updates to the cluster should be performed.
 |===
 
 
+[id="{anchor_prefix}-github-com-elastic-cloud-on-k8s-pkg-apis-elasticsearch-v1-volumeclaimdeletepolicy"]
+=== VolumeClaimDeletePolicy (string) 
+
+VolumeClaimDeletePolicy describes the delete policy for handling PersistentVolumeClaims that hold Elasticsearch data. Inspired by https://github.com/kubernetes/enhancements/pull/2440
+
+.Appears In:
+****
+- xref:{anchor_prefix}-github-com-elastic-cloud-on-k8s-pkg-apis-elasticsearch-v1-elasticsearchspec[$$ElasticsearchSpec$$]
+****
+
+
+
 
 
 

pkg/apis/elasticsearch/v1/elasticsearch_types.go

@@ -66,8 +66,26 @@ type ElasticsearchSpec struct {
 	// RemoteClusters enables you to establish uni-directional connections to a remote Elasticsearch cluster.
 	// +optional
 	RemoteClusters []RemoteCluster `json:"remoteClusters,omitempty"`
+
+	// VolumeClaimDeletePolicy sets the policy for handling deletion of PersistentVolumeClaims for all NodeSets.
+	// Possible values are DeleteOnScaledownOnly and DeleteOnScaledownAndClusterDeletion. Defaults to DeleteOnScaledownAndClusterDeletion.
+	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Enum=DeleteOnScaledownOnly;DeleteOnScaledownAndClusterDeletion
+	VolumeClaimDeletePolicy VolumeClaimDeletePolicy `json:"volumeClaimDeletePolicy,omitempty"`
 }
 
+// VolumeClaimDeletePolicy describes the delete policy for handling PersistentVolumeClaims that hold Elasticsearch data.
+// Inspired by https://github.com/kubernetes/enhancements/pull/2440
+type VolumeClaimDeletePolicy string
+
+const (
+	// DeleteOnScaledownAndClusterDeletionPolicy remove PersistentVolumeClaims when the corresponding Elasticsearch node is removed.
+	DeleteOnScaledownAndClusterDeletionPolicy VolumeClaimDeletePolicy = "DeleteOnScaledownAndClusterDeletion"
+	// DeleteOnScaledownOnlyPolicy removes PersistentVolumeClaims on scale down of Elasticsearch nodes but retains all
+	// current PersistenVolumeClaims when the Elasticsearch cluster has been deleted.
+	DeleteOnScaledownOnlyPolicy VolumeClaimDeletePolicy = "DeleteOnScaledownOnly"
+)
+
 // TransportConfig holds the transport layer settings for Elasticsearch.
 type TransportConfig struct {
 	// Service defines the template for the associated Kubernetes Service object.
@@ -118,6 +136,13 @@ func (es ElasticsearchSpec) NodeCount() int32 {
 	return count
 }
 
+func (es ElasticsearchSpec) VolumeClaimDeletePolicyOrDefault() VolumeClaimDeletePolicy {
+	if es.VolumeClaimDeletePolicy == "" {
+		return DeleteOnScaledownAndClusterDeletionPolicy
+	}
+	return es.VolumeClaimDeletePolicy
+}
+
 // Auth contains user authentication and authorization security settings for Elasticsearch.
 type Auth struct {
 	// Roles to propagate to the Elasticsearch cluster.

pkg/controller/common/reconciler/secret.go

@@ -13,7 +13,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -118,7 +117,7 @@ func ReconcileSecretNoOwnerRef(c k8s.Client, expected corev1.Secret, softOwner r
 				// or if secret data is not strictly equal
 				!reflect.DeepEqual(expected.Data, reconciled.Data) ||
 				// or if an existing owner should be removed
-				hasOwner(&reconciled, ownerMeta)
+				k8s.HasOwner(&reconciled, ownerMeta)
 		},
 		UpdateReconciled: func() {
 			// set expected annotations and labels, but don't remove existing ones
@@ -127,7 +126,7 @@ func ReconcileSecretNoOwnerRef(c k8s.Client, expected corev1.Secret, softOwner r
 			reconciled.Annotations = maps.Merge(reconciled.Annotations, expected.Annotations)
 			reconciled.Data = expected.Data
 			// remove existing owner
-			removeOwner(&reconciled, ownerMeta)
+			k8s.RemoveOwner(&reconciled, ownerMeta)
 		},
 	}); err != nil {
 		return corev1.Secret{}, err
@@ -221,38 +220,3 @@ func GarbageCollectAllSoftOwnedOrphanSecrets(c k8s.Client, ownerKinds map[string
 	}
 	return nil
 }
-
-func hasOwner(resource metav1.Object, owner metav1.Object) bool {
-	if owner == nil || resource == nil {
-		return false
-	}
-	found, _ := findOwner(resource, owner)
-	return found
-}
-
-func removeOwner(resource metav1.Object, owner metav1.Object) {
-	if resource == nil || owner == nil {
-		return
-	}
-	found, index := findOwner(resource, owner)
-	if !found {
-		return
-	}
-	owners := resource.GetOwnerReferences()
-	// remove the owner at index i from the slice
-	newOwners := append(owners[:index], owners[index+1:]...)
-	resource.SetOwnerReferences(newOwners)
-}
-
-func findOwner(resource metav1.Object, owner metav1.Object) (found bool, index int) {
-	if owner == nil || resource == nil {
-		return false, 0
-	}
-	ownerRefs := resource.GetOwnerReferences()
-	for i := range ownerRefs {
-		if ownerRefs[i].Name == owner.GetName() && ownerRefs[i].UID == owner.GetUID() {
-			return true, i
-		}
-	}
-	return false, 0
-}

pkg/controller/common/reconciler/secret_test.go

@@ -245,188 +245,6 @@ func addOwner(secret *corev1.Secret, name string, uid types.UID) *corev1.Secret
 	return secret
 }
 
-func Test_hasOwner(t *testing.T) {
-	owner := sampleOwner()
-	type args struct {
-		resource metav1.Object
-		owner    metav1.Object
-	}
-	tests := []struct {
-		name string
-		args args
-		want bool
-	}{
-		{
-			name: "owner is referenced (same name and uid)",
-			args: args{
-				resource: addOwner(&corev1.Secret{}, owner.Name, owner.UID),
-				owner:    owner,
-			},
-			want: true,
-		},
-		{
-			name: "owner referenced among other owner references",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, "another-name", types.UID("another-id")), owner.Name, owner.UID),
-				owner:    owner,
-			},
-			want: true,
-		},
-		{
-			name: "owner not referenced",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, "another-name", types.UID("another-id")), "yet-another-name", "yet-another-uid"),
-				owner:    owner,
-			},
-			want: false,
-		},
-		{
-			name: "no owner ref",
-			args: args{
-				resource: &corev1.Secret{},
-				owner:    owner,
-			},
-			want: false,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if got := hasOwner(tt.args.resource, tt.args.owner); got != tt.want {
-				t.Errorf("hasOwner() = %v, want %v", got, tt.want)
-			}
-		})
-	}
-}
-
-func Test_removeOwner(t *testing.T) {
-	type args struct {
-		resource metav1.Object
-		owner    metav1.Object
-	}
-	tests := []struct {
-		name         string
-		args         args
-		wantResource *corev1.Secret
-	}{
-		{
-			name: "no owner: no-op",
-			args: args{
-				resource: &corev1.Secret{},
-				owner:    sampleOwner(),
-			},
-			wantResource: &corev1.Secret{},
-		},
-		{
-			name: "different owner: no-op",
-			args: args{
-				resource: addOwner(&corev1.Secret{}, "another-owner-name", "another-owner-id"),
-				owner:    sampleOwner(),
-			},
-			wantResource: addOwner(&corev1.Secret{}, "another-owner-name", "another-owner-id"),
-		},
-		{
-			name: "remove the single owner",
-			args: args{
-				resource: addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID),
-				owner:    sampleOwner(),
-			},
-			wantResource: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{}}},
-		},
-		{
-			name: "remove the owner from a list of owners",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID), "another-owner", "another-uid"),
-				owner:    sampleOwner(),
-			},
-			wantResource: addOwner(&corev1.Secret{}, "another-owner", "another-uid"),
-		},
-		{
-			name: "owner listed twice in the list (shouldn't happen): remove the first occurrence",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID), sampleOwner().Name, sampleOwner().UID),
-				owner:    sampleOwner(),
-			},
-			wantResource: addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID),
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			removeOwner(tt.args.resource, tt.args.owner)
-			require.Equal(t, tt.wantResource, tt.args.resource)
-		})
-	}
-}
-
-func Test_findOwner(t *testing.T) {
-	type args struct {
-		resource metav1.Object
-		owner    metav1.Object
-	}
-	tests := []struct {
-		name      string
-		args      args
-		wantFound bool
-		wantIndex int
-	}{
-		{
-			name: "no owner: not found",
-			args: args{
-				resource: &corev1.Secret{},
-				owner:    sampleOwner(),
-			},
-			wantFound: false,
-			wantIndex: 0,
-		},
-		{
-			name: "different owner: not found",
-			args: args{
-				resource: addOwner(&corev1.Secret{}, "another-owner-name", "another-owner-id"),
-				owner:    sampleOwner(),
-			},
-			wantFound: false,
-			wantIndex: 0,
-		},
-		{
-			name: "owner at index 0",
-			args: args{
-				resource: addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID),
-				owner:    sampleOwner(),
-			},
-			wantFound: true,
-			wantIndex: 0,
-		},
-		{
-			name: "owner at index 1",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, "another-owner", "another-uid"), sampleOwner().Name, sampleOwner().UID),
-				owner:    sampleOwner(),
-			},
-			wantFound: true,
-			wantIndex: 1,
-		},
-		{
-			name: "owner listed twice in the list (shouldn't happen): return the first occurrence (index 0)",
-			args: args{
-				resource: addOwner(addOwner(&corev1.Secret{}, sampleOwner().Name, sampleOwner().UID), sampleOwner().Name, sampleOwner().UID),
-				owner:    sampleOwner(),
-			},
-			wantFound: true,
-			wantIndex: 0,
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			gotFound, gotIndex := findOwner(tt.args.resource, tt.args.owner)
-			if gotFound != tt.wantFound {
-				t.Errorf("findOwner() gotFound = %v, want %v", gotFound, tt.wantFound)
-			}
-			if gotIndex != tt.wantIndex {
-				t.Errorf("findOwner() gotIndex = %v, want %v", gotIndex, tt.wantIndex)
-			}
-		})
-	}
-}
-
 func ownedSecret(namespace, name, ownerNs, ownerName, ownerKind string) *corev1.Secret {
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name, Labels: map[string]string{

pkg/controller/elasticsearch/driver/nodes.go

@@ -120,6 +120,10 @@ func (d *defaultDriver) reconcileNodeSpecs(
 		return results.WithError(err)
 	}
 
+	if err := reconcilePVCOwnerRefs(d.K8sClient(), d.ES); err != nil {
+		return results.WithError(err)
+	}
+
 	if err := GarbageCollectPVCs(d.K8sClient(), d.ES, actualStatefulSets, expectedResources.StatefulSets()); err != nil {
 		return results.WithError(err)
 	}