From 9f8d15db4fccb533ee01f37683a166ca3447155f Mon Sep 17 00:00:00 2001 From: Michael Morello Date: Fri, 4 Oct 2019 16:17:46 +0200 Subject: [PATCH 1/4] set BlockOwnerDeletion to false --- pkg/controller/elasticsearch/nodespec/statefulset.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/controller/elasticsearch/nodespec/statefulset.go b/pkg/controller/elasticsearch/nodespec/statefulset.go index 2a5c209bdd1..a009d117f01 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset.go +++ b/pkg/controller/elasticsearch/nodespec/statefulset.go @@ -5,6 +5,8 @@ package nodespec import ( + "fmt" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1" @@ -77,14 +79,20 @@ func BuildStatefulSet( // set the owner reference of all volume claims to the ES resource, // so PVC get deleted automatically upon Elasticsearch resource deletion + f := false claims := make([]corev1.PersistentVolumeClaim, 0, len(nodeSet.VolumeClaimTemplates)) for _, claim := range nodeSet.VolumeClaimTemplates { if err := controllerutil.SetControllerReference(&es, &claim, scheme); err != nil { return appsv1.StatefulSet{}, err } + for _, ref := range claim.OwnerReferences { + ref.BlockOwnerDeletion = &f + } claims = append(claims, claim) } + fmt.Printf("%v", claims) + sset := appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ Namespace: es.Namespace, From 9592c3f80ce1e12fbaf3d9370ba3831b74b88131 Mon Sep 17 00:00:00 2001 From: Michael Morello Date: Fri, 4 Oct 2019 17:06:08 +0200 Subject: [PATCH 2/4] Set BlockOwnerDeletion to false --- .../elasticsearch/nodespec/resources.go | 7 ++- .../elasticsearch/nodespec/resources_test.go | 63 +++++++++++++++++++ .../elasticsearch/nodespec/statefulset.go | 47 +++++++++----- 3 files changed, 99 insertions(+), 18 deletions(-) diff --git a/pkg/controller/elasticsearch/nodespec/resources.go b/pkg/controller/elasticsearch/nodespec/resources.go index 26c61220b6f..36a16c6d4c2 100644 --- a/pkg/controller/elasticsearch/nodespec/resources.go +++ b/pkg/controller/elasticsearch/nodespec/resources.go @@ -37,7 +37,12 @@ func (l ResourcesList) StatefulSets() sset.StatefulSetList { return ssetList } -func BuildExpectedResources(es v1beta1.Elasticsearch, keystoreResources *keystore.Resources, scheme *runtime.Scheme, certResources *certificates.CertificateResources) (ResourcesList, error) { +func BuildExpectedResources( + es v1beta1.Elasticsearch, + keystoreResources *keystore.Resources, + scheme *runtime.Scheme, + certResources *certificates.CertificateResources, +) (ResourcesList, error) { nodesResources := make(ResourcesList, 0, len(es.Spec.NodeSets)) ver, err := version.Parse(es.Spec.Version) diff --git a/pkg/controller/elasticsearch/nodespec/resources_test.go b/pkg/controller/elasticsearch/nodespec/resources_test.go index 4964a2c8ed1..9208059b158 100644 --- a/pkg/controller/elasticsearch/nodespec/resources_test.go +++ b/pkg/controller/elasticsearch/nodespec/resources_test.go @@ -8,7 +8,13 @@ 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/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" ) func TestResourcesList_MasterNodesNames(t *testing.T) { @@ -43,3 +49,60 @@ func TestResourcesList_MasterNodesNames(t *testing.T) { }) } } + +func TestSetVolumeClaimsControllerReference(t *testing.T) { + es := v1beta1.Elasticsearch{ + ObjectMeta: v1.ObjectMeta{ + Name: "es1", + Namespace: "default", + UID: "ABCDEF", + }, + } + require.NoError(t, v1beta1.AddToScheme(scheme.Scheme)) + 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"}}, + {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, scheme.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 a009d117f01..a92d9450e73 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset.go +++ b/pkg/controller/elasticsearch/nodespec/statefulset.go @@ -5,8 +5,6 @@ package nodespec import ( - "fmt" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1beta1" @@ -18,7 +16,6 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/settings" 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" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -26,6 +23,10 @@ import ( "k8s.io/apimachinery/pkg/types" ) +var ( + f = false +) + // HeadlessServiceName returns the name of the headless service for the given StatefulSet. func HeadlessServiceName(ssetName string) string { // just use the sset name @@ -77,22 +78,11 @@ func BuildStatefulSet( ssetLabels[k] = v } - // set the owner reference of all volume claims to the ES resource, - // so PVC get deleted automatically upon Elasticsearch resource deletion - f := false - claims := make([]corev1.PersistentVolumeClaim, 0, len(nodeSet.VolumeClaimTemplates)) - for _, claim := range nodeSet.VolumeClaimTemplates { - if err := controllerutil.SetControllerReference(&es, &claim, scheme); err != nil { - return appsv1.StatefulSet{}, err - } - for _, ref := range claim.OwnerReferences { - ref.BlockOwnerDeletion = &f - } - claims = append(claims, claim) + claims, err := setVolumeClaimsControllerReference(nodeSet.VolumeClaimTemplates, es, scheme) + if err != nil { + return appsv1.StatefulSet{}, err } - fmt.Printf("%v", claims) - sset := appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{ Namespace: es.Namespace, @@ -126,6 +116,29 @@ func BuildStatefulSet( return sset, nil } +func setVolumeClaimsControllerReference( + persistentVolumeClaims []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 { + if err := controllerutil.SetControllerReference(&es, &claim, scheme); err != nil { + return nil, err + } + // Set block owner deletion to false as the statefulset controller might not be able to do that + // See https://github.com/elastic/cloud-on-k8s/issues/1884 + refs := claim.OwnerReferences + for i := range refs { + refs[i].BlockOwnerDeletion = &f + } + claims = append(claims, claim) + } + return claims, nil +} + // UpdateReplicas updates the given StatefulSet with the given replicas, // and modifies the template hash label accordingly. func UpdateReplicas(statefulSet *appsv1.StatefulSet, replicas *int32) { From 5e2b6a34b9c31ae9ddc14d2c442edf921af0cb48 Mon Sep 17 00:00:00 2001 From: Michael Morello Date: Mon, 7 Oct 2019 09:04:34 +0200 Subject: [PATCH 3/4] Add unit test --- pkg/controller/elasticsearch/nodespec/resources_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/controller/elasticsearch/nodespec/resources_test.go b/pkg/controller/elasticsearch/nodespec/resources_test.go index 9208059b158..6c012be765c 100644 --- a/pkg/controller/elasticsearch/nodespec/resources_test.go +++ b/pkg/controller/elasticsearch/nodespec/resources_test.go @@ -70,6 +70,15 @@ func TestSetVolumeClaimsControllerReference(t *testing.T) { }{ { 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"}}, From d7e52a4cd90e6691a05ddbdb647b5212b6c98abd Mon Sep 17 00:00:00 2001 From: Michael Morello Date: Mon, 7 Oct 2019 10:31:22 +0200 Subject: [PATCH 4/4] Update comments --- pkg/controller/elasticsearch/nodespec/statefulset.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/controller/elasticsearch/nodespec/statefulset.go b/pkg/controller/elasticsearch/nodespec/statefulset.go index a92d9450e73..99630417596 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset.go +++ b/pkg/controller/elasticsearch/nodespec/statefulset.go @@ -128,7 +128,8 @@ func setVolumeClaimsControllerReference( if err := controllerutil.SetControllerReference(&es, &claim, scheme); err != nil { return nil, err } - // Set block owner deletion to false as the statefulset controller might not be able to do that + // 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 refs := claim.OwnerReferences for i := range refs {