Inherit existing sset.volumeClaimTemplates ownerReferences

pkg/controller/elasticsearch/driver/nodes.go

@@ -48,7 +48,7 @@ func (d *defaultDriver) reconcileNodeSpecs(
 		return results.WithError(err)
 	}
 
-	expectedResources, err := nodespec.BuildExpectedResources(d.ES, keystoreResources, d.Scheme(), certResources)
+	expectedResources, err := nodespec.BuildExpectedResources(d.ES, keystoreResources, d.Scheme(), certResources, actualStatefulSets)
 	if err != nil {
 		return results.WithError(err)
 	}

pkg/controller/elasticsearch/nodespec/resources.go

@@ -42,6 +42,7 @@ func BuildExpectedResources(
 	keystoreResources *keystore.Resources,
 	scheme *runtime.Scheme,
 	certResources *certificates.CertificateResources,
+	existingStatefulSets sset.StatefulSetList,
 ) (ResourcesList, error) {
 	nodesResources := make(ResourcesList, 0, len(es.Spec.NodeSets))
 
@@ -62,7 +63,7 @@ func BuildExpectedResources(
 		}
 
 		// build stateful set and associated headless service
-		statefulSet, err := BuildStatefulSet(es, nodeSpec, cfg, keystoreResources, scheme)
+		statefulSet, err := BuildStatefulSet(es, nodeSpec, cfg, keystoreResources, existingStatefulSets, scheme)
 		if err != nil {
 			return nil, err
 		}

pkg/controller/elasticsearch/nodespec/resources_test.go

@@ -8,13 +8,7 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset"
-	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	corev1 "k8s.io/api/core/v1"
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func TestResourcesList_MasterNodesNames(t *testing.T) {
@@ -49,68 +43,3 @@ func TestResourcesList_MasterNodesNames(t *testing.T) {
 		})
 	}
 }
