diff --git a/pkg/controller/elasticsearch/driver/nodes.go b/pkg/controller/elasticsearch/driver/nodes.go index 9180116799..a30c2d0822 100644 --- a/pkg/controller/elasticsearch/driver/nodes.go +++ b/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) } diff --git a/pkg/controller/elasticsearch/nodespec/resources.go b/pkg/controller/elasticsearch/nodespec/resources.go index 36a16c6d4c..7e145d6da4 100644 --- a/pkg/controller/elasticsearch/nodespec/resources.go +++ b/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 } diff --git a/pkg/controller/elasticsearch/nodespec/resources_test.go b/pkg/controller/elasticsearch/nodespec/resources_test.go index a217d16336..4964a2c8ed 100644 --- a/pkg/controller/elasticsearch/nodespec/resources_test.go +++ b/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) - } - }) - } -} diff --git a/pkg/controller/elasticsearch/nodespec/statefulset.go b/pkg/controller/elasticsearch/nodespec/statefulset.go index 43873876d6..0c9682fcc7 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset.go +++ b/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 existingClaims []corev1.PersistentVolumeClaim + if existingSset, exists := existingStatefulSets.GetByName(statefulSetName); exists { + existingClaims = existingSset.Spec.VolumeClaimTemplates + } + claims, err := setVolumeClaimsControllerReference(nodeSet.VolumeClaimTemplates, existingClaims, es, scheme) if err != nil { return appsv1.StatefulSet{}, err } @@ -116,6 +123,7 @@ func BuildStatefulSet( func setVolumeClaimsControllerReference( persistentVolumeClaims []corev1.PersistentVolumeClaim, + existingClaims []corev1.PersistentVolumeClaim, es v1beta1.Elasticsearch, scheme *runtime.Scheme, ) ([]corev1.PersistentVolumeClaim, error) { @@ -123,14 +131,31 @@ func setVolumeClaimsControllerReference( // 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 + // from being 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) { diff --git a/pkg/controller/elasticsearch/nodespec/statefulset_test.go b/pkg/controller/elasticsearch/nodespec/statefulset_test.go new file mode 100644 index 0000000000..ee3e2c2753 --- /dev/null +++ b/pkg/controller/elasticsearch/nodespec/statefulset_test.go @@ -0,0 +1,223 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License; +// you may not use this file except in compliance with the Elastic License. + +package nodespec + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" +) + +func Test_setVolumeClaimsControllerReference(t *testing.T) { + varTrue := true + varFalse := false + es := v1beta1.Elasticsearch{ + TypeMeta: v1.TypeMeta{ + Kind: "Elasticsearch", + APIVersion: "elasticsearch.k8s.elastic.co/v1beta1", + }, + ObjectMeta: v1.ObjectMeta{ + Name: "es1", + Namespace: "default", + UID: "ABCDEF", + }, + } + tests := []struct { + name string + persistentVolumeClaims []corev1.PersistentVolumeClaim + existingClaims []corev1.PersistentVolumeClaim + wantClaims []corev1.PersistentVolumeClaim + }{ + { + name: "should set the ownerRef when building a new StatefulSet", + persistentVolumeClaims: []corev1.PersistentVolumeClaim{ + {ObjectMeta: v1.ObjectMeta{Name: "elasticsearch-data"}}, + }, + existingClaims: nil, + wantClaims: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: v1.ObjectMeta{ + Name: "elasticsearch-data", + OwnerReferences: []v1.OwnerReference{ + { + APIVersion: es.APIVersion, + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + }, + }, + { + name: "should set the ownerRef on user-provided claims when building a new StatefulSet", + persistentVolumeClaims: []corev1.PersistentVolumeClaim{ + {ObjectMeta: v1.ObjectMeta{Name: "elasticsearch-data"}}, + {ObjectMeta: v1.ObjectMeta{Name: "user-provided"}}, + }, + existingClaims: nil, + wantClaims: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: v1.ObjectMeta{ + Name: "elasticsearch-data", + OwnerReferences: []v1.OwnerReference{ + { + APIVersion: es.APIVersion, + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + { + ObjectMeta: v1.ObjectMeta{ + Name: "user-provided", + OwnerReferences: []v1.OwnerReference{ + { + APIVersion: es.APIVersion, + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + }, + }, + { + name: "should inherit existing claim ownerRefs that may have a different apiVersion", + persistentVolumeClaims: []corev1.PersistentVolumeClaim{ + {ObjectMeta: v1.ObjectMeta{Name: "elasticsearch-data"}}, + {ObjectMeta: v1.ObjectMeta{Name: "user-provided"}}, + }, + existingClaims: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: v1.ObjectMeta{ + Name: "elasticsearch-data", + OwnerReferences: []v1.OwnerReference{ + { + // claim already exists, with a different apiVersion + APIVersion: "elasticsearch.k8s.elastic.co/v1alpha1", + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + { + ObjectMeta: v1.ObjectMeta{ + Name: "user-provided", + OwnerReferences: []v1.OwnerReference{ + { + // claim already exists, with a different apiVersion + APIVersion: "elasticsearch.k8s.elastic.co/v1alpha1", + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + }, + // existing claims should be preserved + wantClaims: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: v1.ObjectMeta{ + Name: "elasticsearch-data", + OwnerReferences: []v1.OwnerReference{ + { + APIVersion: "elasticsearch.k8s.elastic.co/v1alpha1", + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + { + ObjectMeta: v1.ObjectMeta{ + Name: "user-provided", + OwnerReferences: []v1.OwnerReference{ + { + APIVersion: "elasticsearch.k8s.elastic.co/v1alpha1", + Kind: es.Kind, + Name: es.Name, + UID: es.UID, + Controller: &varTrue, + BlockOwnerDeletion: &varFalse, + }, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := setVolumeClaimsControllerReference(tt.persistentVolumeClaims, tt.existingClaims, es, k8s.Scheme()) + require.NoError(t, err) + require.Equal(t, tt.wantClaims, got) + }) + } +} + +func Test_getClaimMatchingName(t *testing.T) { + tests := []struct { + name string + claims []corev1.PersistentVolumeClaim + claimName string + want *corev1.PersistentVolumeClaim + }{ + { + name: "return matching claim", + claims: []corev1.PersistentVolumeClaim{ + {ObjectMeta: metav1.ObjectMeta{Name: "claim1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "claim2"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "claim3"}}, + }, + claimName: "claim2", + want: &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Name: "claim2"}}, + }, + { + name: "return nil if no match", + claims: []corev1.PersistentVolumeClaim{ + {ObjectMeta: metav1.ObjectMeta{Name: "claim1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "claim2"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "claim3"}}, + }, + claimName: "claim4", + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := getClaimMatchingName(tt.claims, tt.claimName); !reflect.DeepEqual(got, tt.want) { + t.Errorf("getMatchingClaim() = %v, want %v", got, tt.want) + } + }) + } +}