From 000e05ab75a0a10cb5aaf6b1af00f12f347b0888 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:10:31 +0200 Subject: [PATCH 01/25] Remove the PVC validation webhook We'll reintroduce it later, in a form that allows modifying PVC storage reqs. --- pkg/apis/elasticsearch/v1/validations.go | 35 --- pkg/apis/elasticsearch/v1/validations_test.go | 262 ------------------ 2 files changed, 297 deletions(-) diff --git a/pkg/apis/elasticsearch/v1/validations.go b/pkg/apis/elasticsearch/v1/validations.go index 263ae126ba..9e530672dd 100644 --- a/pkg/apis/elasticsearch/v1/validations.go +++ b/pkg/apis/elasticsearch/v1/validations.go @@ -9,7 +9,6 @@ import ( "net" "strings" - apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/util/validation/field" commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" @@ -29,7 +28,6 @@ const ( nodeRolesInOldVersionMsg = "node.roles setting is not available in this version of Elasticsearch" parseStoredVersionErrMsg = "Cannot parse current Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" parseVersionErrMsg = "Cannot parse Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" - pvcImmutableMsg = "Volume claim templates cannot be modified" unsupportedConfigErrMsg = "Configuration setting is reserved for internal use. User-configured use is unsupported" unsupportedUpgradeMsg = "Unsupported version upgrade path. Check the Elasticsearch documentation for supported upgrade paths." unsupportedVersionMsg = "Unsupported version" @@ -52,7 +50,6 @@ type updateValidation func(*Elasticsearch, *Elasticsearch) field.ErrorList var updateValidations = []updateValidation{ noDowngrades, validUpgradePath, - pvcModification, } func (es *Elasticsearch) check(validations []validation) field.ErrorList { @@ -207,29 +204,6 @@ func checkNodeSetNameUniqueness(es *Elasticsearch) field.ErrorList { return errs } -// pvcModification ensures no PVCs are changed, as volume claim templates are immutable in stateful sets -func pvcModification(current, proposed *Elasticsearch) field.ErrorList { - var errs field.ErrorList - if current == nil || proposed == nil { - return errs - } - for i, node := range proposed.Spec.NodeSets { - currNode := getNode(node.Name, current) - if currNode == nil { - // this is a new sset, so there is nothing to check - continue - } - - // ssets do not allow modifications to fields other than 'replicas', 'template', and 'updateStrategy' - // reflection isn't ideal, but okay here since the ES object does not have the status of the claims. - // Checking semantic equality here allows providing PVC storage size with different units (eg. 1Ti vs. 1024Gi). - if !apiequality.Semantic.DeepEqual(node.VolumeClaimTemplates, currNode.VolumeClaimTemplates) { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), node.VolumeClaimTemplates, pvcImmutableMsg)) - } - } - return errs -} - func noDowngrades(current, proposed *Elasticsearch) field.ErrorList { var errs field.ErrorList if current == nil || proposed == nil { @@ -283,12 +257,3 @@ func validUpgradePath(current, proposed *Elasticsearch) field.ErrorList { } return errs } - -func getNode(name string, es *Elasticsearch) *NodeSet { - for i := range es.Spec.NodeSets { - if es.Spec.NodeSets[i].Name == name { - return &es.Spec.NodeSets[i] - } - } - return nil -} diff --git a/pkg/apis/elasticsearch/v1/validations_test.go b/pkg/apis/elasticsearch/v1/validations_test.go index 71ad2d5173..5d075885be 100644 --- a/pkg/apis/elasticsearch/v1/validations_test.go +++ b/pkg/apis/elasticsearch/v1/validations_test.go @@ -9,7 +9,6 @@ import ( commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -316,239 +315,6 @@ func Test_validSanIP(t *testing.T) { } } -func Test_pvcModified(t *testing.T) { - current := getEsCluster() - - tests := []struct { - name string - current *Elasticsearch - proposed *Elasticsearch - expectErrors bool - }{ - { - name: "resize fails", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("10Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: true, - }, - { - name: "same size with different unit accepted", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5120Mi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: false, - }, - { - name: "same size accepted", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: false, - }, - - { - name: "additional PVC fails", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data1", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: true, - }, - - { - name: "name change rejected", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data1", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: true, - }, - - { - name: "add new node set accepted", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - }, - }, - { - Name: "ingest", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("10Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, - expectErrors: false, - }, - - { - name: "new instance accepted", - current: nil, - proposed: current, - expectErrors: false, - }, - } - - for _, tt := range tests { - actual := pvcModification(tt.current, tt.proposed) - actualErrors := len(actual) > 0 - if tt.expectErrors != actualErrors { - t.Errorf("failed pvcModification(). Name: %v, actual %v, wanted: %v, value: %v", tt.name, actual, tt.expectErrors, tt.proposed) - } - } -} - func TestValidation_noDowngrades(t *testing.T) { tests := []struct { name string @@ -702,31 +468,3 @@ func es(v string) *Elasticsearch { Spec: ElasticsearchSpec{Version: v}, } } - -// // getEsCluster returns a ES cluster test fixture -func getEsCluster() *Elasticsearch { - return &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5Gi"), - }, - }, - }, - }, - }, - }, - }, - }, - } -} From 41e750f8bce3da4eaa1883aa983f19db4f17d0a1 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:11:27 +0200 Subject: [PATCH 02/25] RBAC access to read storage classes --- .../manifest-gen/assets/charts/eck/templates/_helpers.tpl | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl b/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl index a62d4256ec..50a5b2d095 100644 --- a/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl +++ b/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl @@ -94,6 +94,14 @@ RBAC permissions - update - patch - delete +- apiGroups: + - storage.k8s.io + resources: + - storageclasses + verbs: + - get + - list + - watch - apiGroups: - apps resources: From 2a28b6b0a1cca801ac954275d5daf5511873be19 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:14:27 +0200 Subject: [PATCH 03/25] Move GetClaim to a common place for reuse --- .../elasticsearch/nodespec/statefulset.go | 12 +---- .../nodespec/statefulset_test.go | 38 --------------- pkg/controller/elasticsearch/sset/getter.go | 11 +++++ .../elasticsearch/sset/getter_test.go | 46 +++++++++++++++++++ 4 files changed, 58 insertions(+), 49 deletions(-) create mode 100644 pkg/controller/elasticsearch/sset/getter_test.go diff --git a/pkg/controller/elasticsearch/nodespec/statefulset.go b/pkg/controller/elasticsearch/nodespec/statefulset.go index ddec278bc1..06cae65860 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset.go +++ b/pkg/controller/elasticsearch/nodespec/statefulset.go @@ -142,7 +142,7 @@ func setVolumeClaimsControllerReference( // so PVC get deleted automatically upon Elasticsearch resource deletion claims := make([]corev1.PersistentVolumeClaim, 0, len(persistentVolumeClaims)) for _, claim := range persistentVolumeClaims { - if existingClaim := getClaimMatchingName(existingClaims, claim.Name); existingClaim != nil { + if existingClaim := sset.GetClaim(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. @@ -179,16 +179,6 @@ 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 index 45dc6f5bea..0a9e47ece9 100644 --- a/pkg/controller/elasticsearch/nodespec/statefulset_test.go +++ b/pkg/controller/elasticsearch/nodespec/statefulset_test.go @@ -5,7 +5,6 @@ package nodespec import ( - "reflect" "testing" "github.com/stretchr/testify/require" @@ -184,40 +183,3 @@ func Test_setVolumeClaimsControllerReference(t *testing.T) { }) } } - -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) - } - }) - } -} diff --git a/pkg/controller/elasticsearch/sset/getter.go b/pkg/controller/elasticsearch/sset/getter.go index 4de69235a8..06e73b320f 100644 --- a/pkg/controller/elasticsearch/sset/getter.go +++ b/pkg/controller/elasticsearch/sset/getter.go @@ -8,6 +8,7 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/common/version" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/label" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" ) // GetReplicas returns the replicas configured for this StatefulSet, or 0 if nil. @@ -22,3 +23,13 @@ func GetReplicas(statefulSet appsv1.StatefulSet) int32 { func GetESVersion(statefulSet appsv1.StatefulSet) (*version.Version, error) { return label.ExtractVersion(statefulSet.Spec.Template.Labels) } + +// GetClaim returns a pointer to the claim with the given name, or nil if not found. +func GetClaim(claims []corev1.PersistentVolumeClaim, claimName string) *corev1.PersistentVolumeClaim { + for i, claim := range claims { + if claim.Name == claimName { + return &claims[i] + } + } + return nil +} diff --git a/pkg/controller/elasticsearch/sset/getter_test.go b/pkg/controller/elasticsearch/sset/getter_test.go new file mode 100644 index 0000000000..af62268fdb --- /dev/null +++ b/pkg/controller/elasticsearch/sset/getter_test.go @@ -0,0 +1,46 @@ +package sset + +import ( + "reflect" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestGetClaim(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 := GetClaim(tt.claims, tt.claimName); !reflect.DeepEqual(got, tt.want) { + t.Errorf("GetClaim() = %v, want %v", got, tt.want) + } + }) + } +} From 160790352147fa15247927b1c6a6a3b7eece99ac Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:31:59 +0200 Subject: [PATCH 04/25] Rename pvc.go to pvc_gc.go --- pkg/controller/elasticsearch/driver/{pvc.go => pvc_gc.go} | 5 ++--- .../elasticsearch/driver/{pvc_test.go => pvc_gc_test.go} | 0 2 files changed, 2 insertions(+), 3 deletions(-) rename pkg/controller/elasticsearch/driver/{pvc.go => pvc_gc.go} (99%) rename pkg/controller/elasticsearch/driver/{pvc_test.go => pvc_gc_test.go} (100%) diff --git a/pkg/controller/elasticsearch/driver/pvc.go b/pkg/controller/elasticsearch/driver/pvc_gc.go similarity index 99% rename from pkg/controller/elasticsearch/driver/pvc.go rename to pkg/controller/elasticsearch/driver/pvc_gc.go index 2e7304a34b..ab17bd2f39 100644 --- a/pkg/controller/elasticsearch/driver/pvc.go +++ b/pkg/controller/elasticsearch/driver/pvc_gc.go @@ -5,14 +5,13 @@ package driver import ( - corev1 "k8s.io/api/core/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/label" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" "github.com/elastic/cloud-on-k8s/pkg/utils/stringsutil" + corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // GarbageCollectPVCs ensures PersistentVolumeClaims created for the given es resource are deleted diff --git a/pkg/controller/elasticsearch/driver/pvc_test.go b/pkg/controller/elasticsearch/driver/pvc_gc_test.go similarity index 100% rename from pkg/controller/elasticsearch/driver/pvc_test.go rename to pkg/controller/elasticsearch/driver/pvc_gc_test.go From 20e51e1cfd217b5586708fe7f73ba83fe2ec9a47 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:34:56 +0200 Subject: [PATCH 05/25] Resize PVCs whose storage req has changed --- .../elasticsearch/driver/pvc_expansion.go | 133 +++++ .../driver/pvc_expansion_test.go | 455 ++++++++++++++++++ pkg/controller/elasticsearch/sset/getter.go | 30 ++ .../elasticsearch/sset/getter_test.go | 88 ++++ 4 files changed, 706 insertions(+) create mode 100644 pkg/controller/elasticsearch/driver/pvc_expansion.go create mode 100644 pkg/controller/elasticsearch/driver/pvc_expansion_test.go diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go new file mode 100644 index 0000000000..96899969f9 --- /dev/null +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -0,0 +1,133 @@ +package driver + +import ( + "errors" + "fmt" + + "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) +// resizePVCs updates the spec of all existing PVCs whose storage requests can be expanded, +// according to their storage class and what's specified in the expected claim. +// It returns an error if the requested storage size is incompatible with the PVC. +func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) error { + // match each existing PVC with an expected claim, and decide whether the PVC should be resized + actualPVCs, err := sset.RetrieveActualPVCs(k8sClient, actualSset) + if err != nil { + return err + } + for claimName, pvcs := range actualPVCs { + expectedClaim := sset.GetClaim(expectedSset.Spec.VolumeClaimTemplates, claimName) + if expectedClaim == nil { + continue + } + for _, pvc := range pvcs { + pvcSize := pvc.Spec.Resources.Requests.Storage() + claimSize := expectedClaim.Spec.Resources.Requests.Storage() + // is it a storage increase? + isExpansion, err := isStorageExpansion(claimSize, pvcSize) + if err != nil { + return err + } + if !isExpansion { + continue + } + // does the storage class allow volume expansion? + if err := ensureClaimSupportsExpansion(k8sClient, *expectedClaim); err != nil { + return err + } + + log.Info("Resizing PVC storage requests. Depending on the volume provisioner, "+ + "Pods may need to be manually deleted for the filesystem to be resized.", + "namespace", pvc.Namespace, "pvc_name", pvc.Name, + "old_value", pvcSize.String(), "new_value", claimSize.String()) + pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *claimSize + if err := k8sClient.Update(&pvc); err != nil { + return err + } + } + } + return nil +} + +// ensureClaimSupportsExpansion inspects whether the storage class referenced by the claim +// allows volume expansion. +func ensureClaimSupportsExpansion(k8sClient k8s.Client, claim corev1.PersistentVolumeClaim) error { + sc, err := getStorageClass(k8sClient, claim) + if err != nil { + return err + } + if !allowsVolumeExpansion(sc) { + return fmt.Errorf("claim %s does not support volume expansion", claim.Name) + } + return nil +} + +// isStorageExpansion returns true if actual is higher than expected. +// Decreasing storage size is unsupported: an error is returned if expected < actual. +func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Quantity) (bool, error) { + if expectedSize == nil || actualSize == nil { + // not much to compare if storage size is unspecified + return false, nil + } + switch expectedSize.Cmp(*actualSize) { + case 0: // same size + return false, nil + case -1: // decrease + return false, fmt.Errorf("storage size cannot be decreased from %s to %s", actualSize.String(), expectedSize.String()) + default: // increase + return true, nil + } +} + +// getStorageClass returns the storage class specified by the given claim, +// or the default storage class if the claim does not specify any. +func getStorageClass(k8sClient k8s.Client, claim corev1.PersistentVolumeClaim) (storagev1.StorageClass, error) { + if claim.Spec.StorageClassName == nil || *claim.Spec.StorageClassName == "" { + return getDefaultStorageClass(k8sClient) + } + var sc storagev1.StorageClass + if err := k8sClient.Get(types.NamespacedName{Name: *claim.Spec.StorageClassName}, &sc); err != nil { + return storagev1.StorageClass{}, fmt.Errorf("cannot retrieve storage class: %w", err) + } + return sc, nil +} + +// getDefaultStorageClass returns the default storage class in the current k8s cluster, +// or an error if there is none. +func getDefaultStorageClass(k8sClient k8s.Client) (storagev1.StorageClass, error) { + var scs storagev1.StorageClassList + if err := k8sClient.List(&scs); err != nil { + return storagev1.StorageClass{}, err + } + for _, sc := range scs.Items { + if isDefaultStorageClass(sc) { + return sc, nil + } + } + return storagev1.StorageClass{}, errors.New("no default storage class found") +} + +// isDefaultStorageClass inspects the given storage class and returns true if it is annotated as the default one. +func isDefaultStorageClass(sc storagev1.StorageClass) bool { + if len(sc.Annotations) == 0 { + return false + } + if sc.Annotations["storageclass.kubernetes.io/is-default-class"] == "true" || + sc.Annotations["storageclass.beta.kubernetes.io/is-default-class"] == "true" { + return true + } + return false +} + +// allowsVolumeExpansion returns true if the given storage class allows volume expansion. +func allowsVolumeExpansion(sc storagev1.StorageClass) bool { + return sc.AllowVolumeExpansion != nil && *sc.AllowVolumeExpansion +} diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go new file mode 100644 index 0000000000..2d9d092a61 --- /dev/null +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -0,0 +1,455 @@ +package driver + +import ( + "fmt" + "reflect" + "testing" + + "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" +) + +var ( + defaultStorageClass = storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default-sc", + Annotations: map[string]string{"storageclass.kubernetes.io/is-default-class": "true"}}} + defaultBetaStorageClass = storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "default-beta-sc", + Annotations: map[string]string{"storageclass.beta.kubernetes.io/is-default-class": "true"}}} + sampleStorageClass = storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ + Name: "sample-sc"}} + + sampleClaim = corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "sample-claim"}, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.StringPtr(sampleStorageClass.Name), + Resources: corev1.ResourceRequirements{Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }}}} + sampleClaim2 = corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "sample-claim-2"}, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: pointer.StringPtr(sampleStorageClass.Name), + Resources: corev1.ResourceRequirements{Requests: map[corev1.ResourceName]resource.Quantity{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }}}} + + sampleSset = appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sample-sset"}} +) + +func withVolumeExpansion(sc storagev1.StorageClass) *storagev1.StorageClass { + sc.AllowVolumeExpansion = pointer.BoolPtr(true) + return &sc +} + +func withClaims(sset appsv1.StatefulSet, claims ...corev1.PersistentVolumeClaim) appsv1.StatefulSet { + s := sset.DeepCopy() + s.Spec.VolumeClaimTemplates = append(s.Spec.VolumeClaimTemplates, claims...) + return *s +} + +func withStorageReq(claim corev1.PersistentVolumeClaim, size string) corev1.PersistentVolumeClaim { + c := claim.DeepCopy() + c.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(size) + return *c +} + +func Test_resizePVCs(t *testing.T) { + sset := appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sample-sset"}, + Spec: appsv1.StatefulSetSpec{ + Replicas: pointer.Int32Ptr(3), + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{sampleClaim}, + }, + } + resizedSset := *sset.DeepCopy() + resizedSset.Spec.VolumeClaimTemplates[0].Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("3Gi") + pvcsWithSize := func(size ...string) []corev1.PersistentVolumeClaim { + var pvcs []corev1.PersistentVolumeClaim + for i, s := range size { + pvcs = append(pvcs, withStorageReq(corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: fmt.Sprintf("sample-claim-sample-sset-%d", i)}, + Spec: sampleClaim.Spec, + }, s)) + } + return pvcs + } + pvcPtrs := func(pvcs []corev1.PersistentVolumeClaim) []runtime.Object { + var ptrs []runtime.Object + for i := range pvcs { + ptrs = append(ptrs, &pvcs[i]) + } + return ptrs + } + + type args struct { + expectedSset appsv1.StatefulSet + actualSset appsv1.StatefulSet + } + tests := []struct { + name string + args args + runtimeObjs []runtime.Object + expectedPVCs []corev1.PersistentVolumeClaim + wantErr bool + }{ + { + name: "no pvc to resize", + args: args{ + expectedSset: sset, + actualSset: sset, + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), + expectedPVCs: pvcsWithSize("1Gi", "1Gi", "1Gi"), + }, + { + name: "all pvcs should be resized", + args: args{ + expectedSset: resizedSset, + actualSset: sset, + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), + expectedPVCs: pvcsWithSize("3Gi", "3Gi", "3Gi"), + }, + { + name: "2 pvcs left to resize", + args: args{ + expectedSset: resizedSset, + actualSset: sset, + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("3Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), + expectedPVCs: pvcsWithSize("3Gi", "3Gi", "3Gi"), + }, + { + name: "one pvc is missing: resize what's there, don't error out", + args: args{ + expectedSset: resizedSset, + actualSset: sset, + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("3Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), + expectedPVCs: pvcsWithSize("3Gi", "3Gi"), + }, + { + name: "storage decrease is not supported: error out", + args: args{ + expectedSset: sset, // 1Gi + actualSset: resizedSset, // 3Gi + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("3Gi", "3Gi")), withVolumeExpansion(sampleStorageClass)), + expectedPVCs: pvcsWithSize("3Gi", "3Gi"), + wantErr: true, + }, + { + name: "storage class does not support volume expansion: error out", + args: args{ + expectedSset: resizedSset, + actualSset: sset, + }, + runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), &sampleStorageClass), + expectedPVCs: pvcsWithSize("1Gi", "1Gi", "1Gi"), + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k8sClient := k8s.WrappedFakeClient(tt.runtimeObjs...) + if err := resizePVCs(k8sClient, tt.args.expectedSset, tt.args.actualSset); (err != nil) != tt.wantErr { + t.Errorf("resizePVCs() error = %v, wantErr %v", err, tt.wantErr) + } + + // all expected PVCs should exist in the apiserver + var pvcs corev1.PersistentVolumeClaimList + err := k8sClient.List(&pvcs) + require.NoError(t, err) + require.Len(t, pvcs.Items, len(tt.expectedPVCs)) + for i, expectedPVC := range tt.expectedPVCs { + comparison.RequireEqual(t, &expectedPVC, &pvcs.Items[i]) + } + }) + } +} + +func Test_isStorageExpansion(t *testing.T) { + type args struct { + expectedSize *resource.Quantity + actualSize *resource.Quantity + } + tests := []struct { + name string + args args + want bool + wantErr bool + }{ + { + name: "expected == actual: false", + args: args{ + expectedSize: resource.NewQuantity(1, resource.DecimalSI), + actualSize: resource.NewQuantity(1, resource.DecimalSI), + }, + want: false, + }, + { + name: "expected > actual: true", + args: args{ + expectedSize: resource.NewQuantity(2, resource.DecimalSI), + actualSize: resource.NewQuantity(1, resource.DecimalSI), + }, + want: true, + }, + { + name: "expected < actual: error out", + args: args{ + expectedSize: resource.NewQuantity(1, resource.DecimalSI), + actualSize: resource.NewQuantity(2, resource.DecimalSI), + }, + want: false, + wantErr: true, + }, + { + name: "expected is nil", + args: args{ + expectedSize: nil, + actualSize: resource.NewQuantity(1, resource.DecimalSI), + }, + want: false, + }, + { + name: "actual is nil", + args: args{ + expectedSize: resource.NewQuantity(1, resource.DecimalSI), + actualSize: nil, + }, + want: false, + }, + { + name: "expected and actual are nil", + args: args{ + expectedSize: nil, + actualSize: nil, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := isStorageExpansion(tt.args.expectedSize, tt.args.actualSize) + if (err != nil) != tt.wantErr { + t.Errorf("isStorageExpansion() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("isStorageExpansion() got = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_ensureClaimSupportsExpansion(t *testing.T) { + tests := []struct { + name string + k8sClient k8s.Client + claim corev1.PersistentVolumeClaim + wantErr bool + }{ + { + name: "specified storage class supports volume expansion", + k8sClient: k8s.WrappedFakeClient(withVolumeExpansion(sampleStorageClass)), + claim: sampleClaim, + wantErr: false, + }, + { + name: "specified storage class does not support volume expansion", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), + claim: sampleClaim, + wantErr: true, + }, + { + name: "default storage class supports volume expansion", + k8sClient: k8s.WrappedFakeClient(withVolumeExpansion(defaultStorageClass)), + claim: corev1.PersistentVolumeClaim{}, + wantErr: false, + }, + { + name: "default storage class does not support volume expansion", + k8sClient: k8s.WrappedFakeClient(&defaultStorageClass), + claim: corev1.PersistentVolumeClaim{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := ensureClaimSupportsExpansion(tt.k8sClient, tt.claim); (err != nil) != tt.wantErr { + t.Errorf("ensureClaimSupportsExpansion() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func Test_allowsVolumeExpansion(t *testing.T) { + tests := []struct { + name string + sc storagev1.StorageClass + want bool + }{ + { + name: "allow volume expansion: true", + sc: storagev1.StorageClass{AllowVolumeExpansion: pointer.BoolPtr(true)}, + want: true, + }, + { + name: "allow volume expansion: false", + sc: storagev1.StorageClass{AllowVolumeExpansion: pointer.BoolPtr(false)}, + want: false, + }, + { + name: "allow volume expansion: nil", + sc: storagev1.StorageClass{AllowVolumeExpansion: nil}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := allowsVolumeExpansion(tt.sc); got != tt.want { + t.Errorf("allowsVolumeExpansion() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_isDefaultStorageClass(t *testing.T) { + tests := []struct { + name string + sc storagev1.StorageClass + want bool + }{ + { + name: "annotated as default", + sc: defaultStorageClass, + want: true, + }, + { + name: "annotated as default (beta)", + sc: defaultBetaStorageClass, + want: true, + }, + { + name: "annotated as default (+ beta)", + sc: storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + "storageclass.kubernetes.io/is-default-class": "true", + "storageclass.beta.kubernetes.io/is-default-class": "true", + }}}, + want: true, + }, + { + name: "no annotations", + sc: storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Annotations: nil}}, + want: false, + }, + { + name: "not annotated as default", + sc: sampleStorageClass, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isDefaultStorageClass(tt.sc); got != tt.want { + t.Errorf("isDefaultStorageClass() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_getDefaultStorageClass(t *testing.T) { + tests := []struct { + name string + k8sClient k8s.Client + want storagev1.StorageClass + wantErr bool + }{ + { + name: "return the default storage class", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), + want: defaultStorageClass, + }, + { + name: "default storage class not found", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), + want: storagev1.StorageClass{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := getDefaultStorageClass(tt.k8sClient) + if (err != nil) != tt.wantErr { + t.Errorf("getDefaultStorageClass() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("getDefaultStorageClass() got = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_getStorageClass(t *testing.T) { + tests := []struct { + name string + k8sClient k8s.Client + claim corev1.PersistentVolumeClaim + want storagev1.StorageClass + wantErr bool + }{ + { + name: "return the specified storage class", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), + claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{StorageClassName: pointer.StringPtr(sampleStorageClass.Name)}}, + want: sampleStorageClass, + wantErr: false, + }, + { + name: "error out if not found", + k8sClient: k8s.WrappedFakeClient(&defaultStorageClass), + claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{StorageClassName: pointer.StringPtr(sampleStorageClass.Name)}}, + want: storagev1.StorageClass{}, + wantErr: true, + }, + { + name: "fallback to the default storage class if unspecified", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), + claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{}}, + want: defaultStorageClass, + wantErr: false, + }, + { + name: "error out if unspecified and default storage class not found", + k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), + claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{}}, + want: storagev1.StorageClass{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := getStorageClass(tt.k8sClient, tt.claim) + if (err != nil) != tt.wantErr { + t.Errorf("getStorageClass() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !comparison.Equal(&got, &tt.want) { + t.Errorf("getStorageClass() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/controller/elasticsearch/sset/getter.go b/pkg/controller/elasticsearch/sset/getter.go index 06e73b320f..f8fdec18ea 100644 --- a/pkg/controller/elasticsearch/sset/getter.go +++ b/pkg/controller/elasticsearch/sset/getter.go @@ -5,10 +5,15 @@ package sset import ( + "fmt" + "github.com/elastic/cloud-on-k8s/pkg/controller/common/version" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/label" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" ) // GetReplicas returns the replicas configured for this StatefulSet, or 0 if nil. @@ -33,3 +38,28 @@ func GetClaim(claims []corev1.PersistentVolumeClaim, claimName string) *corev1.P } return nil } + +// RetrieveActualPVCs returns all existing PVCs for that StatefulSet, per claim name. +func RetrieveActualPVCs(k8sClient k8s.Client, statefulSet appsv1.StatefulSet) (map[string][]corev1.PersistentVolumeClaim, error) { + pvcs := make(map[string][]corev1.PersistentVolumeClaim) + for _, podName := range PodNames(statefulSet) { + for _, claim := range statefulSet.Spec.VolumeClaimTemplates { + if claim.Name == "" { + continue + } + pvcName := fmt.Sprintf("%s-%s", claim.Name, podName) + var pvc corev1.PersistentVolumeClaim + if err := k8sClient.Get(types.NamespacedName{Namespace: statefulSet.Namespace, Name: pvcName}, &pvc); err != nil { + if apierrors.IsNotFound(err) { + continue // PVC does not exist (yet) + } + return nil, err + } + if _, exists := pvcs[claim.Name]; !exists { + pvcs[claim.Name] = make([]corev1.PersistentVolumeClaim, 0) + } + pvcs[claim.Name] = append(pvcs[claim.Name], pvc) + } + } + return pvcs, nil +} diff --git a/pkg/controller/elasticsearch/sset/getter_test.go b/pkg/controller/elasticsearch/sset/getter_test.go index af62268fdb..19b7d111b3 100644 --- a/pkg/controller/elasticsearch/sset/getter_test.go +++ b/pkg/controller/elasticsearch/sset/getter_test.go @@ -4,8 +4,14 @@ import ( "reflect" "testing" + "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" ) func TestGetClaim(t *testing.T) { @@ -44,3 +50,85 @@ func TestGetClaim(t *testing.T) { }) } } + +func TestRetrieveActualPVCs(t *testing.T) { + // 3 replicas, 2 PVCs each + sset := appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset"}, + Spec: appsv1.StatefulSetSpec{ + Replicas: pointer.Int32Ptr(3), + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + {ObjectMeta: metav1.ObjectMeta{Name: "claim1"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "claim2"}}, + }, + }, + } + pvcs := []corev1.PersistentVolumeClaim{ + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-0"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-0"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-1"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-1"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-2"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-2"}}, + } + expected := map[string][]corev1.PersistentVolumeClaim{ + "claim1": { + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-0"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-1"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-2"}}, + }, + "claim2": { + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-0"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-1"}}, + {ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim2-sset-2"}}, + }, + } + asRuntimeObjs := func(pvcs []corev1.PersistentVolumeClaim) []runtime.Object { + objs := make([]runtime.Object, 0, len(pvcs)) + for i := range pvcs { + objs = append(objs, &pvcs[i]) + } + return objs + } + + tests := []struct { + name string + k8sClient k8s.Client + statefulSet appsv1.StatefulSet + want map[string][]corev1.PersistentVolumeClaim + }{ + { + name: "return expected PVCs for the StatefulSet", + k8sClient: k8s.WrappedFakeClient(asRuntimeObjs(pvcs)...), + statefulSet: sset, + want: expected, + }, + { + name: "some PVCs are missing: return what can be returned", + k8sClient: k8s.WrappedFakeClient(&corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-0"}}), + statefulSet: sset, + want: map[string][]corev1.PersistentVolumeClaim{"claim1": {{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-0"}}}}, + }, + { + name: "extra PVCs exist but are not expected: don't return them", + k8sClient: k8s.WrappedFakeClient(asRuntimeObjs(append(pvcs, corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "claim1-sset-3"}}))...), + statefulSet: sset, + want: expected, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := RetrieveActualPVCs(tt.k8sClient, tt.statefulSet) + require.NoError(t, err) + require.Equal(t, len(got), len(tt.want)) + for claim, pvcs := range tt.want { + gotPVCs, exists := got[claim] + require.True(t, exists) + require.Equal(t, len(pvcs), len(gotPVCs)) + for i := range pvcs { + comparison.RequireEqual(t, &gotPVCs[i], &pvcs[i]) + } + } + }) + } +} From 9b24749d59b0c20cf9abb764269bf1246b61adc6 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:35:49 +0200 Subject: [PATCH 06/25] Delete StatefulSet for recreation on volume expansion --- .../elasticsearch/driver/pvc_expansion.go | 54 ++++++++++++ .../driver/pvc_expansion_test.go | 83 +++++++++++++++++++ 2 files changed, 137 insertions(+) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index 96899969f9..a490d28a30 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -57,6 +57,60 @@ func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSse return nil } +// deleteSsetForClaimResize compares expected vs. actual StatefulSets, and deletes the actual one +// if a volume expansion can be performed. Pods remain orphan until the StatefulSet is created again. +func deleteSsetForClaimResize(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { + shouldRecreate, err := needsRecreate(k8sClient, expectedSset, actualSset) + if err != nil { + return false, err + } + if !shouldRecreate { + return false, nil + } + + log.Info("Deleting StatefulSet to account for resized PVCs, it will be recreated automatically", + "namespace", actualSset.Namespace, "statefulset_name", actualSset.Name) + + opts := client.DeleteOptions{} + // ensure Pods are not also deleted + orphanPolicy := metav1.DeletePropagationOrphan + opts.PropagationPolicy = &orphanPolicy + // ensure we are not deleting based on out-of-date sset spec + opts.Preconditions = &metav1.Preconditions{ + UID: &actualSset.UID, + ResourceVersion: &actualSset.ResourceVersion, + } + return true, k8sClient.Delete(&actualSset, &opts) +} + +// needsRecreate returns true if the StatefulSet needs to be re-created to account for volume expansion. +// An error is returned if volume expansion is required but claims are incompatible. +func needsRecreate(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { + recreate := false + // match each expected claim with an actual existing one: we want to return true + // if at least one claim has increased storage reqs + // however we want to error-out if any claim has an incompatible storage req + for _, expectedClaim := range expectedSset.Spec.VolumeClaimTemplates { + actualClaim := sset.GetClaim(actualSset.Spec.VolumeClaimTemplates, expectedClaim.Name) + if actualClaim == nil { + continue + } + isExpansion, err := isStorageExpansion(expectedClaim.Spec.Resources.Requests.Storage(), actualClaim.Spec.Resources.Requests.Storage()) + if err != nil { + return false, err + } + if !isExpansion { + continue + } + if err := ensureClaimSupportsExpansion(k8sClient, *actualClaim); err != nil { + return false, err + } + recreate = true + } + + return recreate, nil +} + // ensureClaimSupportsExpansion inspects whether the storage class referenced by the claim // allows volume expansion. func ensureClaimSupportsExpansion(k8sClient k8s.Client, claim corev1.PersistentVolumeClaim) error { diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 2d9d092a61..16d1ea0bbd 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -179,6 +179,89 @@ func Test_resizePVCs(t *testing.T) { } } +func Test_deleteSsetForClaimResize(t *testing.T) { + type args struct { + k8sClient k8s.Client + expectedSset appsv1.StatefulSet + actualSset appsv1.StatefulSet + } + tests := []struct { + name string + args args + want bool + wantErr bool + }{ + { + name: "requested storage increase in the 2nd claim: recreate", + args: args{ + k8sClient: k8s.WrappedFakeClient(&sampleSset, withVolumeExpansion(sampleStorageClass)), + expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "3Gi")), + actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), + }, + want: true, + }, + { + name: "requested storage increase in the 2nd claim, but storage class does not allow it: error out", + args: args{ + k8sClient: k8s.WrappedFakeClient(&sampleSset, &sampleStorageClass), + expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "3Gi")), + actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), + }, + want: false, + wantErr: true, + }, + { + name: "no claim in the StatefulSet", + args: args{ + k8sClient: k8s.WrappedFakeClient(&sampleSset), + expectedSset: sampleSset, + actualSset: sampleSset, + }, + want: false, + }, + { + name: "no change in the claim", + args: args{ + k8sClient: k8s.WrappedFakeClient(&sampleSset), + expectedSset: withClaims(sampleSset, sampleClaim), + actualSset: withClaims(sampleSset, sampleClaim), + }, + want: false, + }, + { + name: "requested storage decrease: error out", + args: args{ + k8sClient: k8s.WrappedFakeClient(&sampleSset, withVolumeExpansion(sampleStorageClass)), + expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "0.5Gi")), + actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), + }, + want: false, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + deleted, err := deleteSsetForClaimResize(tt.args.k8sClient, tt.args.expectedSset, tt.args.actualSset) + if (err != nil) != tt.wantErr { + t.Errorf("deleteSsetForClaimResize() error = %v, wantErr %v", err, tt.wantErr) + return + } + if deleted != tt.want { + t.Errorf("deleteSsetForClaimResize() got = %v, want %v", deleted, tt.want) + } + + // double-check if the sset is indeed deleted + var retrieved appsv1.StatefulSet + err = tt.args.k8sClient.Get(k8s.ExtractNamespacedName(&tt.args.actualSset), &retrieved) + if deleted { + require.True(t, apierrors.IsNotFound(err)) + } else { + require.NoError(t, err) + } + }) + } +} + func Test_isStorageExpansion(t *testing.T) { type args struct { expectedSize *resource.Quantity From 6be1279142c1c266df63ae5c550a3a05cf591335 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:38:56 +0200 Subject: [PATCH 07/25] Pass the replicas count instead of the entire sset to LimitMasterNodesCreation --- .../elasticsearch/driver/upscale.go | 2 +- .../elasticsearch/driver/upscale_state.go | 8 +- .../driver/upscale_state_test.go | 134 +++++++++--------- 3 files changed, 71 insertions(+), 73 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index a4bdde53f8..73547d4ef3 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -113,7 +113,7 @@ func adjustStatefulSetReplicas( actualReplicas := sset.GetReplicas(actual) if actualReplicas < expectedReplicas { - return upscaleState.limitNodesCreation(actual, expected) + return upscaleState.limitNodesCreation(actualReplicas, expected) } if alreadyExists && expectedReplicas < actualReplicas { diff --git a/pkg/controller/elasticsearch/driver/upscale_state.go b/pkg/controller/elasticsearch/driver/upscale_state.go index 4406705b09..0b25c7f31c 100644 --- a/pkg/controller/elasticsearch/driver/upscale_state.go +++ b/pkg/controller/elasticsearch/driver/upscale_state.go @@ -157,7 +157,7 @@ func (s *upscaleState) getMaxNodesToCreate(noMoreThan int32) int32 { // limitNodesCreation decreases replica count in specs as needed, assumes an upscale is requested func (s *upscaleState) limitNodesCreation( - actual appsv1.StatefulSet, + actualReplicas int32, toApply appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { if err := buildOnce(s); err != nil { @@ -165,10 +165,9 @@ func (s *upscaleState) limitNodesCreation( } if label.IsMasterNodeSet(toApply) { - return s.limitMasterNodesCreation(actual, toApply) + return s.limitMasterNodesCreation(actualReplicas, toApply) } - actualReplicas := sset.GetReplicas(actual) targetReplicas := sset.GetReplicas(toApply) nodespec.UpdateReplicas(&toApply, pointer.Int32(actualReplicas)) @@ -195,10 +194,9 @@ func (s *upscaleState) limitNodesCreation( } func (s *upscaleState) limitMasterNodesCreation( - actual appsv1.StatefulSet, + actualReplicas int32, toApply appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { - actualReplicas := sset.GetReplicas(actual) targetReplicas := sset.GetReplicas(toApply) nodespec.UpdateReplicas(&toApply, pointer.Int32(actualReplicas)) diff --git a/pkg/controller/elasticsearch/driver/upscale_state_test.go b/pkg/controller/elasticsearch/driver/upscale_state_test.go index 1e45400765..7807373aa5 100644 --- a/pkg/controller/elasticsearch/driver/upscale_state_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_state_test.go @@ -22,97 +22,97 @@ import ( func Test_upscaleState_limitNodesCreation(t *testing.T) { tests := []struct { - name string - state *upscaleState - actual appsv1.StatefulSet - ssetToApply appsv1.StatefulSet - wantSset appsv1.StatefulSet - wantState *upscaleState + name string + state *upscaleState + actualReplicas int32 + ssetToApply appsv1.StatefulSet + wantSset appsv1.StatefulSet + wantState *upscaleState }{ { - name: "no change on the sset spec", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, - actual: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + name: "no change on the sset spec", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + actualReplicas: 3, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, }, { - name: "spec change (same replicas)", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, - actual: sset.TestSset{Name: "sset", Version: "6.8.0", Replicas: 3, Master: true}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + name: "spec change (same replicas)", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + actualReplicas: 3, + ssetToApply: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, }, { - name: "upscale data nodes from 1 to 3: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, - actual: sset.TestSset{Name: "sset", Replicas: 1, Master: false}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale data nodes from 1 to 3: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, + actualReplicas: 1, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale data nodes from 1 to 4: should limit to 3", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, - actual: sset.TestSset{Name: "sset", Replicas: 1, Master: false}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale data nodes from 1 to 4: should limit to 3", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, + actualReplicas: 1, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale master nodes from 1 to 3: should limit to 2", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, - actual: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 2, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, + name: "upscale master nodes from 1 to 3: should limit to 2", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, + actualReplicas: 1, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 2, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, }, { - name: "upscale master nodes from 1 to 3 when cluster not yet bootstrapped: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2)}, - actual: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale master nodes from 1 to 3 when cluster not yet bootstrapped: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2)}, + actualReplicas: 1, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale masters from 3 to 4, but no creates allowed: should limit to 0", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, - actual: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, + name: "upscale masters from 3 to 4, but no creates allowed: should limit to 0", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, + actualReplicas: 3, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, }, { - name: "upscale data nodes from 3 to 4, but no creates allowed: should limit to 0", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, - actual: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, + name: "upscale data nodes from 3 to 4, but no creates allowed: should limit to 0", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, + actualReplicas: 3, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, }, { - name: "new StatefulSet with 5 master nodes, cluster isn't bootstrapped yet: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3)}, - actual: appsv1.StatefulSet{}, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3), recordedCreates: 3}, + name: "new StatefulSet with 5 master nodes, cluster isn't bootstrapped yet: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3)}, + actualReplicas: 0, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3), recordedCreates: 3}, }, { - name: "new StatefulSet with 5 master nodes, cluster already bootstrapped: should limit to 1", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, - actual: appsv1.StatefulSet{}, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, + name: "new StatefulSet with 5 master nodes, cluster already bootstrapped: should limit to 1", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, + actualReplicas: 0, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotSset, err := tt.state.limitNodesCreation(tt.actual, tt.ssetToApply) + gotSset, err := tt.state.limitNodesCreation(tt.actualReplicas, tt.ssetToApply) require.NoError(t, err) // StatefulSet should be adapted require.Equal(t, tt.wantSset, gotSset) From 613e650f33bcd2423d9854bf366d869060845173 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:40:08 +0200 Subject: [PATCH 08/25] Take into account temporarily deleted ssets when adjusting replicas --- .../elasticsearch/driver/upscale.go | 27 ++++++++-- .../elasticsearch/driver/upscale_test.go | 51 ++++++++++++++++++- 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index 73547d4ef3..9840fa4766 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -75,7 +75,7 @@ func adjustResources( upscaleState := newUpscaleState(ctx, actualStatefulSets, expectedResources) adjustedResources := make(nodespec.ResourcesList, 0, len(expectedResources)) for _, nodeSpecRes := range expectedResources { - adjusted, err := adjustStatefulSetReplicas(upscaleState, actualStatefulSets, *nodeSpecRes.StatefulSet.DeepCopy()) + adjusted, err := adjustStatefulSetReplicas(ctx.k8sClient, upscaleState, actualStatefulSets, *nodeSpecRes.StatefulSet.DeepCopy()) if err != nil { return nil, err } @@ -104,22 +104,39 @@ func adjustZenConfig(k8sClient k8s.Client, es esv1.Elasticsearch, resources node // adjustStatefulSetReplicas updates the replicas count in expected according to // what is allowed by the upscaleState, that may be mutated as a result. func adjustStatefulSetReplicas( + k8sClient k8s.Client, upscaleState *upscaleState, actualStatefulSets sset.StatefulSetList, expected appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { - actual, alreadyExists := actualStatefulSets.GetByName(expected.Name) expectedReplicas := sset.GetReplicas(expected) - actualReplicas := sset.GetReplicas(actual) + + actual, alreadyExists := actualStatefulSets.GetByName(expected.Name) + actualReplicas := sset.GetReplicas(actual) // 0 if actual is empty + if !alreadyExists { + // In most cases, if the StatefulSet does not exist we're creating it for the first time (actualReplicas = 0). + // However if some Pods matching that missing StatefulSet exist we are likely in a situation where the + // StatefulSet is being re-created for volume expansion. In which case we want to take into account + // the number of existing Pods as replica count. + pods, err := sset.GetActualPodsForStatefulSet(k8sClient, k8s.ExtractNamespacedName(&expected)) + if err != nil { + return appsv1.StatefulSet{}, err + } + actualReplicas = int32(len(pods)) + if actualReplicas > 0 { + log.Info("Adjusting StatefulSet replicas based on orphan Pods from volume expansion", + "namespace", expected.Namespace, "statefulset_name", expected.Name, "replicas", actualReplicas) + } + } if actualReplicas < expectedReplicas { return upscaleState.limitNodesCreation(actualReplicas, expected) } - if alreadyExists && expectedReplicas < actualReplicas { + if expectedReplicas < actualReplicas { // this is a downscale. // We still want to update the sset spec to the newest one, but leave scaling down as it's done later. - nodespec.UpdateReplicas(&expected, actual.Spec.Replicas) + nodespec.UpdateReplicas(&expected, &actualReplicas) } return expected, nil diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index 773519e408..1ca5e56191 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -205,6 +205,7 @@ func Test_isReplicaIncrease(t *testing.T) { func Test_adjustStatefulSetReplicas(t *testing.T) { type args struct { + k8sClient k8s.Client state *upscaleState actualStatefulSets sset.StatefulSetList expected appsv1.StatefulSet @@ -218,6 +219,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "new StatefulSet to create", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{}, expected: sset.TestSset{Name: "new-sset", Replicas: 3}.Build(), @@ -228,6 +230,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "same StatefulSet already exists", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 3}.Build(), @@ -238,6 +241,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "downscale case", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 1}.Build(), @@ -248,6 +252,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: data nodes", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: false, Data: true}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 5, Master: false, Data: true}.Build(), @@ -258,6 +263,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: master nodes - one by one", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: true, Data: true}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 5, Master: true, Data: true}.Build(), @@ -268,6 +274,7 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: new additional master sset - one by one", args: args{ + k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: true, Data: true}.Build()}, expected: sset.TestSset{Name: "sset-2", Replicas: 3, Master: true, Data: true}.Build(), @@ -275,10 +282,52 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { want: sset.TestSset{Name: "sset-2", Replicas: 1, Master: true, Data: true}.Build(), wantUpscaleState: &upscaleState{recordedCreates: 1, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, }, + { + name: "new data-nodes StatefulSet to create, but some Pods already exist: volume expansion case", + args: args{ + // 2 Pods exist out of the 4 replicas + k8sClient: k8s.WrappedFakeClient( + // the match between sset and Pods is based on StatefulSetNameLabelName + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-0", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-1", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + ), + state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, + actualStatefulSets: sset.StatefulSetList{}, + expected: sset.TestSset{Name: "new-sset", Replicas: 4}.Build(), + }, + // allow 2 (pre-existing) + 2 (new creation) replicas + want: sset.TestSset{Name: "new-sset", Replicas: 4}.Build(), + wantUpscaleState: &upscaleState{recordedCreates: 2, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, + }, + { + name: "new master-nodes StatefulSet to create, but some Pods already exist: volume expansion case", + args: args{ + // 2 Pods exist out of the 4 replicas + k8sClient: k8s.WrappedFakeClient( + // the match between sset and Pods is based on StatefulSetNameLabelName + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-0", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-1", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + ), + state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, + actualStatefulSets: sset.StatefulSetList{}, + expected: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), + }, + // allow 2 (pre-existing) + 1 (new master, one by one), out of 4 + want: sset.TestSset{Name: "new-sset", Master: true, Replicas: 3}.Build(), + wantUpscaleState: &upscaleState{recordedCreates: 1, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := adjustStatefulSetReplicas(tt.args.state, tt.args.actualStatefulSets, tt.args.expected) + got, err := adjustStatefulSetReplicas(tt.args.k8sClient, tt.args.state, tt.args.actualStatefulSets, tt.args.expected) require.NoError(t, err) require.Nil(t, deep.Equal(got, tt.want)) require.Equal(t, tt.wantUpscaleState, tt.args.state) From 607136b11258398e73a94b822719e13c6155985e Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:41:07 +0200 Subject: [PATCH 09/25] Plug the volume expansion logic to the upscale phase --- pkg/controller/elasticsearch/driver/nodes.go | 6 +- .../elasticsearch/driver/pvc_expansion.go | 16 ++ .../elasticsearch/driver/upscale.go | 32 +++- .../elasticsearch/driver/upscale_test.go | 148 ++++++++++++++++-- 4 files changed, 178 insertions(+), 24 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/nodes.go b/pkg/controller/elasticsearch/driver/nodes.go index 9a8bcd9856..9a68c6791f 100644 --- a/pkg/controller/elasticsearch/driver/nodes.go +++ b/pkg/controller/elasticsearch/driver/nodes.go @@ -74,7 +74,7 @@ func (d *defaultDriver) reconcileNodeSpecs( esState: esState, expectations: d.Expectations, } - actualStatefulSets, err = HandleUpscaleAndSpecChanges(upscaleCtx, actualStatefulSets, expectedResources) + upscaleResults, err := HandleUpscaleAndSpecChanges(upscaleCtx, actualStatefulSets, expectedResources) if err != nil { reconcileState.AddEvent(corev1.EventTypeWarning, events.EventReconciliationError, fmt.Sprintf("Failed to apply spec change: %v", err)) var podTemplateErr *sset.PodTemplateError @@ -84,6 +84,10 @@ func (d *defaultDriver) reconcileNodeSpecs( } return results.WithError(err) } + if upscaleResults.Requeue { + return results.WithResult(defaultRequeue) + } + actualStatefulSets = upscaleResults.ActualStatefulSets // Update PDB to account for new replicas. if err := pdb.Reconcile(d.Client, d.ES, actualStatefulSets); err != nil { diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index a490d28a30..e58e559ecc 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -14,6 +14,22 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) + +// handleVolumeExpansion works around the immutability of VolumeClaimTemplates in StatefulSets by: +// 1. updating storage requests in PVCs whose storage class supports volume expansion +// 2. deleting the StatefulSet, to be recreated with the new storage spec +// It returns a boolean indicating whether the StatefulSet was deleted. +// Note that some storage drivers also require Pods to be deleted/recreated for the filesystem to be resized +// (as opposed to a hot resize while the Pod is running). This is left to the responsibility of the user. +// This should be handled differently once supported by the StatefulSet controller: https://github.com/kubernetes/kubernetes/issues/68737. +func handleVolumeExpansion(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { + err := resizePVCs(k8sClient, expectedSset, actualSset) + if err != nil { + return false, err + } + return deleteSsetForClaimResize(k8sClient, expectedSset, actualSset) +} + // resizePVCs updates the spec of all existing PVCs whose storage requests can be expanded, // according to their storage class and what's specified in the expected claim. // It returns an error if the requested storage size is incompatible with the PVC. diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index 9840fa4766..a069f81c6c 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -30,12 +30,18 @@ type upscaleCtx struct { expectations *expectations.Expectations } +type UpscaleResults struct { + ActualStatefulSets sset.StatefulSetList + Requeue bool +} + // HandleUpscaleAndSpecChanges reconciles expected NodeSet resources. // It does: // - create any new StatefulSets // - update existing StatefulSets specification, to be used for future pods rotation // - upscale StatefulSet for which we expect more replicas // - limit master node creation to one at a time +// - resize (inline) existing PVCs to match new StatefulSet storage reqs, & delete the StatefulSet for recreation // It does not: // - perform any StatefulSet downscale (left for downscale phase) // - perform any pod upgrade (left for rolling upgrade phase) @@ -43,28 +49,42 @@ func HandleUpscaleAndSpecChanges( ctx upscaleCtx, actualStatefulSets sset.StatefulSetList, expectedResources nodespec.ResourcesList, -) (sset.StatefulSetList, error) { +) (UpscaleResults, error) { + results := UpscaleResults{} // adjust expected replicas to control nodes creation and deletion adjusted, err := adjustResources(ctx, actualStatefulSets, expectedResources) if err != nil { - return nil, fmt.Errorf("adjust resources: %w", err) + return results, fmt.Errorf("adjust resources: %w", err) } // reconcile all resources for _, res := range adjusted { if err := settings.ReconcileConfig(ctx.k8sClient, ctx.es, res.StatefulSet.Name, res.Config); err != nil { - return nil, fmt.Errorf("reconcile config: %w", err) + return results, fmt.Errorf("reconcile config: %w", err) } if _, err := common.ReconcileService(ctx.parentCtx, ctx.k8sClient, &res.HeadlessService, &ctx.es); err != nil { - return nil, fmt.Errorf("reconcile service: %w", err) + return results, fmt.Errorf("reconcile service: %w", err) + } + if actualSset, exists := actualStatefulSets.GetByName(res.StatefulSet.Name); exists { + ssetDeleted, err := handleVolumeExpansion(ctx.k8sClient, res.StatefulSet, actualSset) + if err != nil { + return results, fmt.Errorf("handle volume expansion: %w", err) + } + if ssetDeleted { + // the StatefulSet was just deleted: it is safer to requeue at the end of this function + // and let it be re-created at the next reconciliation + results.Requeue = true + continue + } } reconciled, err := sset.ReconcileStatefulSet(ctx.k8sClient, ctx.es, res.StatefulSet, ctx.expectations) if err != nil { - return nil, fmt.Errorf("reconcile sset %w", err) + return results, fmt.Errorf("reconcile StatefulSet: %w", err) } // update actual with the reconciled ones for next steps to work with up-to-date information actualStatefulSets = actualStatefulSets.WithStatefulSet(reconciled) } - return actualStatefulSets, nil + results.ActualStatefulSets = actualStatefulSets + return results, nil } func adjustResources( diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index 1ca5e56191..84821774f0 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -9,14 +9,6 @@ import ( "sync" "testing" - "github.com/go-test/deep" - "github.com/stretchr/testify/require" - appsv1 "k8s.io/api/apps/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" "github.com/elastic/cloud-on-k8s/pkg/controller/common/expectations" @@ -28,6 +20,16 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" "github.com/elastic/cloud-on-k8s/pkg/utils/pointer" + "github.com/go-test/deep" + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ) var onceDone = &sync.Once{} @@ -112,17 +114,17 @@ func TestHandleUpscaleAndSpecChanges(t *testing.T) { // when no StatefulSets already exists actualStatefulSets := sset.StatefulSetList{} - updatedStatefulSets, err := HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + res, err := HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) require.NoError(t, err) // StatefulSets should be created with their expected replicas var sset1 appsv1.StatefulSet require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: "sset1"}, &sset1)) require.Equal(t, pointer.Int32(3), sset1.Spec.Replicas) - comparison.RequireEqual(t, &updatedStatefulSets[0], &sset1) + comparison.RequireEqual(t, &res.ActualStatefulSets[0], &sset1) var sset2 appsv1.StatefulSet require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: "sset2"}, &sset2)) require.Equal(t, pointer.Int32(4), sset2.Spec.Replicas) - comparison.RequireEqual(t, &updatedStatefulSets[1], &sset2) + comparison.RequireEqual(t, &res.ActualStatefulSets[1], &sset2) // headless services should be created for both require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: nodespec.HeadlessServiceName("sset1")}, &corev1.Service{})) require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: nodespec.HeadlessServiceName("sset2")}, &corev1.Service{})) @@ -137,35 +139,147 @@ func TestHandleUpscaleAndSpecChanges(t *testing.T) { require.NoError(t, k8sClient.Get(k8s.ExtractNamespacedName(&es.ObjectMeta), &es)) // update context with current version of Elasticsearch resource ctx.es = es - updatedStatefulSets, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + res, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) require.NoError(t, err) require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: "sset2"}, &sset2)) require.Equal(t, pointer.Int32(10), sset2.Spec.Replicas) - comparison.RequireEqual(t, &updatedStatefulSets[1], &sset2) + comparison.RequireEqual(t, &res.ActualStatefulSets[1], &sset2) // expectations should have been set require.NotEmpty(t, ctx.expectations.GetGenerations()) // apply a spec change actualStatefulSets = sset.StatefulSetList{sset1, sset2} expectedResources[1].StatefulSet.Spec.Template.Labels = map[string]string{"a": "b"} - updatedStatefulSets, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + res, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) require.NoError(t, err) require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: "sset2"}, &sset2)) require.Equal(t, "b", sset2.Spec.Template.Labels["a"]) - comparison.RequireEqual(t, &updatedStatefulSets[1], &sset2) + comparison.RequireEqual(t, &res.ActualStatefulSets[1], &sset2) // apply a spec change and a downscale from 10 to 2 actualStatefulSets = sset.StatefulSetList{sset1, sset2} expectedResources[1].StatefulSet.Spec.Replicas = pointer.Int32(2) expectedResources[1].StatefulSet.Spec.Template.Labels = map[string]string{"a": "c"} - updatedStatefulSets, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + res, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) require.NoError(t, err) + require.False(t, res.Requeue) + require.Len(t, res.ActualStatefulSets, 2) require.NoError(t, k8sClient.Get(types.NamespacedName{Namespace: "ns", Name: "sset2"}, &sset2)) // spec should be updated require.Equal(t, "c", sset2.Spec.Template.Labels["a"]) // but StatefulSet should not be downscaled require.Equal(t, pointer.Int32(10), sset2.Spec.Replicas) - comparison.RequireEqual(t, &updatedStatefulSets[1], &sset2) + comparison.RequireEqual(t, &res.ActualStatefulSets[1], &sset2) +} + +func TestHandleUpscaleAndSpecChanges_PVCResize(t *testing.T) { + // focus on the special case of handling PVC resize + es := esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es"}, + Spec: esv1.ElasticsearchSpec{Version: "7.5.0"}, + } + + truePtr := true + storageClass := storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{Name: "resizeable"}, + AllowVolumeExpansion: &truePtr, + } + + // 3 masters, 4 data x 1Gi storage + actualStatefulSets := []appsv1.StatefulSet{ + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sset1", + }, + Spec: appsv1.StatefulSetSpec{ + Replicas: pointer.Int32(3), + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + string(label.NodeTypesMasterLabelName): "true", + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sset2", + }, + Spec: appsv1.StatefulSetSpec{ + Replicas: pointer.Int32(4), + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + {ObjectMeta: metav1.ObjectMeta{Name: "elasticsearch-data"}, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Gi"), + }, + }, + StorageClassName: &storageClass.Name, + }, + }, + }, + }, + }, + } + // expected: same 2 StatefulSets, but the 2nd one has its storage resized to 3Gi + dataResized := *actualStatefulSets[1].DeepCopy() + dataResized.Spec.VolumeClaimTemplates[0].Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("3Gi") + expectedResources := nodespec.ResourcesList{ + { + StatefulSet: actualStatefulSets[0], + HeadlessService: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sset1", + }, + }, + Config: settings.CanonicalConfig{}, + }, + { + StatefulSet: dataResized, + HeadlessService: corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sset2", + }, + }, + Config: settings.CanonicalConfig{}, + }, + } + + k8sClient := k8s.WrappedFakeClient(&es, &storageClass, &actualStatefulSets[0], &actualStatefulSets[1]) + ctx := upscaleCtx{ + k8sClient: k8sClient, + es: es, + esState: nil, + expectations: expectations.NewExpectations(k8sClient), + parentCtx: context.Background(), + } + + // 2nd StatefulSet should be deleted, we should requeue + res, err := HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + require.NoError(t, err) + require.True(t, res.Requeue) + var retrievedSset appsv1.StatefulSet + err = k8sClient.Get(k8s.ExtractNamespacedName(&dataResized), &retrievedSset) + require.True(t, apierrors.IsNotFound(err)) + + // re-fetch es to simulate actual reconciliation behaviour + require.NoError(t, k8sClient.Get(k8s.ExtractNamespacedName(&es.ObjectMeta), &es)) + ctx.es = es + // the 2nd sset does not exist anymore + actualStatefulSets = []appsv1.StatefulSet{actualStatefulSets[0]} + + // next run: the missing StatefulSet should be recreated + res, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) + require.NoError(t, err) + require.False(t, res.Requeue) + require.Len(t, res.ActualStatefulSets, 2) + require.Equal(t, resource.MustParse("3Gi"), *res.ActualStatefulSets[1].Spec.VolumeClaimTemplates[0].Spec.Resources.Requests.Storage()) } func Test_isReplicaIncrease(t *testing.T) { From cbe0a9706876c5f3f222e4af8ed4a78edbea45f0 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:41:26 +0200 Subject: [PATCH 10/25] Remove deprecated rolling upgrade partition strategy from upscale tests --- pkg/controller/elasticsearch/driver/upscale_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index 84821774f0..c61185273b 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -60,12 +60,6 @@ func TestHandleUpscaleAndSpecChanges(t *testing.T) { }, Spec: appsv1.StatefulSetSpec{ Replicas: pointer.Int32(3), - UpdateStrategy: appsv1.StatefulSetUpdateStrategy{ - Type: appsv1.RollingUpdateStatefulSetStrategyType, - RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{ - Partition: pointer.Int32(3), - }, - }, Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ @@ -91,12 +85,6 @@ func TestHandleUpscaleAndSpecChanges(t *testing.T) { }, Spec: appsv1.StatefulSetSpec{ Replicas: pointer.Int32(4), - UpdateStrategy: appsv1.StatefulSetUpdateStrategy{ - Type: appsv1.RollingUpdateStatefulSetStrategyType, - RollingUpdate: &appsv1.RollingUpdateStatefulSetStrategy{ - Partition: pointer.Int32(4), - }, - }, }, }, HeadlessService: corev1.Service{ From 8080ad7518ba0322951a1a41ac62dcc42a199154 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 10:42:28 +0200 Subject: [PATCH 11/25] Garbage collect PVCs after having dealt with temporarily deleted ssets --- pkg/controller/elasticsearch/driver/nodes.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/nodes.go b/pkg/controller/elasticsearch/driver/nodes.go index 9a68c6791f..14d1b6c5d9 100644 --- a/pkg/controller/elasticsearch/driver/nodes.go +++ b/pkg/controller/elasticsearch/driver/nodes.go @@ -59,10 +59,6 @@ func (d *defaultDriver) reconcileNodeSpecs( return results.WithError(err) } - if err := GarbageCollectPVCs(d.K8sClient(), d.ES, actualStatefulSets, expectedResources.StatefulSets()); err != nil { - return results.WithError(err) - } - esState := NewMemoizingESState(ctx, esClient) // Phase 1: apply expected StatefulSets resources and scale up. @@ -94,6 +90,10 @@ func (d *defaultDriver) reconcileNodeSpecs( return results.WithError(err) } + if err := GarbageCollectPVCs(d.K8sClient(), d.ES, actualStatefulSets, expectedResources.StatefulSets()); err != nil { + return results.WithError(err) + } + // Phase 2: if there is any Pending or bootlooping Pod to upgrade, do it. attempted, err := d.MaybeForceUpgrade(actualStatefulSets) if err != nil || attempted { From 8872518eefc5216d4313e351052209104d3d47bc Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 12:03:58 +0200 Subject: [PATCH 12/25] Use the highest existing ordinal instead of the pods count ...when setting the initial replicas values for statefulset re-creation. Otherwise, if we're unlucky and one Pod is missing for some reasons (eg. pod-0), using the Pod count could lead to removing the Pods with highest ordinals. --- .../elasticsearch/driver/upscale.go | 15 +++++++++++-- .../elasticsearch/driver/upscale_test.go | 21 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index a069f81c6c..e0bf741c16 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -137,12 +137,23 @@ func adjustStatefulSetReplicas( // In most cases, if the StatefulSet does not exist we're creating it for the first time (actualReplicas = 0). // However if some Pods matching that missing StatefulSet exist we are likely in a situation where the // StatefulSet is being re-created for volume expansion. In which case we want to take into account - // the number of existing Pods as replica count. + // the existing Pods when setting the initial replicas value. pods, err := sset.GetActualPodsForStatefulSet(k8sClient, k8s.ExtractNamespacedName(&expected)) if err != nil { return appsv1.StatefulSet{}, err } - actualReplicas = int32(len(pods)) + // The new StatefulSet replicas count should match the highest ordinal of Pods ready to be adopted. + actualReplicas = int32(0) + for _, pod := range pods { + _, ordinal, err := sset.StatefulSetName(pod.Name) + if err != nil { + return appsv1.StatefulSet{}, err + } + ordinalBasedReplicas := ordinal + 1 // 3 replicas -> pod-0, pod-1, pod-2 + if ordinalBasedReplicas > actualReplicas { + actualReplicas = ordinalBasedReplicas + } + } if actualReplicas > 0 { log.Info("Adjusting StatefulSet replicas based on orphan Pods from volume expansion", "namespace", expected.Namespace, "statefulset_name", expected.Name, "replicas", actualReplicas) diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index c61185273b..e429b2c260 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -426,6 +426,27 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { want: sset.TestSset{Name: "new-sset", Master: true, Replicas: 3}.Build(), wantUpscaleState: &upscaleState{recordedCreates: 1, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, }, + { + name: "new master-nodes StatefulSet to create, some Pods already exist (volume expansion case), but first ones are missing", + args: args{ + // Pods 2 and 3 exist, but not 0 and 1 (which would normally just be recreated by the sset controller) + k8sClient: k8s.WrappedFakeClient( + // the match between sset and Pods is based on StatefulSetNameLabelName + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-2", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-3", Labels: map[string]string{ + label.StatefulSetNameLabelName: "new-sset", + }}}, + ), + state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, + actualStatefulSets: sset.StatefulSetList{}, + expected: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), + }, + // allow the 4 masters that should be there (out of 4) + want: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), + wantUpscaleState: &upscaleState{recordedCreates: 0, isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From f48939651029651ec6d105f392bf30d3c85b9cba Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 12:14:24 +0200 Subject: [PATCH 13/25] Add missing license headers --- pkg/controller/elasticsearch/driver/pvc_expansion.go | 4 ++++ pkg/controller/elasticsearch/driver/pvc_expansion_test.go | 4 ++++ pkg/controller/elasticsearch/sset/getter_test.go | 4 ++++ 3 files changed, 12 insertions(+) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index e58e559ecc..e6d8a76471 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -1,3 +1,7 @@ +// 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 driver import ( diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 16d1ea0bbd..90a31346b6 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -1,3 +1,7 @@ +// 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 driver import ( diff --git a/pkg/controller/elasticsearch/sset/getter_test.go b/pkg/controller/elasticsearch/sset/getter_test.go index 19b7d111b3..635ebd2cb2 100644 --- a/pkg/controller/elasticsearch/sset/getter_test.go +++ b/pkg/controller/elasticsearch/sset/getter_test.go @@ -1,3 +1,7 @@ +// 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 sset import ( From e4fe5807006c350d920dc3622a5dbeb3e1dbd002 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 15 Sep 2020 15:47:02 +0200 Subject: [PATCH 14/25] Improvements from PR review --- pkg/controller/elasticsearch/driver/pvc_expansion.go | 3 ++- pkg/controller/elasticsearch/driver/upscale.go | 8 +++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index e6d8a76471..f05a7a25a0 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -155,7 +155,8 @@ func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Qu case 0: // same size return false, nil case -1: // decrease - return false, fmt.Errorf("storage size cannot be decreased from %s to %s", actualSize.String(), expectedSize.String()) + return false, fmt.Errorf("decreasing storage size is not supported, "+ + "but an attempt was made to resize from %s to %s", actualSize.String(), expectedSize.String()) default: // increase return true, nil } diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index e0bf741c16..bc3b395365 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -41,7 +41,7 @@ type UpscaleResults struct { // - update existing StatefulSets specification, to be used for future pods rotation // - upscale StatefulSet for which we expect more replicas // - limit master node creation to one at a time -// - resize (inline) existing PVCs to match new StatefulSet storage reqs, & delete the StatefulSet for recreation +// - resize (inline) existing PVCs to match new StatefulSet storage reqs and delete the StatefulSet for recreation // It does not: // - perform any StatefulSet downscale (left for downscale phase) // - perform any pod upgrade (left for rolling upgrade phase) @@ -70,8 +70,10 @@ func HandleUpscaleAndSpecChanges( return results, fmt.Errorf("handle volume expansion: %w", err) } if ssetDeleted { - // the StatefulSet was just deleted: it is safer to requeue at the end of this function - // and let it be re-created at the next reconciliation + // The StatefulSet was just deleted: it is safer to requeue at the end of this function + // and let it be re-created at the next reconciliation. + // Otherwise, downscales and rolling upgrades could be performed with wrong assumptions: + // the sset isn't reported in actualStatefulSets (was just deleted), but the Pods actually exist. results.Requeue = true continue } From 08c54d181c2ce4082937403bd867e9c5d91a1957 Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 17 Sep 2020 18:20:29 +0200 Subject: [PATCH 15/25] Revert "RBAC access to read storage classes" This reverts commit 41e750f8bce3da4eaa1883aa983f19db4f17d0a1. --- .../manifest-gen/assets/charts/eck/templates/_helpers.tpl | 8 -------- 1 file changed, 8 deletions(-) diff --git a/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl b/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl index 50a5b2d095..a62d4256ec 100644 --- a/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl +++ b/hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl @@ -94,14 +94,6 @@ RBAC permissions - update - patch - delete -- apiGroups: - - storage.k8s.io - resources: - - storageclasses - verbs: - - get - - list - - watch - apiGroups: - apps resources: From aaf0b58c807a0f5f3a05175f7eb571897bf4359c Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 17 Sep 2020 18:26:37 +0200 Subject: [PATCH 16/25] Remove storage class checks for rbac concerns --- .../elasticsearch/driver/pvc_expansion.go | 77 +----- .../driver/pvc_expansion_test.go | 228 ------------------ 2 files changed, 4 insertions(+), 301 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index f05a7a25a0..aa0c3214ec 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -5,17 +5,14 @@ package driver import ( - "errors" "fmt" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" - storagev1 "k8s.io/api/storage/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -59,10 +56,6 @@ func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSse if !isExpansion { continue } - // does the storage class allow volume expansion? - if err := ensureClaimSupportsExpansion(k8sClient, *expectedClaim); err != nil { - return err - } log.Info("Resizing PVC storage requests. Depending on the volume provisioner, "+ "Pods may need to be manually deleted for the filesystem to be resized.", @@ -80,7 +73,7 @@ func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSse // deleteSsetForClaimResize compares expected vs. actual StatefulSets, and deletes the actual one // if a volume expansion can be performed. Pods remain orphan until the StatefulSet is created again. func deleteSsetForClaimResize(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { - shouldRecreate, err := needsRecreate(k8sClient, expectedSset, actualSset) + shouldRecreate, err := needsRecreate(expectedSset, actualSset) if err != nil { return false, err } @@ -105,7 +98,7 @@ func deleteSsetForClaimResize(k8sClient k8s.Client, expectedSset appsv1.Stateful // needsRecreate returns true if the StatefulSet needs to be re-created to account for volume expansion. // An error is returned if volume expansion is required but claims are incompatible. -func needsRecreate(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { +func needsRecreate(expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { recreate := false // match each expected claim with an actual existing one: we want to return true // if at least one claim has increased storage reqs @@ -119,31 +112,14 @@ func needsRecreate(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actual if err != nil { return false, err } - if !isExpansion { - continue + if isExpansion { + recreate = true } - if err := ensureClaimSupportsExpansion(k8sClient, *actualClaim); err != nil { - return false, err - } - recreate = true } return recreate, nil } -// ensureClaimSupportsExpansion inspects whether the storage class referenced by the claim -// allows volume expansion. -func ensureClaimSupportsExpansion(k8sClient k8s.Client, claim corev1.PersistentVolumeClaim) error { - sc, err := getStorageClass(k8sClient, claim) - if err != nil { - return err - } - if !allowsVolumeExpansion(sc) { - return fmt.Errorf("claim %s does not support volume expansion", claim.Name) - } - return nil -} - // isStorageExpansion returns true if actual is higher than expected. // Decreasing storage size is unsupported: an error is returned if expected < actual. func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Quantity) (bool, error) { @@ -161,48 +137,3 @@ func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Qu return true, nil } } - -// getStorageClass returns the storage class specified by the given claim, -// or the default storage class if the claim does not specify any. -func getStorageClass(k8sClient k8s.Client, claim corev1.PersistentVolumeClaim) (storagev1.StorageClass, error) { - if claim.Spec.StorageClassName == nil || *claim.Spec.StorageClassName == "" { - return getDefaultStorageClass(k8sClient) - } - var sc storagev1.StorageClass - if err := k8sClient.Get(types.NamespacedName{Name: *claim.Spec.StorageClassName}, &sc); err != nil { - return storagev1.StorageClass{}, fmt.Errorf("cannot retrieve storage class: %w", err) - } - return sc, nil -} - -// getDefaultStorageClass returns the default storage class in the current k8s cluster, -// or an error if there is none. -func getDefaultStorageClass(k8sClient k8s.Client) (storagev1.StorageClass, error) { - var scs storagev1.StorageClassList - if err := k8sClient.List(&scs); err != nil { - return storagev1.StorageClass{}, err - } - for _, sc := range scs.Items { - if isDefaultStorageClass(sc) { - return sc, nil - } - } - return storagev1.StorageClass{}, errors.New("no default storage class found") -} - -// isDefaultStorageClass inspects the given storage class and returns true if it is annotated as the default one. -func isDefaultStorageClass(sc storagev1.StorageClass) bool { - if len(sc.Annotations) == 0 { - return false - } - if sc.Annotations["storageclass.kubernetes.io/is-default-class"] == "true" || - sc.Annotations["storageclass.beta.kubernetes.io/is-default-class"] == "true" { - return true - } - return false -} - -// allowsVolumeExpansion returns true if the given storage class allows volume expansion. -func allowsVolumeExpansion(sc storagev1.StorageClass) bool { - return sc.AllowVolumeExpansion != nil && *sc.AllowVolumeExpansion -} diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 90a31346b6..e2d9d30a4c 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -6,7 +6,6 @@ package driver import ( "fmt" - "reflect" "testing" "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" @@ -23,13 +22,6 @@ import ( ) var ( - defaultStorageClass = storagev1.StorageClass{ - ObjectMeta: metav1.ObjectMeta{ - Name: "default-sc", - Annotations: map[string]string{"storageclass.kubernetes.io/is-default-class": "true"}}} - defaultBetaStorageClass = storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ - Name: "default-beta-sc", - Annotations: map[string]string{"storageclass.beta.kubernetes.io/is-default-class": "true"}}} sampleStorageClass = storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{ Name: "sample-sc"}} @@ -153,16 +145,6 @@ func Test_resizePVCs(t *testing.T) { expectedPVCs: pvcsWithSize("3Gi", "3Gi"), wantErr: true, }, - { - name: "storage class does not support volume expansion: error out", - args: args{ - expectedSset: resizedSset, - actualSset: sset, - }, - runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), &sampleStorageClass), - expectedPVCs: pvcsWithSize("1Gi", "1Gi", "1Gi"), - wantErr: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -204,16 +186,6 @@ func Test_deleteSsetForClaimResize(t *testing.T) { }, want: true, }, - { - name: "requested storage increase in the 2nd claim, but storage class does not allow it: error out", - args: args{ - k8sClient: k8s.WrappedFakeClient(&sampleSset, &sampleStorageClass), - expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "3Gi")), - actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), - }, - want: false, - wantErr: true, - }, { name: "no claim in the StatefulSet", args: args{ @@ -340,203 +312,3 @@ func Test_isStorageExpansion(t *testing.T) { }) } } - -func Test_ensureClaimSupportsExpansion(t *testing.T) { - tests := []struct { - name string - k8sClient k8s.Client - claim corev1.PersistentVolumeClaim - wantErr bool - }{ - { - name: "specified storage class supports volume expansion", - k8sClient: k8s.WrappedFakeClient(withVolumeExpansion(sampleStorageClass)), - claim: sampleClaim, - wantErr: false, - }, - { - name: "specified storage class does not support volume expansion", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), - claim: sampleClaim, - wantErr: true, - }, - { - name: "default storage class supports volume expansion", - k8sClient: k8s.WrappedFakeClient(withVolumeExpansion(defaultStorageClass)), - claim: corev1.PersistentVolumeClaim{}, - wantErr: false, - }, - { - name: "default storage class does not support volume expansion", - k8sClient: k8s.WrappedFakeClient(&defaultStorageClass), - claim: corev1.PersistentVolumeClaim{}, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if err := ensureClaimSupportsExpansion(tt.k8sClient, tt.claim); (err != nil) != tt.wantErr { - t.Errorf("ensureClaimSupportsExpansion() error = %v, wantErr %v", err, tt.wantErr) - } - }) - } -} - -func Test_allowsVolumeExpansion(t *testing.T) { - tests := []struct { - name string - sc storagev1.StorageClass - want bool - }{ - { - name: "allow volume expansion: true", - sc: storagev1.StorageClass{AllowVolumeExpansion: pointer.BoolPtr(true)}, - want: true, - }, - { - name: "allow volume expansion: false", - sc: storagev1.StorageClass{AllowVolumeExpansion: pointer.BoolPtr(false)}, - want: false, - }, - { - name: "allow volume expansion: nil", - sc: storagev1.StorageClass{AllowVolumeExpansion: nil}, - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := allowsVolumeExpansion(tt.sc); got != tt.want { - t.Errorf("allowsVolumeExpansion() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_isDefaultStorageClass(t *testing.T) { - tests := []struct { - name string - sc storagev1.StorageClass - want bool - }{ - { - name: "annotated as default", - sc: defaultStorageClass, - want: true, - }, - { - name: "annotated as default (beta)", - sc: defaultBetaStorageClass, - want: true, - }, - { - name: "annotated as default (+ beta)", - sc: storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ - "storageclass.kubernetes.io/is-default-class": "true", - "storageclass.beta.kubernetes.io/is-default-class": "true", - }}}, - want: true, - }, - { - name: "no annotations", - sc: storagev1.StorageClass{ObjectMeta: metav1.ObjectMeta{Annotations: nil}}, - want: false, - }, - { - name: "not annotated as default", - sc: sampleStorageClass, - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := isDefaultStorageClass(tt.sc); got != tt.want { - t.Errorf("isDefaultStorageClass() = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_getDefaultStorageClass(t *testing.T) { - tests := []struct { - name string - k8sClient k8s.Client - want storagev1.StorageClass - wantErr bool - }{ - { - name: "return the default storage class", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), - want: defaultStorageClass, - }, - { - name: "default storage class not found", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), - want: storagev1.StorageClass{}, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := getDefaultStorageClass(tt.k8sClient) - if (err != nil) != tt.wantErr { - t.Errorf("getDefaultStorageClass() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("getDefaultStorageClass() got = %v, want %v", got, tt.want) - } - }) - } -} - -func Test_getStorageClass(t *testing.T) { - tests := []struct { - name string - k8sClient k8s.Client - claim corev1.PersistentVolumeClaim - want storagev1.StorageClass - wantErr bool - }{ - { - name: "return the specified storage class", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), - claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{StorageClassName: pointer.StringPtr(sampleStorageClass.Name)}}, - want: sampleStorageClass, - wantErr: false, - }, - { - name: "error out if not found", - k8sClient: k8s.WrappedFakeClient(&defaultStorageClass), - claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{StorageClassName: pointer.StringPtr(sampleStorageClass.Name)}}, - want: storagev1.StorageClass{}, - wantErr: true, - }, - { - name: "fallback to the default storage class if unspecified", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass, &defaultStorageClass), - claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{}}, - want: defaultStorageClass, - wantErr: false, - }, - { - name: "error out if unspecified and default storage class not found", - k8sClient: k8s.WrappedFakeClient(&sampleStorageClass), - claim: corev1.PersistentVolumeClaim{Spec: corev1.PersistentVolumeClaimSpec{}}, - want: storagev1.StorageClass{}, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := getStorageClass(tt.k8sClient, tt.claim) - if (err != nil) != tt.wantErr { - t.Errorf("getStorageClass() error = %v, wantErr %v", err, tt.wantErr) - return - } - if !comparison.Equal(&got, &tt.want) { - t.Errorf("getStorageClass() got = %v, want %v", got, tt.want) - } - }) - } -} From 40498401529cca5bf0276819555f824d3e79add9 Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 17 Sep 2020 18:27:34 +0200 Subject: [PATCH 17/25] Revert "Remove the PVC validation webhook" This reverts commit 000e05ab75a0a10cb5aaf6b1af00f12f347b0888. --- pkg/apis/elasticsearch/v1/validations.go | 35 +++ pkg/apis/elasticsearch/v1/validations_test.go | 262 ++++++++++++++++++ 2 files changed, 297 insertions(+) diff --git a/pkg/apis/elasticsearch/v1/validations.go b/pkg/apis/elasticsearch/v1/validations.go index 9e530672dd..263ae126ba 100644 --- a/pkg/apis/elasticsearch/v1/validations.go +++ b/pkg/apis/elasticsearch/v1/validations.go @@ -9,6 +9,7 @@ import ( "net" "strings" + apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/util/validation/field" commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" @@ -28,6 +29,7 @@ const ( nodeRolesInOldVersionMsg = "node.roles setting is not available in this version of Elasticsearch" parseStoredVersionErrMsg = "Cannot parse current Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" parseVersionErrMsg = "Cannot parse Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" + pvcImmutableMsg = "Volume claim templates cannot be modified" unsupportedConfigErrMsg = "Configuration setting is reserved for internal use. User-configured use is unsupported" unsupportedUpgradeMsg = "Unsupported version upgrade path. Check the Elasticsearch documentation for supported upgrade paths." unsupportedVersionMsg = "Unsupported version" @@ -50,6 +52,7 @@ type updateValidation func(*Elasticsearch, *Elasticsearch) field.ErrorList var updateValidations = []updateValidation{ noDowngrades, validUpgradePath, + pvcModification, } func (es *Elasticsearch) check(validations []validation) field.ErrorList { @@ -204,6 +207,29 @@ func checkNodeSetNameUniqueness(es *Elasticsearch) field.ErrorList { return errs } +// pvcModification ensures no PVCs are changed, as volume claim templates are immutable in stateful sets +func pvcModification(current, proposed *Elasticsearch) field.ErrorList { + var errs field.ErrorList + if current == nil || proposed == nil { + return errs + } + for i, node := range proposed.Spec.NodeSets { + currNode := getNode(node.Name, current) + if currNode == nil { + // this is a new sset, so there is nothing to check + continue + } + + // ssets do not allow modifications to fields other than 'replicas', 'template', and 'updateStrategy' + // reflection isn't ideal, but okay here since the ES object does not have the status of the claims. + // Checking semantic equality here allows providing PVC storage size with different units (eg. 1Ti vs. 1024Gi). + if !apiequality.Semantic.DeepEqual(node.VolumeClaimTemplates, currNode.VolumeClaimTemplates) { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), node.VolumeClaimTemplates, pvcImmutableMsg)) + } + } + return errs +} + func noDowngrades(current, proposed *Elasticsearch) field.ErrorList { var errs field.ErrorList if current == nil || proposed == nil { @@ -257,3 +283,12 @@ func validUpgradePath(current, proposed *Elasticsearch) field.ErrorList { } return errs } + +func getNode(name string, es *Elasticsearch) *NodeSet { + for i := range es.Spec.NodeSets { + if es.Spec.NodeSets[i].Name == name { + return &es.Spec.NodeSets[i] + } + } + return nil +} diff --git a/pkg/apis/elasticsearch/v1/validations_test.go b/pkg/apis/elasticsearch/v1/validations_test.go index 5d075885be..71ad2d5173 100644 --- a/pkg/apis/elasticsearch/v1/validations_test.go +++ b/pkg/apis/elasticsearch/v1/validations_test.go @@ -9,6 +9,7 @@ import ( commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -315,6 +316,239 @@ func Test_validSanIP(t *testing.T) { } } +func Test_pvcModified(t *testing.T) { + current := getEsCluster() + + tests := []struct { + name string + current *Elasticsearch + proposed *Elasticsearch + expectErrors bool + }{ + { + name: "resize fails", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("10Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: true, + }, + { + name: "same size with different unit accepted", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5120Mi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: false, + }, + { + name: "same size accepted", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: false, + }, + + { + name: "additional PVC fails", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data1", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: true, + }, + + { + name: "name change rejected", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data1", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: true, + }, + + { + name: "add new node set accepted", + current: current, + proposed: &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + }, + }, + { + Name: "ingest", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("10Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + expectErrors: false, + }, + + { + name: "new instance accepted", + current: nil, + proposed: current, + expectErrors: false, + }, + } + + for _, tt := range tests { + actual := pvcModification(tt.current, tt.proposed) + actualErrors := len(actual) > 0 + if tt.expectErrors != actualErrors { + t.Errorf("failed pvcModification(). Name: %v, actual %v, wanted: %v, value: %v", tt.name, actual, tt.expectErrors, tt.proposed) + } + } +} + func TestValidation_noDowngrades(t *testing.T) { tests := []struct { name string @@ -468,3 +702,31 @@ func es(v string) *Elasticsearch { Spec: ElasticsearchSpec{Version: v}, } } + +// // getEsCluster returns a ES cluster test fixture +func getEsCluster() *Elasticsearch { + return &Elasticsearch{ + Spec: ElasticsearchSpec{ + Version: "7.2.0", + NodeSets: []NodeSet{ + { + Name: "master", + VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "elasticsearch-data", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("5Gi"), + }, + }, + }, + }, + }, + }, + }, + }, + } +} From ffe8e59d7c6febe2bed6155038563a001e6ff7a3 Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 17 Sep 2020 18:41:48 +0200 Subject: [PATCH 18/25] Ignore storage requests changes in the pvc validation webhook --- pkg/apis/elasticsearch/v1/validations.go | 40 +++++++++++++------ pkg/apis/elasticsearch/v1/validations_test.go | 31 +------------- 2 files changed, 29 insertions(+), 42 deletions(-) diff --git a/pkg/apis/elasticsearch/v1/validations.go b/pkg/apis/elasticsearch/v1/validations.go index 263ae126ba..4b35134f02 100644 --- a/pkg/apis/elasticsearch/v1/validations.go +++ b/pkg/apis/elasticsearch/v1/validations.go @@ -9,13 +9,14 @@ import ( "net" "strings" - apiequality "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/apimachinery/pkg/util/validation/field" - commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/version" esversion "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/version" netutil "github.com/elastic/cloud-on-k8s/pkg/utils/net" + corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/resource" + "k8s.io/apimachinery/pkg/util/validation/field" ) const ( @@ -29,7 +30,7 @@ const ( nodeRolesInOldVersionMsg = "node.roles setting is not available in this version of Elasticsearch" parseStoredVersionErrMsg = "Cannot parse current Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" parseVersionErrMsg = "Cannot parse Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" - pvcImmutableMsg = "Volume claim templates cannot be modified" + pvcImmutableMsg = "Volume claim templates cannot be modified. Only storage requests may be increased, if the storage class allows volume expansion." unsupportedConfigErrMsg = "Configuration setting is reserved for internal use. User-configured use is unsupported" unsupportedUpgradeMsg = "Unsupported version upgrade path. Check the Elasticsearch documentation for supported upgrade paths." unsupportedVersionMsg = "Unsupported version" @@ -207,29 +208,44 @@ func checkNodeSetNameUniqueness(es *Elasticsearch) field.ErrorList { return errs } -// pvcModification ensures no PVCs are changed, as volume claim templates are immutable in stateful sets +// pvcModification ensures the only part of volume claim templates that can be changed is storage requests. +// VolumeClaimTemplates are immutable in StatefulSets, but we still manage to deal with volume expansion. func pvcModification(current, proposed *Elasticsearch) field.ErrorList { var errs field.ErrorList if current == nil || proposed == nil { return errs } - for i, node := range proposed.Spec.NodeSets { - currNode := getNode(node.Name, current) - if currNode == nil { + for i, proposedNodeSet := range proposed.Spec.NodeSets { + currNodeSet := getNode(proposedNodeSet.Name, current) + if currNodeSet == nil { // this is a new sset, so there is nothing to check continue } - // ssets do not allow modifications to fields other than 'replicas', 'template', and 'updateStrategy' - // reflection isn't ideal, but okay here since the ES object does not have the status of the claims. + // Check that no modification was made to the volumeClaimTemplates, except on storage requests. // Checking semantic equality here allows providing PVC storage size with different units (eg. 1Ti vs. 1024Gi). - if !apiequality.Semantic.DeepEqual(node.VolumeClaimTemplates, currNode.VolumeClaimTemplates) { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), node.VolumeClaimTemplates, pvcImmutableMsg)) + // We could conceptually also disallow storage decrease here (unsupported), but it would make it hard for users + // to revert to the previous size if tried increasing but their storage class doesn't support expansion. + currentClaims := currNodeSet.VolumeClaimTemplates + proposedClaims := proposedNodeSet.VolumeClaimTemplates + if !apiequality.Semantic.DeepEqual(claimsWithoutStorageReq(currentClaims), claimsWithoutStorageReq(proposedClaims)) { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), proposedClaims, pvcImmutableMsg)) } } return errs } +// claimsWithoutStorageReq returns a copy of the given claims, with all storage requests set to the empty quantity. +func claimsWithoutStorageReq(claims []corev1.PersistentVolumeClaim) []corev1.PersistentVolumeClaim { + result := make([]corev1.PersistentVolumeClaim, 0, len(claims)) + for _, claim := range claims { + patchedClaim := *claim.DeepCopy() + patchedClaim.Spec.Resources.Requests[corev1.ResourceStorage] = resource.Quantity{} + result = append(result, patchedClaim) + } + return result +} + func noDowngrades(current, proposed *Elasticsearch) field.ErrorList { var errs field.ErrorList if current == nil || proposed == nil { diff --git a/pkg/apis/elasticsearch/v1/validations_test.go b/pkg/apis/elasticsearch/v1/validations_test.go index 71ad2d5173..e40359b221 100644 --- a/pkg/apis/elasticsearch/v1/validations_test.go +++ b/pkg/apis/elasticsearch/v1/validations_test.go @@ -326,7 +326,7 @@ func Test_pvcModified(t *testing.T) { expectErrors bool }{ { - name: "resize fails", + name: "resize succeeds", current: current, proposed: &Elasticsearch{ Spec: ElasticsearchSpec{ @@ -352,35 +352,6 @@ func Test_pvcModified(t *testing.T) { }, }, }, - expectErrors: true, - }, - { - name: "same size with different unit accepted", - current: current, - proposed: &Elasticsearch{ - Spec: ElasticsearchSpec{ - Version: "7.2.0", - NodeSets: []NodeSet{ - { - Name: "master", - VolumeClaimTemplates: []corev1.PersistentVolumeClaim{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "elasticsearch-data", - }, - Spec: corev1.PersistentVolumeClaimSpec{ - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("5120Mi"), - }, - }, - }, - }, - }, - }, - }, - }, - }, expectErrors: false, }, { From 0793b913e1e1f4c04a705f8c009889a778cabc95 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 22 Sep 2020 11:32:10 +0200 Subject: [PATCH 19/25] Reword the pvc modification error msg --- pkg/apis/elasticsearch/v1/validations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/elasticsearch/v1/validations.go b/pkg/apis/elasticsearch/v1/validations.go index 4b35134f02..d994e4ef10 100644 --- a/pkg/apis/elasticsearch/v1/validations.go +++ b/pkg/apis/elasticsearch/v1/validations.go @@ -30,7 +30,7 @@ const ( nodeRolesInOldVersionMsg = "node.roles setting is not available in this version of Elasticsearch" parseStoredVersionErrMsg = "Cannot parse current Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" parseVersionErrMsg = "Cannot parse Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]" - pvcImmutableMsg = "Volume claim templates cannot be modified. Only storage requests may be increased, if the storage class allows volume expansion." + pvcImmutableMsg = "Volume claim templates can only have their storage requests increased, if the storage class allows volume expansion. Any other change is forbidden." unsupportedConfigErrMsg = "Configuration setting is reserved for internal use. User-configured use is unsupported" unsupportedUpgradeMsg = "Unsupported version upgrade path. Check the Elasticsearch documentation for supported upgrade paths." unsupportedVersionMsg = "Unsupported version" From 697f1c11f407bd6b2c46551f25bf9b4433b607d4 Mon Sep 17 00:00:00 2001 From: sebgl Date: Tue, 29 Sep 2020 18:54:39 +0200 Subject: [PATCH 20/25] Store statefulsets to recreate in an annotation Before this commit, it was possible to end up with orphan Pods in case the Elasticsearch spec changed after a StatefulSet was just deleted. This commit ensures deleted StatefulSets are always recreated, by storing their spec in an annotation of the ES resource before doing the actual deletion. --- pkg/controller/elasticsearch/driver/nodes.go | 15 ++ .../elasticsearch/driver/pvc_expansion.go | 162 +++++++++++++--- .../driver/pvc_expansion_test.go | 173 +++++++++++++++--- .../elasticsearch/driver/upscale.go | 11 +- .../elasticsearch/driver/upscale_test.go | 29 +-- 5 files changed, 315 insertions(+), 75 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/nodes.go b/pkg/controller/elasticsearch/driver/nodes.go index 14d1b6c5d9..0d21536044 100644 --- a/pkg/controller/elasticsearch/driver/nodes.go +++ b/pkg/controller/elasticsearch/driver/nodes.go @@ -49,6 +49,21 @@ func (d *defaultDriver) reconcileNodeSpecs( return results.WithResult(defaultRequeue) } + // recreate any StatefulSet that needs to account for PVC expansion + recreations, err := recreateStatefulSets(d.K8sClient(), d.ES) + if err != nil { + return results.WithError(fmt.Errorf("StatefulSet recreation: %w", err)) + } + if recreations > 0 { + // Some StatefulSets are in the process of being recreated to handle PVC expansion: + // it is safer to requeue until the re-creation is done. + // Otherwise, some operation could be performed with wrong assumptions: + // the sset doesn't exist (was just deleted), but the Pods do actually exist. + log.V(1).Info("StatefulSets recreation in progress, re-queueing.", + "namespace", d.ES.Namespace, "es_name", d.ES.Name, "recreations", recreations) + return results.WithResult(defaultRequeue) + } + actualStatefulSets, err := sset.RetrieveActualStatefulSets(d.Client, k8s.ExtractNamespacedName(&d.ES)) if err != nil { return results.WithError(err) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index aa0c3214ec..5abc56a92a 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -5,30 +5,49 @@ package driver import ( + "encoding/json" "fmt" + "strings" + "sigs.k8s.io/controller-runtime/pkg/client" + + esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + // RecreateStatefulSetAnnotationPrefix is used to annotate the Elasticsearch resource + // with StatefulSets to recreate. The StatefulSet name is appended to this name. + RecreateStatefulSetAnnotationPrefix = "elasticsearch.k8s.elastic.co/recreate-" ) // handleVolumeExpansion works around the immutability of VolumeClaimTemplates in StatefulSets by: // 1. updating storage requests in PVCs whose storage class supports volume expansion -// 2. deleting the StatefulSet, to be recreated with the new storage spec -// It returns a boolean indicating whether the StatefulSet was deleted. +// 2. scheduling the StatefulSet for recreation with the new storage spec +// It returns a boolean indicating whether the StatefulSet needs to be recreated. // Note that some storage drivers also require Pods to be deleted/recreated for the filesystem to be resized // (as opposed to a hot resize while the Pod is running). This is left to the responsibility of the user. // This should be handled differently once supported by the StatefulSet controller: https://github.com/kubernetes/kubernetes/issues/68737. -func handleVolumeExpansion(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { +func handleVolumeExpansion(k8sClient k8s.Client, es esv1.Elasticsearch, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { err := resizePVCs(k8sClient, expectedSset, actualSset) if err != nil { return false, err } - return deleteSsetForClaimResize(k8sClient, expectedSset, actualSset) + + recreate, err := needsRecreate(expectedSset, actualSset) + if err != nil { + return false, err + } + if !recreate { + return false, nil + } + return true, annotateForRecreation(k8sClient, es, actualSset, expectedSset.Spec.VolumeClaimTemplates) } // resizePVCs updates the spec of all existing PVCs whose storage requests can be expanded, @@ -70,30 +89,28 @@ func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSse return nil } -// deleteSsetForClaimResize compares expected vs. actual StatefulSets, and deletes the actual one -// if a volume expansion can be performed. Pods remain orphan until the StatefulSet is created again. -func deleteSsetForClaimResize(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { - shouldRecreate, err := needsRecreate(expectedSset, actualSset) +// annotateForRecreation stores the StatefulSet spec with updated storage requirements +// in an annotation of the Elasticsearch resource, to be recreated at the next reconciliation. +func annotateForRecreation( + k8sClient k8s.Client, + es esv1.Elasticsearch, + actualSset appsv1.StatefulSet, + expectedClaims []corev1.PersistentVolumeClaim, +) error { + log.Info("Preparing StatefulSet re-creation to account for PVC resize", + "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", actualSset.Name) + + actualSset.Spec.VolumeClaimTemplates = expectedClaims + asJSON, err := json.Marshal(actualSset) if err != nil { - return false, err + return err } - if !shouldRecreate { - return false, nil + if es.Annotations == nil { + es.Annotations = make(map[string]string, 1) } + es.Annotations[RecreateStatefulSetAnnotationPrefix+actualSset.Name] = string(asJSON) - log.Info("Deleting StatefulSet to account for resized PVCs, it will be recreated automatically", - "namespace", actualSset.Namespace, "statefulset_name", actualSset.Name) - - opts := client.DeleteOptions{} - // ensure Pods are not also deleted - orphanPolicy := metav1.DeletePropagationOrphan - opts.PropagationPolicy = &orphanPolicy - // ensure we are not deleting based on out-of-date sset spec - opts.Preconditions = &metav1.Preconditions{ - UID: &actualSset.UID, - ResourceVersion: &actualSset.ResourceVersion, - } - return true, k8sClient.Delete(&actualSset, &opts) + return k8sClient.Update(&es) } // needsRecreate returns true if the StatefulSet needs to be re-created to account for volume expansion. @@ -137,3 +154,98 @@ func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Qu return true, nil } } + +// recreateStatefulSets re-creates StatefulSets as specified in Elasticsearch annotations, to account for +// resized volume claims. +// This functions acts as a state machine that depends on the annotation and the UID of existing StatefulSets. +// A standard flow may span over multiple reconciliations like this: +// 1. No annotation set: nothing to do. +// 2. An annotation specifies StatefulSet Foo needs to be recreated. That StatefulSet actually exists: delete it. +// 3. An annotation specifies StatefulSet Foo needs to be recreated. That StatefulSet does not exist: create it. +// 4. An annotation specifies StatefulSet Foo needs to be recreated. That StatefulSet actually exists, but with +// a different UID: the re-creation is over, remove the annotation. +func recreateStatefulSets(k8sClient k8s.Client, es esv1.Elasticsearch) (int, error) { + toRecreate, err := ssetsToRecreate(es) + if err != nil { + return 0, err + } + recreations := len(toRecreate) + + for annotation, toRecreate := range toRecreate { + var existing appsv1.StatefulSet + err := k8sClient.Get(k8s.ExtractNamespacedName(&toRecreate), &existing) + switch { + // error case + case err != nil && !apierrors.IsNotFound(err): + return recreations, err + + // already exists with the same UID: deletion case + case existing.UID == toRecreate.UID: + log.Info("Deleting StatefulSet to account for resized PVCs, it will be recreated automatically", + "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", existing.Name) + if err := deleteStatefulSet(k8sClient, existing); err != nil { + return recreations, err + } + + // already deleted: creation case + case err != nil && apierrors.IsNotFound(err): + log.Info("Re-creating StatefulSet to account for resized PVCs", + "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", toRecreate.Name) + if err := recreateStatefulSet(k8sClient, toRecreate); err != nil { + return recreations, err + } + + // already recreated (existing.UID != toRecreate.UID): we're done + default: + // remove the annotation + delete(es.Annotations, annotation) + if err := k8sClient.Update(&es); err != nil { + return recreations, err + } + } + } + + return recreations, nil +} + +// ssetsToRecreateFromAnnotation returns the list of StatefulSet that should be recreated, based on annotations +// in the Elasticsearch resource. +func ssetsToRecreate(es esv1.Elasticsearch) (map[string]appsv1.StatefulSet, error) { + toRecreate := map[string]appsv1.StatefulSet{} + for key, value := range es.Annotations { + if !strings.HasPrefix(key, RecreateStatefulSetAnnotationPrefix) { + continue + } + var sset appsv1.StatefulSet + if err := json.Unmarshal([]byte(value), &sset); err != nil { + return nil, err + } + toRecreate[key] = sset + } + return toRecreate, nil +} + +func deleteStatefulSet(k8sClient k8s.Client, sset appsv1.StatefulSet) error { + opts := client.DeleteOptions{} + // ensure we are not deleting the StatefulSet that was already recreated with a different UID + opts.Preconditions = &metav1.Preconditions{UID: &sset.UID} + // ensure Pods are not also deleted + orphanPolicy := metav1.DeletePropagationOrphan + opts.PropagationPolicy = &orphanPolicy + + return k8sClient.Delete(&sset, &opts) +} + +func recreateStatefulSet(k8sClient k8s.Client, sset appsv1.StatefulSet) error { + // don't keep metadata inherited from the old StatefulSet + newObjMeta := metav1.ObjectMeta{ + Name: sset.Name, + Namespace: sset.Namespace, + Labels: sset.Labels, + Annotations: sset.Annotations, + OwnerReferences: sset.OwnerReferences, + Finalizers: sset.Finalizers, + } + sset.ObjectMeta = newObjMeta + return k8sClient.Create(&sset) +} diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index e2d9d30a4c..028c5eddb3 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -5,16 +5,17 @@ package driver import ( + "encoding/json" "fmt" "testing" + esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -60,7 +61,8 @@ func withStorageReq(claim corev1.PersistentVolumeClaim, size string) corev1.Pers return *c } -func Test_resizePVCs(t *testing.T) { +func Test_handleVolumeExpansion(t *testing.T) { + es := esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es"}} sset := appsv1.StatefulSet{ ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sample-sset"}, Spec: appsv1.StatefulSetSpec{ @@ -98,6 +100,7 @@ func Test_resizePVCs(t *testing.T) { runtimeObjs []runtime.Object expectedPVCs []corev1.PersistentVolumeClaim wantErr bool + wantRecreate bool }{ { name: "no pvc to resize", @@ -107,6 +110,7 @@ func Test_resizePVCs(t *testing.T) { }, runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), expectedPVCs: pvcsWithSize("1Gi", "1Gi", "1Gi"), + wantRecreate: false, }, { name: "all pvcs should be resized", @@ -116,6 +120,7 @@ func Test_resizePVCs(t *testing.T) { }, runtimeObjs: append(pvcPtrs(pvcsWithSize("1Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), expectedPVCs: pvcsWithSize("3Gi", "3Gi", "3Gi"), + wantRecreate: true, }, { name: "2 pvcs left to resize", @@ -125,6 +130,7 @@ func Test_resizePVCs(t *testing.T) { }, runtimeObjs: append(pvcPtrs(pvcsWithSize("3Gi", "1Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), expectedPVCs: pvcsWithSize("3Gi", "3Gi", "3Gi"), + wantRecreate: true, }, { name: "one pvc is missing: resize what's there, don't error out", @@ -134,6 +140,7 @@ func Test_resizePVCs(t *testing.T) { }, runtimeObjs: append(pvcPtrs(pvcsWithSize("3Gi", "1Gi")), withVolumeExpansion(sampleStorageClass)), expectedPVCs: pvcsWithSize("3Gi", "3Gi"), + wantRecreate: true, }, { name: "storage decrease is not supported: error out", @@ -148,26 +155,48 @@ func Test_resizePVCs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - k8sClient := k8s.WrappedFakeClient(tt.runtimeObjs...) - if err := resizePVCs(k8sClient, tt.args.expectedSset, tt.args.actualSset); (err != nil) != tt.wantErr { + k8sClient := k8s.WrappedFakeClient(append(tt.runtimeObjs, &es)...) + recreate, err := handleVolumeExpansion(k8sClient, es, tt.args.expectedSset, tt.args.actualSset) + if (err != nil) != tt.wantErr { t.Errorf("resizePVCs() error = %v, wantErr %v", err, tt.wantErr) } + require.Equal(t, tt.wantRecreate, recreate) // all expected PVCs should exist in the apiserver var pvcs corev1.PersistentVolumeClaimList - err := k8sClient.List(&pvcs) + err = k8sClient.List(&pvcs) require.NoError(t, err) require.Len(t, pvcs.Items, len(tt.expectedPVCs)) for i, expectedPVC := range tt.expectedPVCs { comparison.RequireEqual(t, &expectedPVC, &pvcs.Items[i]) } + + // Elasticsearch should be annotated with the sset to recreate + var retrievedES esv1.Elasticsearch + err = k8sClient.Get(k8s.ExtractNamespacedName(&es), &retrievedES) + require.NoError(t, err) + if tt.wantRecreate { + require.Len(t, retrievedES.Annotations, 1) + wantUpdatedSset := tt.args.actualSset.DeepCopy() + // should have the expected claims + wantUpdatedSset.Spec.VolumeClaimTemplates = tt.args.expectedSset.Spec.VolumeClaimTemplates + + // test ssetsToRecreate along the way + toRecreate, err := ssetsToRecreate(retrievedES) + require.NoError(t, err) + require.Equal(t, + map[string]appsv1.StatefulSet{ + "elasticsearch.k8s.elastic.co/recreate-" + tt.args.actualSset.Name: *wantUpdatedSset}, + toRecreate) + } else { + require.Empty(t, retrievedES.Annotations) + } }) } } -func Test_deleteSsetForClaimResize(t *testing.T) { +func Test_needsRecreate(t *testing.T) { type args struct { - k8sClient k8s.Client expectedSset appsv1.StatefulSet actualSset appsv1.StatefulSet } @@ -180,7 +209,6 @@ func Test_deleteSsetForClaimResize(t *testing.T) { { name: "requested storage increase in the 2nd claim: recreate", args: args{ - k8sClient: k8s.WrappedFakeClient(&sampleSset, withVolumeExpansion(sampleStorageClass)), expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "3Gi")), actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), }, @@ -189,7 +217,6 @@ func Test_deleteSsetForClaimResize(t *testing.T) { { name: "no claim in the StatefulSet", args: args{ - k8sClient: k8s.WrappedFakeClient(&sampleSset), expectedSset: sampleSset, actualSset: sampleSset, }, @@ -198,7 +225,6 @@ func Test_deleteSsetForClaimResize(t *testing.T) { { name: "no change in the claim", args: args{ - k8sClient: k8s.WrappedFakeClient(&sampleSset), expectedSset: withClaims(sampleSset, sampleClaim), actualSset: withClaims(sampleSset, sampleClaim), }, @@ -207,7 +233,6 @@ func Test_deleteSsetForClaimResize(t *testing.T) { { name: "requested storage decrease: error out", args: args{ - k8sClient: k8s.WrappedFakeClient(&sampleSset, withVolumeExpansion(sampleStorageClass)), expectedSset: withClaims(sampleSset, sampleClaim, withStorageReq(sampleClaim2, "0.5Gi")), actualSset: withClaims(sampleSset, sampleClaim, sampleClaim2), }, @@ -217,22 +242,13 @@ func Test_deleteSsetForClaimResize(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - deleted, err := deleteSsetForClaimResize(tt.args.k8sClient, tt.args.expectedSset, tt.args.actualSset) + got, err := needsRecreate(tt.args.expectedSset, tt.args.actualSset) if (err != nil) != tt.wantErr { - t.Errorf("deleteSsetForClaimResize() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("needsRecreate() error = %v, wantErr %v", err, tt.wantErr) return } - if deleted != tt.want { - t.Errorf("deleteSsetForClaimResize() got = %v, want %v", deleted, tt.want) - } - - // double-check if the sset is indeed deleted - var retrieved appsv1.StatefulSet - err = tt.args.k8sClient.Get(k8s.ExtractNamespacedName(&tt.args.actualSset), &retrieved) - if deleted { - require.True(t, apierrors.IsNotFound(err)) - } else { - require.NoError(t, err) + if got != tt.want { + t.Errorf("needsRecreate() got = %v, want %v", got, tt.want) } }) } @@ -312,3 +328,112 @@ func Test_isStorageExpansion(t *testing.T) { }) } } + +func Test_recreateStatefulSets(t *testing.T) { + es := func() *esv1.Elasticsearch { + return &esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es"}} + } + withAnnotation := func(es *esv1.Elasticsearch, key, value string) *esv1.Elasticsearch { + if es.Annotations == nil { + es.Annotations = map[string]string{} + } + es.Annotations[key] = value + return es + } + + sset1 := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-uid"}} + sset1Bytes, _ := json.Marshal(sset1) + sset1Json := string(sset1Bytes) + sset1DifferentUID := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-differentuid"}} + + sset2 := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset2", UID: "sset2-uid"}} + sset2Bytes, _ := json.Marshal(sset2) + sset2Json := string(sset2Bytes) + + type args struct { + existingSsets []runtime.Object + es esv1.Elasticsearch + } + tests := []struct { + name string + args + wantES esv1.Elasticsearch + wantSsets []appsv1.StatefulSet + wantRecreations int + }{ + { + name: "no annotation: nothing to do", + args: args{ + existingSsets: []runtime.Object{sset1}, + es: *es(), + }, + wantES: *es(), + wantRecreations: 0, + }, + { + name: "StatefulSet to delete", + args: args{ + existingSsets: []runtime.Object{sset1}, // exists with the same UID + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + }, + wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + wantSsets: nil, // deleted + wantRecreations: 1, + }, + { + name: "StatefulSet to create", + args: args{ + existingSsets: []runtime.Object{}, // doesn't exist + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + }, + wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + // created, no UUID due to how the fake client creates objects + wantSsets: []appsv1.StatefulSet{{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1"}}}, + wantRecreations: 1, + }, + { + name: "StatefulSet already recreated: remove the annotation", + args: args{ + existingSsets: []runtime.Object{sset1DifferentUID}, // recreated + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + }, + wantES: *es(), // annotation removed + wantSsets: []appsv1.StatefulSet{*sset1DifferentUID}, // same + wantRecreations: 1, // not considered done yet + }, + { + name: "multiple statefulsets to handle", + args: args{ + existingSsets: []runtime.Object{sset1, sset2}, + es: *withAnnotation(withAnnotation(es(), + "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), + }, + wantES: *withAnnotation(withAnnotation(es(), + "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), + wantSsets: nil, + wantRecreations: 2, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + k8sClient := k8s.WrappedFakeClient(append(tt.args.existingSsets, &tt.args.es)...) + got, err := recreateStatefulSets(k8sClient, tt.args.es) + require.NoError(t, err) + require.Equal(t, tt.wantRecreations, got) + + var retrievedES esv1.Elasticsearch + err = k8sClient.Get(k8s.ExtractNamespacedName(&tt.args.es), &retrievedES) + require.NoError(t, err) + comparison.RequireEqual(t, &tt.wantES, &retrievedES) + + var retrievedSsets appsv1.StatefulSetList + err = k8sClient.List(&retrievedSsets) + require.NoError(t, err) + for i := range tt.wantSsets { + comparison.RequireEqual(t, &tt.wantSsets[i], &retrievedSsets.Items[i]) + } + }) + } +} diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index bc3b395365..c79a8f0a37 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -41,7 +41,7 @@ type UpscaleResults struct { // - update existing StatefulSets specification, to be used for future pods rotation // - upscale StatefulSet for which we expect more replicas // - limit master node creation to one at a time -// - resize (inline) existing PVCs to match new StatefulSet storage reqs and delete the StatefulSet for recreation +// - resize (inline) existing PVCs to match new StatefulSet storage reqs and schedule the StatefulSet recreation // It does not: // - perform any StatefulSet downscale (left for downscale phase) // - perform any pod upgrade (left for rolling upgrade phase) @@ -65,15 +65,12 @@ func HandleUpscaleAndSpecChanges( return results, fmt.Errorf("reconcile service: %w", err) } if actualSset, exists := actualStatefulSets.GetByName(res.StatefulSet.Name); exists { - ssetDeleted, err := handleVolumeExpansion(ctx.k8sClient, res.StatefulSet, actualSset) + recreateSset, err := handleVolumeExpansion(ctx.k8sClient, ctx.es, res.StatefulSet, actualSset) if err != nil { return results, fmt.Errorf("handle volume expansion: %w", err) } - if ssetDeleted { - // The StatefulSet was just deleted: it is safer to requeue at the end of this function - // and let it be re-created at the next reconciliation. - // Otherwise, downscales and rolling upgrades could be performed with wrong assumptions: - // the sset isn't reported in actualStatefulSets (was just deleted), but the Pods actually exist. + if recreateSset { + // The StatefulSet is scheduled for recreation: let's requeue before attempting any further spec change. results.Requeue = true continue } diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index e429b2c260..41b5e37496 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -25,7 +25,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" storagev1 "k8s.io/api/storage/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -163,8 +162,12 @@ func TestHandleUpscaleAndSpecChanges(t *testing.T) { func TestHandleUpscaleAndSpecChanges_PVCResize(t *testing.T) { // focus on the special case of handling PVC resize es := esv1.Elasticsearch{ - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es"}, - Spec: esv1.ElasticsearchSpec{Version: "7.5.0"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es", Annotations: map[string]string{ + // simulate annotation already set otherwise we get a conflict when es is updated twice + // (first for initial master nodes, then for sset recreation) + "elasticsearch.k8s.elastic.co/initial-master-nodes": "sset1-0,sset1-1,sset1-2", + }}, + Spec: esv1.ElasticsearchSpec{Version: "7.5.0"}, } truePtr := true @@ -240,6 +243,8 @@ func TestHandleUpscaleAndSpecChanges_PVCResize(t *testing.T) { } k8sClient := k8s.WrappedFakeClient(&es, &storageClass, &actualStatefulSets[0], &actualStatefulSets[1]) + // retrieve the created es with its resource version set + require.NoError(t, k8sClient.Get(k8s.ExtractNamespacedName(&es.ObjectMeta), &es)) ctx := upscaleCtx{ k8sClient: k8sClient, es: es, @@ -248,26 +253,12 @@ func TestHandleUpscaleAndSpecChanges_PVCResize(t *testing.T) { parentCtx: context.Background(), } - // 2nd StatefulSet should be deleted, we should requeue + // 2nd StatefulSet should be marked for recreation, we should requeue res, err := HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) require.NoError(t, err) require.True(t, res.Requeue) - var retrievedSset appsv1.StatefulSet - err = k8sClient.Get(k8s.ExtractNamespacedName(&dataResized), &retrievedSset) - require.True(t, apierrors.IsNotFound(err)) - - // re-fetch es to simulate actual reconciliation behaviour require.NoError(t, k8sClient.Get(k8s.ExtractNamespacedName(&es.ObjectMeta), &es)) - ctx.es = es - // the 2nd sset does not exist anymore - actualStatefulSets = []appsv1.StatefulSet{actualStatefulSets[0]} - - // next run: the missing StatefulSet should be recreated - res, err = HandleUpscaleAndSpecChanges(ctx, actualStatefulSets, expectedResources) - require.NoError(t, err) - require.False(t, res.Requeue) - require.Len(t, res.ActualStatefulSets, 2) - require.Equal(t, resource.MustParse("3Gi"), *res.ActualStatefulSets[1].Spec.VolumeClaimTemplates[0].Spec.Resources.Requests.Storage()) + require.Len(t, es.Annotations, 2) // initial master nodes + sset to recreate } func Test_isReplicaIncrease(t *testing.T) { From 55a5c7732a2ba93ee5a587348554a8f84896d842 Mon Sep 17 00:00:00 2001 From: sebgl Date: Wed, 30 Sep 2020 14:35:00 +0200 Subject: [PATCH 21/25] Remove the special upscale state fix not needed anymore --- .../elasticsearch/driver/upscale.go | 40 +----- .../elasticsearch/driver/upscale_state.go | 8 +- .../driver/upscale_state_test.go | 134 +++++++++--------- .../elasticsearch/driver/upscale_test.go | 72 +--------- 4 files changed, 79 insertions(+), 175 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/upscale.go b/pkg/controller/elasticsearch/driver/upscale.go index c79a8f0a37..7d730761aa 100644 --- a/pkg/controller/elasticsearch/driver/upscale.go +++ b/pkg/controller/elasticsearch/driver/upscale.go @@ -94,7 +94,7 @@ func adjustResources( upscaleState := newUpscaleState(ctx, actualStatefulSets, expectedResources) adjustedResources := make(nodespec.ResourcesList, 0, len(expectedResources)) for _, nodeSpecRes := range expectedResources { - adjusted, err := adjustStatefulSetReplicas(ctx.k8sClient, upscaleState, actualStatefulSets, *nodeSpecRes.StatefulSet.DeepCopy()) + adjusted, err := adjustStatefulSetReplicas(upscaleState, actualStatefulSets, *nodeSpecRes.StatefulSet.DeepCopy()) if err != nil { return nil, err } @@ -123,50 +123,22 @@ func adjustZenConfig(k8sClient k8s.Client, es esv1.Elasticsearch, resources node // adjustStatefulSetReplicas updates the replicas count in expected according to // what is allowed by the upscaleState, that may be mutated as a result. func adjustStatefulSetReplicas( - k8sClient k8s.Client, upscaleState *upscaleState, actualStatefulSets sset.StatefulSetList, expected appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { - expectedReplicas := sset.GetReplicas(expected) - actual, alreadyExists := actualStatefulSets.GetByName(expected.Name) - actualReplicas := sset.GetReplicas(actual) // 0 if actual is empty - if !alreadyExists { - // In most cases, if the StatefulSet does not exist we're creating it for the first time (actualReplicas = 0). - // However if some Pods matching that missing StatefulSet exist we are likely in a situation where the - // StatefulSet is being re-created for volume expansion. In which case we want to take into account - // the existing Pods when setting the initial replicas value. - pods, err := sset.GetActualPodsForStatefulSet(k8sClient, k8s.ExtractNamespacedName(&expected)) - if err != nil { - return appsv1.StatefulSet{}, err - } - // The new StatefulSet replicas count should match the highest ordinal of Pods ready to be adopted. - actualReplicas = int32(0) - for _, pod := range pods { - _, ordinal, err := sset.StatefulSetName(pod.Name) - if err != nil { - return appsv1.StatefulSet{}, err - } - ordinalBasedReplicas := ordinal + 1 // 3 replicas -> pod-0, pod-1, pod-2 - if ordinalBasedReplicas > actualReplicas { - actualReplicas = ordinalBasedReplicas - } - } - if actualReplicas > 0 { - log.Info("Adjusting StatefulSet replicas based on orphan Pods from volume expansion", - "namespace", expected.Namespace, "statefulset_name", expected.Name, "replicas", actualReplicas) - } - } + expectedReplicas := sset.GetReplicas(expected) + actualReplicas := sset.GetReplicas(actual) if actualReplicas < expectedReplicas { - return upscaleState.limitNodesCreation(actualReplicas, expected) + return upscaleState.limitNodesCreation(actual, expected) } - if expectedReplicas < actualReplicas { + if alreadyExists && expectedReplicas < actualReplicas { // this is a downscale. // We still want to update the sset spec to the newest one, but leave scaling down as it's done later. - nodespec.UpdateReplicas(&expected, &actualReplicas) + nodespec.UpdateReplicas(&expected, actual.Spec.Replicas) } return expected, nil diff --git a/pkg/controller/elasticsearch/driver/upscale_state.go b/pkg/controller/elasticsearch/driver/upscale_state.go index 0b25c7f31c..4406705b09 100644 --- a/pkg/controller/elasticsearch/driver/upscale_state.go +++ b/pkg/controller/elasticsearch/driver/upscale_state.go @@ -157,7 +157,7 @@ func (s *upscaleState) getMaxNodesToCreate(noMoreThan int32) int32 { // limitNodesCreation decreases replica count in specs as needed, assumes an upscale is requested func (s *upscaleState) limitNodesCreation( - actualReplicas int32, + actual appsv1.StatefulSet, toApply appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { if err := buildOnce(s); err != nil { @@ -165,9 +165,10 @@ func (s *upscaleState) limitNodesCreation( } if label.IsMasterNodeSet(toApply) { - return s.limitMasterNodesCreation(actualReplicas, toApply) + return s.limitMasterNodesCreation(actual, toApply) } + actualReplicas := sset.GetReplicas(actual) targetReplicas := sset.GetReplicas(toApply) nodespec.UpdateReplicas(&toApply, pointer.Int32(actualReplicas)) @@ -194,9 +195,10 @@ func (s *upscaleState) limitNodesCreation( } func (s *upscaleState) limitMasterNodesCreation( - actualReplicas int32, + actual appsv1.StatefulSet, toApply appsv1.StatefulSet, ) (appsv1.StatefulSet, error) { + actualReplicas := sset.GetReplicas(actual) targetReplicas := sset.GetReplicas(toApply) nodespec.UpdateReplicas(&toApply, pointer.Int32(actualReplicas)) diff --git a/pkg/controller/elasticsearch/driver/upscale_state_test.go b/pkg/controller/elasticsearch/driver/upscale_state_test.go index 7807373aa5..687877411c 100644 --- a/pkg/controller/elasticsearch/driver/upscale_state_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_state_test.go @@ -22,97 +22,97 @@ import ( func Test_upscaleState_limitNodesCreation(t *testing.T) { tests := []struct { - name string - state *upscaleState - actualReplicas int32 - ssetToApply appsv1.StatefulSet - wantSset appsv1.StatefulSet - wantState *upscaleState + name string + state *upscaleState + actual appsv1.StatefulSet + ssetToApply appsv1.StatefulSet + wantSset appsv1.StatefulSet + wantState *upscaleState }{ { - name: "no change on the sset spec", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, - actualReplicas: 3, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + name: "no change on the sset spec", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + actual: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, }, { - name: "spec change (same replicas)", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, - actualReplicas: 3, - ssetToApply: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + name: "spec change (same replicas)", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, + actual: sset.TestSset{Name: "sset", Version: "6.8.0", Replicas: 3, Master: true}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Version: "7.2.0", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true}, }, { - name: "upscale data nodes from 1 to 3: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, - actualReplicas: 1, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale data nodes from 1 to 3: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, + actual: sset.TestSset{Name: "sset", Replicas: 1, Master: false}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale data nodes from 1 to 4: should limit to 3", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, - actualReplicas: 1, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale data nodes from 1 to 4: should limit to 3", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2)}, + actual: sset.TestSset{Name: "sset", Replicas: 1, Master: false}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale master nodes from 1 to 3: should limit to 2", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, - actualReplicas: 1, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 2, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, + name: "upscale master nodes from 1 to 3: should limit to 2", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, + actual: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 2, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, }, { - name: "upscale master nodes from 1 to 3 when cluster not yet bootstrapped: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2)}, - actualReplicas: 1, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2), recordedCreates: 2}, + name: "upscale master nodes from 1 to 3 when cluster not yet bootstrapped: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2)}, + actual: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(2), recordedCreates: 2}, }, { - name: "upscale masters from 3 to 4, but no creates allowed: should limit to 0", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, - actualReplicas: 3, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, + name: "upscale masters from 3 to 4, but no creates allowed: should limit to 0", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, + actual: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, }, { - name: "upscale data nodes from 3 to 4, but no creates allowed: should limit to 0", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, - actualReplicas: 3, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, + name: "upscale data nodes from 3 to 4, but no creates allowed: should limit to 0", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, + actual: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: false}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0}, }, { - name: "new StatefulSet with 5 master nodes, cluster isn't bootstrapped yet: should go through", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3)}, - actualReplicas: 0, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3), recordedCreates: 3}, + name: "new StatefulSet with 5 master nodes, cluster isn't bootstrapped yet: should go through", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3)}, + actual: appsv1.StatefulSet{}, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: false, createsAllowed: pointer.Int32(3), recordedCreates: 3}, }, { - name: "new StatefulSet with 5 master nodes, cluster already bootstrapped: should limit to 1", - state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, - actualReplicas: 0, - ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), - wantSset: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), - wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, + name: "new StatefulSet with 5 master nodes, cluster already bootstrapped: should limit to 1", + state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(1)}, + actual: appsv1.StatefulSet{}, + ssetToApply: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), + wantSset: sset.TestSset{Name: "sset", Replicas: 1, Master: true}.Build(), + wantState: &upscaleState{allowMasterCreation: false, isBootstrapped: true, createsAllowed: pointer.Int32(1), recordedCreates: 1}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotSset, err := tt.state.limitNodesCreation(tt.actualReplicas, tt.ssetToApply) + gotSset, err := tt.state.limitNodesCreation(tt.actual, tt.ssetToApply) require.NoError(t, err) // StatefulSet should be adapted require.Equal(t, tt.wantSset, gotSset) diff --git a/pkg/controller/elasticsearch/driver/upscale_test.go b/pkg/controller/elasticsearch/driver/upscale_test.go index 41b5e37496..b65d445116 100644 --- a/pkg/controller/elasticsearch/driver/upscale_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_test.go @@ -298,7 +298,6 @@ func Test_isReplicaIncrease(t *testing.T) { func Test_adjustStatefulSetReplicas(t *testing.T) { type args struct { - k8sClient k8s.Client state *upscaleState actualStatefulSets sset.StatefulSetList expected appsv1.StatefulSet @@ -312,7 +311,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "new StatefulSet to create", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{}, expected: sset.TestSset{Name: "new-sset", Replicas: 3}.Build(), @@ -323,7 +321,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "same StatefulSet already exists", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 3}.Build(), @@ -334,7 +331,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "downscale case", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 1}.Build(), @@ -345,7 +341,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: data nodes", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: false, Data: true}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 5, Master: false, Data: true}.Build(), @@ -356,7 +351,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: master nodes - one by one", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: true, Data: true}.Build()}, expected: sset.TestSset{Name: "sset", Replicas: 5, Master: true, Data: true}.Build(), @@ -367,7 +361,6 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { { name: "upscale case: new additional master sset - one by one", args: args{ - k8sClient: k8s.WrappedFakeClient(), state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(3)}, actualStatefulSets: sset.StatefulSetList{sset.TestSset{Name: "sset", Replicas: 3, Master: true, Data: true}.Build()}, expected: sset.TestSset{Name: "sset-2", Replicas: 3, Master: true, Data: true}.Build(), @@ -375,73 +368,10 @@ func Test_adjustStatefulSetReplicas(t *testing.T) { want: sset.TestSset{Name: "sset-2", Replicas: 1, Master: true, Data: true}.Build(), wantUpscaleState: &upscaleState{recordedCreates: 1, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(3)}, }, - { - name: "new data-nodes StatefulSet to create, but some Pods already exist: volume expansion case", - args: args{ - // 2 Pods exist out of the 4 replicas - k8sClient: k8s.WrappedFakeClient( - // the match between sset and Pods is based on StatefulSetNameLabelName - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-0", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-1", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - ), - state: &upscaleState{isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, - actualStatefulSets: sset.StatefulSetList{}, - expected: sset.TestSset{Name: "new-sset", Replicas: 4}.Build(), - }, - // allow 2 (pre-existing) + 2 (new creation) replicas - want: sset.TestSset{Name: "new-sset", Replicas: 4}.Build(), - wantUpscaleState: &upscaleState{recordedCreates: 2, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, - }, - { - name: "new master-nodes StatefulSet to create, but some Pods already exist: volume expansion case", - args: args{ - // 2 Pods exist out of the 4 replicas - k8sClient: k8s.WrappedFakeClient( - // the match between sset and Pods is based on StatefulSetNameLabelName - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-0", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-1", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - ), - state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, - actualStatefulSets: sset.StatefulSetList{}, - expected: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), - }, - // allow 2 (pre-existing) + 1 (new master, one by one), out of 4 - want: sset.TestSset{Name: "new-sset", Master: true, Replicas: 3}.Build(), - wantUpscaleState: &upscaleState{recordedCreates: 1, isBootstrapped: true, allowMasterCreation: false, createsAllowed: pointer.Int32(4)}, - }, - { - name: "new master-nodes StatefulSet to create, some Pods already exist (volume expansion case), but first ones are missing", - args: args{ - // Pods 2 and 3 exist, but not 0 and 1 (which would normally just be recreated by the sset controller) - k8sClient: k8s.WrappedFakeClient( - // the match between sset and Pods is based on StatefulSetNameLabelName - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-2", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "new-sset-3", Labels: map[string]string{ - label.StatefulSetNameLabelName: "new-sset", - }}}, - ), - state: &upscaleState{isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, - actualStatefulSets: sset.StatefulSetList{}, - expected: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), - }, - // allow the 4 masters that should be there (out of 4) - want: sset.TestSset{Name: "new-sset", Master: true, Replicas: 4}.Build(), - wantUpscaleState: &upscaleState{recordedCreates: 0, isBootstrapped: true, allowMasterCreation: true, createsAllowed: pointer.Int32(4)}, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := adjustStatefulSetReplicas(tt.args.k8sClient, tt.args.state, tt.args.actualStatefulSets, tt.args.expected) + got, err := adjustStatefulSetReplicas(tt.args.state, tt.args.actualStatefulSets, tt.args.expected) require.NoError(t, err) require.Nil(t, deep.Equal(got, tt.want)) require.Equal(t, tt.wantUpscaleState, tt.args.state) From 5121657c32dfb7aa0120fc34d817922cbf8cb32f Mon Sep 17 00:00:00 2001 From: sebgl Date: Wed, 30 Sep 2020 14:35:16 +0200 Subject: [PATCH 22/25] Improvements from PR review --- .../elasticsearch/driver/pvc_expansion.go | 13 ++++++++----- .../elasticsearch/driver/pvc_expansion_test.go | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index 5abc56a92a..17ee5ee5ff 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -35,7 +35,7 @@ const ( // (as opposed to a hot resize while the Pod is running). This is left to the responsibility of the user. // This should be handled differently once supported by the StatefulSet controller: https://github.com/kubernetes/kubernetes/issues/68737. func handleVolumeExpansion(k8sClient k8s.Client, es esv1.Elasticsearch, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) (bool, error) { - err := resizePVCs(k8sClient, expectedSset, actualSset) + err := resizePVCs(k8sClient, es, expectedSset, actualSset) if err != nil { return false, err } @@ -53,7 +53,7 @@ func handleVolumeExpansion(k8sClient k8s.Client, es esv1.Elasticsearch, expected // resizePVCs updates the spec of all existing PVCs whose storage requests can be expanded, // according to their storage class and what's specified in the expected claim. // It returns an error if the requested storage size is incompatible with the PVC. -func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) error { +func resizePVCs(k8sClient k8s.Client, es esv1.Elasticsearch, expectedSset appsv1.StatefulSet, actualSset appsv1.StatefulSet) error { // match each existing PVC with an expected claim, and decide whether the PVC should be resized actualPVCs, err := sset.RetrieveActualPVCs(k8sClient, actualSset) if err != nil { @@ -78,7 +78,8 @@ func resizePVCs(k8sClient k8s.Client, expectedSset appsv1.StatefulSet, actualSse log.Info("Resizing PVC storage requests. Depending on the volume provisioner, "+ "Pods may need to be manually deleted for the filesystem to be resized.", - "namespace", pvc.Namespace, "pvc_name", pvc.Name, + "namespace", pvc.Namespace, "es_name", es.Name, + "pvc_name", pvc.Name, "old_value", pvcSize.String(), "new_value", claimSize.String()) pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *claimSize if err := k8sClient.Update(&pvc); err != nil { @@ -157,7 +158,7 @@ func isStorageExpansion(expectedSize *resource.Quantity, actualSize *resource.Qu // recreateStatefulSets re-creates StatefulSets as specified in Elasticsearch annotations, to account for // resized volume claims. -// This functions acts as a state machine that depends on the annotation and the UID of existing StatefulSets. +// This function acts as a state machine that depends on the annotation and the UID of existing StatefulSets. // A standard flow may span over multiple reconciliations like this: // 1. No annotation set: nothing to do. // 2. An annotation specifies StatefulSet Foo needs to be recreated. That StatefulSet actually exists: delete it. @@ -202,13 +203,15 @@ func recreateStatefulSets(k8sClient k8s.Client, es esv1.Elasticsearch) (int, err if err := k8sClient.Update(&es); err != nil { return recreations, err } + // one less recreation + recreations-- } } return recreations, nil } -// ssetsToRecreateFromAnnotation returns the list of StatefulSet that should be recreated, based on annotations +// ssetsToRecreate returns the list of StatefulSet that should be recreated, based on annotations // in the Elasticsearch resource. func ssetsToRecreate(es esv1.Elasticsearch) (map[string]appsv1.StatefulSet, error) { toRecreate := map[string]appsv1.StatefulSet{} diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 028c5eddb3..c700791267 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -399,7 +399,7 @@ func Test_recreateStatefulSets(t *testing.T) { }, wantES: *es(), // annotation removed wantSsets: []appsv1.StatefulSet{*sset1DifferentUID}, // same - wantRecreations: 1, // not considered done yet + wantRecreations: 0, }, { name: "multiple statefulsets to handle", From f22af15885bd2e8e212965a652df585d480b6213 Mon Sep 17 00:00:00 2001 From: sebgl Date: Wed, 30 Sep 2020 16:10:28 +0200 Subject: [PATCH 23/25] Set a temporary ownerref on Pods while the StatefulSet is being recreated If the Elasticsearch resource is removed while the StatefulSet is being recreated, orphan Pods will stay around forever. To avoid that situation, we can set append a temporary owner ref to the Pods before deleting their StatefulSet, so Pods don't stay orphans if Elasticsearch is deleted. We can then remove the temporary owner ref that was set after the StatefulSet is recreated. --- .../elasticsearch/driver/pvc_expansion.go | 60 ++++- .../driver/pvc_expansion_test.go | 221 ++++++++++++++++-- 2 files changed, 265 insertions(+), 16 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion.go b/pkg/controller/elasticsearch/driver/pvc_expansion.go index 17ee5ee5ff..27abcf0536 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion.go @@ -9,8 +9,6 @@ import ( "fmt" "strings" - "sigs.k8s.io/controller-runtime/pkg/client" - esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/sset" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" @@ -19,6 +17,9 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) const ( @@ -184,6 +185,10 @@ func recreateStatefulSets(k8sClient k8s.Client, es esv1.Elasticsearch) (int, err case existing.UID == toRecreate.UID: log.Info("Deleting StatefulSet to account for resized PVCs, it will be recreated automatically", "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", existing.Name) + // mark the Pod as owned by the ES resource while the StatefulSet is removed + if err := updatePodOwners(k8sClient, es, existing); err != nil { + return recreations, err + } if err := deleteStatefulSet(k8sClient, existing); err != nil { return recreations, err } @@ -198,6 +203,10 @@ func recreateStatefulSets(k8sClient k8s.Client, es esv1.Elasticsearch) (int, err // already recreated (existing.UID != toRecreate.UID): we're done default: + // remove the temporary pod owner set before the StatefulSet was deleted + if err := removeESPodOwner(k8sClient, es, existing); err != nil { + return recreations, err + } // remove the annotation delete(es.Annotations, annotation) if err := k8sClient.Update(&es); err != nil { @@ -252,3 +261,50 @@ func recreateStatefulSet(k8sClient k8s.Client, sset appsv1.StatefulSet) error { sset.ObjectMeta = newObjMeta return k8sClient.Create(&sset) } + +// updatePodOwners marks all Pods managed by the given StatefulSet as owned by the Elasticsearch resource. +// Pods are already owned by the StatefulSet resource, but when we'll (temporarily) delete that StatefulSet +// they won't be owned anymore. At this point if the Elasticsearch resource is deleted (before the StatefulSet +// is re-created), we also want the Pods to be deleted automatically. +func updatePodOwners(k8sClient k8s.Client, es esv1.Elasticsearch, statefulSet appsv1.StatefulSet) error { + log.V(1).Info("Setting an owner ref to the Elasticsearch resource on the future orphan Pods", + "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", statefulSet.Name) + return updatePods(k8sClient, statefulSet, func(p *corev1.Pod) error { + return controllerutil.SetOwnerReference(&es, p, scheme.Scheme) + }) +} + +// removeESPodOwner removes any reference to the ES resource from the Pods, that was set in updatePodOwners. +func removeESPodOwner(k8sClient k8s.Client, es esv1.Elasticsearch, statefulSet appsv1.StatefulSet) error { + log.V(1).Info("Removing any Pod owner ref set to the Elasticsearch resource after StatefulSet re-creation", + "namespace", es.Namespace, "es_name", es.Name, "statefulset_name", statefulSet.Name) + updateFunc := func(p *corev1.Pod) error { + for i, ownerRef := range p.OwnerReferences { + if ownerRef.UID == es.UID && ownerRef.Name == es.Name && ownerRef.Kind == es.Kind { + // remove from the owner ref slice + p.OwnerReferences = append(p.OwnerReferences[:i], p.OwnerReferences[i+1:]...) + return nil + } + } + return nil + } + return updatePods(k8sClient, statefulSet, updateFunc) +} + +// updatePods applies updateFunc on all existing Pods from the StatefulSet, then update those Pods. +func updatePods(k8sClient k8s.Client, statefulSet appsv1.StatefulSet, updateFunc func(p *corev1.Pod) error) error { + pods, err := sset.GetActualPodsForStatefulSet(k8sClient, k8s.ExtractNamespacedName(&statefulSet)) + if err != nil { + return err + } + for i := range pods { + if err := updateFunc(&pods[i]); err != nil { + return err + } + if err := k8sClient.Update(&pods[i]); err != nil { + return err + } + } + return nil + +} diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index c700791267..1a79dc0d17 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -11,6 +11,8 @@ import ( esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" + controllerscheme "github.com/elastic/cloud-on-k8s/pkg/controller/common/scheme" + "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/label" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" @@ -19,7 +21,9 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) var ( @@ -330,8 +334,9 @@ func Test_isStorageExpansion(t *testing.T) { } func Test_recreateStatefulSets(t *testing.T) { + controllerscheme.SetupScheme() es := func() *esv1.Elasticsearch { - return &esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es"}} + return &esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es", UID: "es-uid"}, TypeMeta: metav1.TypeMeta{Kind: "Elasticsearch"}} } withAnnotation := func(es *esv1.Elasticsearch, key, value string) *esv1.Elasticsearch { if es.Annotations == nil { @@ -345,66 +350,76 @@ func Test_recreateStatefulSets(t *testing.T) { sset1Bytes, _ := json.Marshal(sset1) sset1Json := string(sset1Bytes) sset1DifferentUID := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-differentuid"}} + pod1 := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1-0", Labels: map[string]string{ + label.StatefulSetNameLabelName: sset1.Name, + }}} + pod1WithOwnerRef := pod1.DeepCopy() + require.NoError(t, controllerutil.SetOwnerReference(es(), pod1WithOwnerRef, scheme.Scheme)) sset2 := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset2", UID: "sset2-uid"}} sset2Bytes, _ := json.Marshal(sset2) sset2Json := string(sset2Bytes) type args struct { - existingSsets []runtime.Object - es esv1.Elasticsearch + runtimeObjs []runtime.Object + es esv1.Elasticsearch } tests := []struct { name string args wantES esv1.Elasticsearch wantSsets []appsv1.StatefulSet + wantPods []corev1.Pod wantRecreations int }{ { name: "no annotation: nothing to do", args: args{ - existingSsets: []runtime.Object{sset1}, - es: *es(), + runtimeObjs: []runtime.Object{sset1, pod1}, + es: *es(), }, wantES: *es(), + wantPods: []corev1.Pod{*pod1}, wantRecreations: 0, }, { name: "StatefulSet to delete", args: args{ - existingSsets: []runtime.Object{sset1}, // exists with the same UID - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + runtimeObjs: []runtime.Object{sset1, pod1}, // sset exists with the same UID + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), }, wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), - wantSsets: nil, // deleted + wantSsets: nil, // deleted + wantPods: []corev1.Pod{*pod1WithOwnerRef}, // owner ref set to the ES resource wantRecreations: 1, }, { name: "StatefulSet to create", args: args{ - existingSsets: []runtime.Object{}, // doesn't exist - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + runtimeObjs: []runtime.Object{pod1}, // sset doesn't exist + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), }, wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), // created, no UUID due to how the fake client creates objects wantSsets: []appsv1.StatefulSet{{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1"}}}, + wantPods: []corev1.Pod{*pod1}, // unmodified wantRecreations: 1, }, { name: "StatefulSet already recreated: remove the annotation", args: args{ - existingSsets: []runtime.Object{sset1DifferentUID}, // recreated - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + runtimeObjs: []runtime.Object{sset1DifferentUID, pod1WithOwnerRef}, // sset recreated + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), }, wantES: *es(), // annotation removed wantSsets: []appsv1.StatefulSet{*sset1DifferentUID}, // same + wantPods: []corev1.Pod{*pod1}, // ownerRef removed wantRecreations: 0, }, { name: "multiple statefulsets to handle", args: args{ - existingSsets: []runtime.Object{sset1, sset2}, + runtimeObjs: []runtime.Object{sset1, sset2, pod1}, es: *withAnnotation(withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), @@ -413,12 +428,13 @@ func Test_recreateStatefulSets(t *testing.T) { "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), wantSsets: nil, + wantPods: []corev1.Pod{*pod1WithOwnerRef}, // ownerRef removed wantRecreations: 2, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - k8sClient := k8s.WrappedFakeClient(append(tt.args.existingSsets, &tt.args.es)...) + k8sClient := k8s.WrappedFakeClient(append(tt.args.runtimeObjs, &tt.args.es)...) got, err := recreateStatefulSets(k8sClient, tt.args.es) require.NoError(t, err) require.Equal(t, tt.wantRecreations, got) @@ -434,6 +450,183 @@ func Test_recreateStatefulSets(t *testing.T) { for i := range tt.wantSsets { comparison.RequireEqual(t, &tt.wantSsets[i], &retrievedSsets.Items[i]) } + + var retrievedPods corev1.PodList + err = k8sClient.List(&retrievedPods) + require.NoError(t, err) + for i := range tt.wantPods { + comparison.RequireEqual(t, &tt.wantPods[i], &retrievedPods.Items[i]) + } + }) + } +} + +var ( + sampleEs = esv1.Elasticsearch{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "es", UID: "es-uid"}, TypeMeta: metav1.TypeMeta{Kind: "Elasticsearch"}} + sset1 = appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-uid"}} + pod1 = corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1-0", Labels: map[string]string{ + label.StatefulSetNameLabelName: sset1.Name, + }}} + pod2 = corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1-1", Labels: map[string]string{ + label.StatefulSetNameLabelName: sset1.Name, + }}} + pod1WithOwnerRef = *pod1.DeepCopy() + pod2WithOwnerRef = *pod2.DeepCopy() +) + +func init() { + controllerscheme.SetupScheme() + if err := controllerutil.SetOwnerReference(&es, &pod1WithOwnerRef, scheme.Scheme); err != nil { + panic(err) + } + if err := controllerutil.SetOwnerReference(&es, &pod2WithOwnerRef, scheme.Scheme); err != nil { + panic(err) + } +} + +func Test_updatePodOwners(t *testing.T) { + type args struct { + k8sClient k8s.Client + es esv1.Elasticsearch + statefulSet appsv1.StatefulSet + } + tests := []struct { + name string + args args + wantPods []corev1.Pod + }{ + { + name: "happy path: set an owner ref to the ES resource on all Pods for that StatefulSet", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1, &pod2), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1WithOwnerRef, pod2WithOwnerRef}, + }, + { + name: "owner ref already set: the function is idempotent", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1WithOwnerRef, &pod2WithOwnerRef), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1WithOwnerRef, pod2WithOwnerRef}, + }, + { + name: "one owner ref already set, one missing", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1WithOwnerRef, &pod2), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1WithOwnerRef, pod2WithOwnerRef}, + }, + { + name: "no Pods: nothing to do", + args: args{ + k8sClient: k8s.WrappedFakeClient(), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := updatePodOwners(tt.args.k8sClient, tt.args.es, tt.args.statefulSet) + require.NoError(t, err) + + var retrievedPods corev1.PodList + err = tt.args.k8sClient.List(&retrievedPods) + require.NoError(t, err) + }) + } +} + +func withOwnerRef(pod corev1.Pod, ownerRef metav1.OwnerReference) *corev1.Pod { + pod = *pod.DeepCopy() + pod.OwnerReferences = append(pod.OwnerReferences, metav1.OwnerReference{ + Kind: "Unrelated", + Name: "unrelated-name", + UID: "uid", + }) + return &pod +} + +func Test_removeESPodOwner(t *testing.T) { + type args struct { + k8sClient k8s.Client + es esv1.Elasticsearch + statefulSet appsv1.StatefulSet + } + tests := []struct { + name string + args args + wantPods []corev1.Pod + }{ + { + name: "happy path: remove the owner ref from all Pods", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1WithOwnerRef, &pod2WithOwnerRef), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1, pod2}, + }, + { + name: "owner refs already removed: function is idempotent", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1, &pod2), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1, pod2}, + }, + { + name: "one owner ref already removed, the other not yet removed", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1WithOwnerRef, &pod2), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1, pod2}, + }, + { + name: "no Pods: nothing to do", + args: args{ + k8sClient: k8s.WrappedFakeClient(), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{}, + }, + { + name: "preserve existing unrelated owner refs", + args: args{ + k8sClient: k8s.WrappedFakeClient(&pod1WithOwnerRef, withOwnerRef(pod2WithOwnerRef, metav1.OwnerReference{ + Kind: "kind", + Name: "name", + UID: "uid", + })), + es: sampleEs, + statefulSet: sset1, + }, + wantPods: []corev1.Pod{pod1, *withOwnerRef(pod2, metav1.OwnerReference{ + Kind: "kind", + Name: "name", + UID: "uid", + })}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := removeESPodOwner(tt.args.k8sClient, tt.args.es, tt.args.statefulSet) + require.NoError(t, err) + + var retrievedPods corev1.PodList + err = tt.args.k8sClient.List(&retrievedPods) + require.NoError(t, err) }) } } From e82ba2d6fda4452267d0ef3c44e4e72c9f3f6797 Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 1 Oct 2020 10:08:05 +0200 Subject: [PATCH 24/25] Fix unit tests --- .../driver/pvc_expansion_test.go | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 1a79dc0d17..5756a2c9c7 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -348,7 +348,7 @@ func Test_recreateStatefulSets(t *testing.T) { sset1 := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-uid"}} sset1Bytes, _ := json.Marshal(sset1) - sset1Json := string(sset1Bytes) + sset1JSON := string(sset1Bytes) sset1DifferentUID := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1", UID: "sset1-differentuid"}} pod1 := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1-0", Labels: map[string]string{ label.StatefulSetNameLabelName: sset1.Name, @@ -386,9 +386,9 @@ func Test_recreateStatefulSets(t *testing.T) { name: "StatefulSet to delete", args: args{ runtimeObjs: []runtime.Object{sset1, pod1}, // sset exists with the same UID - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), }, - wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), wantSsets: nil, // deleted wantPods: []corev1.Pod{*pod1WithOwnerRef}, // owner ref set to the ES resource wantRecreations: 1, @@ -397,9 +397,9 @@ func Test_recreateStatefulSets(t *testing.T) { name: "StatefulSet to create", args: args{ runtimeObjs: []runtime.Object{pod1}, // sset doesn't exist - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), }, - wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + wantES: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), // created, no UUID due to how the fake client creates objects wantSsets: []appsv1.StatefulSet{{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset1"}}}, wantPods: []corev1.Pod{*pod1}, // unmodified @@ -409,7 +409,7 @@ func Test_recreateStatefulSets(t *testing.T) { name: "StatefulSet already recreated: remove the annotation", args: args{ runtimeObjs: []runtime.Object{sset1DifferentUID, pod1WithOwnerRef}, // sset recreated - es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + es: *withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), }, wantES: *es(), // annotation removed wantSsets: []appsv1.StatefulSet{*sset1DifferentUID}, // same @@ -421,16 +421,30 @@ func Test_recreateStatefulSets(t *testing.T) { args: args{ runtimeObjs: []runtime.Object{sset1, sset2, pod1}, es: *withAnnotation(withAnnotation(es(), - "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), }, wantES: *withAnnotation(withAnnotation(es(), - "elasticsearch.k8s.elastic.co/recreate-sset1", sset1Json), + "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), wantSsets: nil, wantPods: []corev1.Pod{*pod1WithOwnerRef}, // ownerRef removed wantRecreations: 2, }, + { + name: "additional annotations are ignored", + args: args{ + runtimeObjs: []runtime.Object{sset1DifferentUID, pod1}, // sset recreated + es: *withAnnotation(withAnnotation(es(), + "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), + "another-annotation-key", sset2Json), + }, + // sset annotation removed, other annotation preserved + wantES: *withAnnotation(es(), "another-annotation-key", sset2Json), + wantSsets: nil, + wantPods: []corev1.Pod{*pod1}, + wantRecreations: 0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -476,10 +490,10 @@ var ( func init() { controllerscheme.SetupScheme() - if err := controllerutil.SetOwnerReference(&es, &pod1WithOwnerRef, scheme.Scheme); err != nil { + if err := controllerutil.SetOwnerReference(&sampleEs, &pod1WithOwnerRef, scheme.Scheme); err != nil { panic(err) } - if err := controllerutil.SetOwnerReference(&es, &pod2WithOwnerRef, scheme.Scheme); err != nil { + if err := controllerutil.SetOwnerReference(&sampleEs, &pod2WithOwnerRef, scheme.Scheme); err != nil { panic(err) } } @@ -540,17 +554,16 @@ func Test_updatePodOwners(t *testing.T) { var retrievedPods corev1.PodList err = tt.args.k8sClient.List(&retrievedPods) require.NoError(t, err) + for i := range tt.wantPods { + comparison.RequireEqual(t, &tt.wantPods[i], &retrievedPods.Items[i]) + } }) } } func withOwnerRef(pod corev1.Pod, ownerRef metav1.OwnerReference) *corev1.Pod { pod = *pod.DeepCopy() - pod.OwnerReferences = append(pod.OwnerReferences, metav1.OwnerReference{ - Kind: "Unrelated", - Name: "unrelated-name", - UID: "uid", - }) + pod.OwnerReferences = append(pod.OwnerReferences, ownerRef) return &pod } @@ -627,6 +640,9 @@ func Test_removeESPodOwner(t *testing.T) { var retrievedPods corev1.PodList err = tt.args.k8sClient.List(&retrievedPods) require.NoError(t, err) + for i := range tt.wantPods { + comparison.RequireEqual(t, &tt.wantPods[i], &retrievedPods.Items[i]) + } }) } } From 9e442b16c7101a35a53a73fcf8899b740721f43e Mon Sep 17 00:00:00 2001 From: sebgl Date: Thu, 1 Oct 2020 11:17:12 +0200 Subject: [PATCH 25/25] Fix unit tests --- .../elasticsearch/driver/pvc_expansion_test.go | 10 +++++----- .../elasticsearch/driver/upscale_state_test.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go index 5756a2c9c7..8fb387e95c 100644 --- a/pkg/controller/elasticsearch/driver/pvc_expansion_test.go +++ b/pkg/controller/elasticsearch/driver/pvc_expansion_test.go @@ -358,7 +358,7 @@ func Test_recreateStatefulSets(t *testing.T) { sset2 := &appsv1.StatefulSet{ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "sset2", UID: "sset2-uid"}} sset2Bytes, _ := json.Marshal(sset2) - sset2Json := string(sset2Bytes) + sset2JSON := string(sset2Bytes) type args struct { runtimeObjs []runtime.Object @@ -422,11 +422,11 @@ func Test_recreateStatefulSets(t *testing.T) { runtimeObjs: []runtime.Object{sset1, sset2, pod1}, es: *withAnnotation(withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), - "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), + "elasticsearch.k8s.elastic.co/recreate-sset2", sset2JSON), }, wantES: *withAnnotation(withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), - "elasticsearch.k8s.elastic.co/recreate-sset2", sset2Json), + "elasticsearch.k8s.elastic.co/recreate-sset2", sset2JSON), wantSsets: nil, wantPods: []corev1.Pod{*pod1WithOwnerRef}, // ownerRef removed wantRecreations: 2, @@ -437,10 +437,10 @@ func Test_recreateStatefulSets(t *testing.T) { runtimeObjs: []runtime.Object{sset1DifferentUID, pod1}, // sset recreated es: *withAnnotation(withAnnotation(es(), "elasticsearch.k8s.elastic.co/recreate-sset1", sset1JSON), - "another-annotation-key", sset2Json), + "another-annotation-key", sset2JSON), }, // sset annotation removed, other annotation preserved - wantES: *withAnnotation(es(), "another-annotation-key", sset2Json), + wantES: *withAnnotation(es(), "another-annotation-key", sset2JSON), wantSsets: nil, wantPods: []corev1.Pod{*pod1}, wantRecreations: 0, diff --git a/pkg/controller/elasticsearch/driver/upscale_state_test.go b/pkg/controller/elasticsearch/driver/upscale_state_test.go index 687877411c..1e45400765 100644 --- a/pkg/controller/elasticsearch/driver/upscale_state_test.go +++ b/pkg/controller/elasticsearch/driver/upscale_state_test.go @@ -80,7 +80,7 @@ func Test_upscaleState_limitNodesCreation(t *testing.T) { { name: "upscale masters from 3 to 4, but no creates allowed: should limit to 0", state: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0)}, - actual: sset.TestSset{Name: "sset", Replicas: 3, Master: false}.Build(), + actual: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), ssetToApply: sset.TestSset{Name: "sset", Replicas: 4, Master: true}.Build(), wantSset: sset.TestSset{Name: "sset", Replicas: 3, Master: true}.Build(), wantState: &upscaleState{allowMasterCreation: true, isBootstrapped: true, createsAllowed: pointer.Int32(0), recordedCreates: 0},