-
-func TestSetVolumeClaimsControllerReference(t *testing.T) {
-	es := v1beta1.Elasticsearch{
-		ObjectMeta: v1.ObjectMeta{
-			Name:      "es1",
-			Namespace: "default",
-			UID:       "ABCDEF",
-		},
-	}
-	type args struct {
-		volumeClaims []corev1.PersistentVolumeClaim
-	}
-	tests := []struct {
-		name    string
-		args    args
-		want    []string
-		wantErr bool
-	}{
-		{
-			name: "Simple test case",
-			args: args{
-				volumeClaims: []corev1.PersistentVolumeClaim{
-					{ObjectMeta: v1.ObjectMeta{Name: "elasticsearch-data"}},
-				},
-			},
-			want: []string{"elasticsearch-data"},
-		},
-		{
-			name: "With a user volume",
-			args: args{
-				volumeClaims: []corev1.PersistentVolumeClaim{
-					{ObjectMeta: v1.ObjectMeta{Name: "elasticsearch-data"}},
-					{ObjectMeta: v1.ObjectMeta{Name: "user-volume"}},
-				},
-			},
-			want: []string{"elasticsearch-data", "user-volume"},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := setVolumeClaimsControllerReference(tt.args.volumeClaims, es, k8s.Scheme())
-			if (err != nil) != tt.wantErr {
-				t.Errorf("BuildExpectedResources() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			assert.Equal(t, len(tt.want), len(got))
-
-			// Extract PVC names
-			actualPVCs := make([]string, len(got))
-			for i := range got {
-				actualPVCs[i] = got[i].Name
-			}
-			// Check the number of PVCs we got
-			assert.ElementsMatch(t, tt.want, actualPVCs)
-
-			// Check that VolumeClaimTemplates have an owner with the right settings
-			for _, pvc := range got {
-				assert.Equal(t, 1, len(pvc.OwnerReferences))
-				ownerRef := pvc.OwnerReferences[0]
-				require.False(t, *ownerRef.BlockOwnerDeletion)
-				assert.Equal(t, es.UID, ownerRef.UID)
-			}
-		})
-	}
-}

pkg/controller/elasticsearch/nodespec/statefulset.go

@@ -12,6 +12,7 @@ import (
 	"github.com/elastic/cloud-on-k8s/pkg/controller/common/reconciler"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/label"
 	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/settings"
+	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset"
 	esvolume "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/volume"
 	"github.com/elastic/cloud-on-k8s/pkg/utils/k8s"
 	appsv1 "k8s.io/api/apps/v1"
@@ -52,6 +53,7 @@ func BuildStatefulSet(
 	nodeSet v1beta1.NodeSet,
 	cfg settings.CanonicalConfig,
 	keystoreResources *keystore.Resources,
+	existingStatefulSets sset.StatefulSetList,
 	scheme *runtime.Scheme,
 ) (appsv1.StatefulSet, error) {
 	statefulSetName := v1beta1.StatefulSet(es.Name, nodeSet.Name)
@@ -76,7 +78,12 @@ func BuildStatefulSet(
 		ssetLabels[k] = v
 	}
 
-	claims, err := setVolumeClaimsControllerReference(nodeSet.VolumeClaimTemplates, es, scheme)
+	// maybe inherit volumeClaimTemplates ownerRefs from the existing StatefulSet
+	var inheritedClaims []corev1.PersistentVolumeClaim
+	if existingSset, exists := existingStatefulSets.GetByName(statefulSetName); exists {
+		inheritedClaims = existingSset.Spec.VolumeClaimTemplates
+	}
+	claims, err := setVolumeClaimsControllerReference(nodeSet.VolumeClaimTemplates, inheritedClaims, es, scheme)
 	if err != nil {
 		return appsv1.StatefulSet{}, err
 	}
@@ -116,21 +123,39 @@ func BuildStatefulSet(
 
 func setVolumeClaimsControllerReference(
 	persistentVolumeClaims []corev1.PersistentVolumeClaim,
+	existingClaims []corev1.PersistentVolumeClaim,
 	es v1beta1.Elasticsearch,
 	scheme *runtime.Scheme,
 ) ([]corev1.PersistentVolumeClaim, error) {
 	// set the owner reference of all volume claims to the ES resource,
 	// so PVC get deleted automatically upon Elasticsearch resource deletion
 	claims := make([]corev1.PersistentVolumeClaim, 0, len(persistentVolumeClaims))
 	for _, claim := range persistentVolumeClaims {
-		// Set the claim namespace to match the ES namespace.
-		// This is technically not required, but `SetControllerReference` does a safety check on
-		// object vs. owner namespace mismatch. We know the PVC will end up in ES namespace anyway,
-		// so it's safe to include it.
+		if existingClaim := getClaimMatchingName(existingClaims, claim.Name); existingClaim != nil {
+			// This claim already exists in the actual resource. Since the volumeClaimTemplates section of
+			// a StatefulSet is immutable, any modification to it will be rejected in the StatefulSet update.
+			// This is fine and we let it error-out. It is caught in a user-friendly way by the validating webhook.
+			//
+			// However, there is one case where the claim we build may differ from the existing one, that was
+			// built with a prior version of the operator. If the Elasticsearch apiVersion has changed,
+			// from eg. `v1beta1` to `v1`, we want to keep the existing ownerRef (pointing to eg. a `v1beta1` owner).
+			// Having ownerReferences with a "deprecated" apiVersion is fine, and does not prevent resources
+			// to be garbage collected as expected.
+			claim.OwnerReferences = existingClaim.OwnerReferences
+
+			claims = append(claims, claim)
+			continue
+		}
+
+		// Temporarily set the claim namespace to match the ES namespace, then set it back to empty.
+		// `SetControllerReference` does a safety check on object vs. owner namespace mismatch to cover common errors,
+		// but in this particular case we don't need to set a namespace in the claim template.
 		claim.Namespace = es.Namespace
 		if err := reconciler.SetControllerReference(&es, &claim, scheme); err != nil {
 			return nil, err
 		}
+		claim.Namespace = ""
+
 		// Set block owner deletion to false as the statefulset controller might not be able to do that if it cannot
 		// set finalizers on the resource.
 		// See https://github.com/elastic/cloud-on-k8s/issues/1884
@@ -143,6 +168,16 @@ func setVolumeClaimsControllerReference(
 	return claims, nil
 }
 
+// getClaimMatchingName returns a claim matching the given name.
+func getClaimMatchingName(claims []corev1.PersistentVolumeClaim, name string) *corev1.PersistentVolumeClaim {
+	for i, claim := range claims {
+		if claim.Name == name {
+			return &claims[i]
+		}
+	}
+	return nil
+}
+
 // UpdateReplicas updates the given StatefulSet with the given replicas,
 // and modifies the template hash label accordingly.
 func UpdateReplicas(statefulSet *appsv1.StatefulSet, replicas *int32) {