From 168ea706f6e8f3e456b636366729095b754c2a1d Mon Sep 17 00:00:00 2001 From: Michael Montgomery Date: Tue, 22 Feb 2022 08:03:09 -0600 Subject: [PATCH] Elasticsearch: Set status.ObservedGeneration from metadata.Generation. (#5331) * Add es.status.ObservedGeneration to ES spec. Set observedGeneration from metadata.generation when generating initial state during ES reconcile. Make observedGeneration optional in spec. Adjust docs for observedGeneration. Add .DS_Store to git ignore. Add tests for observedGeneration behavior. Add e2e test for observedGeneration. * Move observedGeneration to be more alphabetical in struct. Adjust wording for state during ES reconciliation, as it's not a pointer. * Correct Spelling * Update documentation in test * Update test comments * Update e2e test to just update annotations, not disable tls. * remove unused ctx * allow fake k8s client to be disable/enabled on demand * directive `//nolint:gosimple` is unused for linter gosimple (nolintlint) * Fix phantom lint error?? * no need for func in testing, just call DisableFailing * Remove MacOS files from gitignore. Update ObservedGeneration documentation/comments. Remove duplicative license header. Use named field in test struct. * rename test file * remove new build tag format from new e2e file. * Removing changes associated with a failing k8s client. * Update crd generation description. Remove specific es generation test. Add generation test to all es mutation tests. * Correct runtime spelling. Update final generation e2e test step to be wrapped in test.Eventually * Use errors.New when not needing formatting. * remove erroneous nolint * s/CompareClusterGenerations/CompareClusterGenerationsSteps/g * Fix indentation in state.go * Adjust ES generation tests because of recent change concerning unknown status being set initially. --- config/crds/v1/all-crds.yaml | 9 + ...search.k8s.elastic.co_elasticsearches.yaml | 9 + .../eck-operator-crds/templates/all-crds.yaml | 9 + .../elasticsearch/v1/elasticsearch_types.go | 7 + .../elasticsearch_controller_test.go | 211 ++++++++++++++++++ .../elasticsearch/reconcile/state.go | 5 +- .../elasticsearch/reconcile/state_test.go | 18 ++ .../test/elasticsearch/cluster_generation.go | 86 +++++++ test/e2e/test/elasticsearch/steps_mutation.go | 3 + 9 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 pkg/controller/elasticsearch/elasticsearch_controller_test.go create mode 100644 test/e2e/test/elasticsearch/cluster_generation.go diff --git a/config/crds/v1/all-crds.yaml b/config/crds/v1/all-crds.yaml index 5120bb3ce5..3a97cff76a 100644 --- a/config/crds/v1/all-crds.yaml +++ b/config/crds/v1/all-crds.yaml @@ -4490,6 +4490,15 @@ spec: single Association of a given type (for ex. single ES reference), this map contains a single entry. type: object + observedGeneration: + description: ObservedGeneration is the most recent generation observed + for this Elasticsearch cluster. It corresponds to the metadata generation, + which is updated on mutation by the API Server. If the generation + observed in status diverges from the generation in metadata, the + Elasticsearch controller has not yet processed the changes contained + in the Elasticsearch specification. + format: int64 + type: integer phase: description: ElasticsearchOrchestrationPhase is the phase Elasticsearch is in from the controller point of view. diff --git a/config/crds/v1/bases/elasticsearch.k8s.elastic.co_elasticsearches.yaml b/config/crds/v1/bases/elasticsearch.k8s.elastic.co_elasticsearches.yaml index 06431d20f2..6cf0ad878f 100644 --- a/config/crds/v1/bases/elasticsearch.k8s.elastic.co_elasticsearches.yaml +++ b/config/crds/v1/bases/elasticsearch.k8s.elastic.co_elasticsearches.yaml @@ -9189,6 +9189,15 @@ spec: single Association of a given type (for ex. single ES reference), this map contains a single entry. type: object + observedGeneration: + description: ObservedGeneration is the most recent generation observed + for this Elasticsearch cluster. It corresponds to the metadata generation, + which is updated on mutation by the API Server. If the generation + observed in status diverges from the generation in metadata, the + Elasticsearch controller has not yet processed the changes contained + in the Elasticsearch specification. + format: int64 + type: integer phase: description: ElasticsearchOrchestrationPhase is the phase Elasticsearch is in from the controller point of view. diff --git a/deploy/eck-operator/charts/eck-operator-crds/templates/all-crds.yaml b/deploy/eck-operator/charts/eck-operator-crds/templates/all-crds.yaml index 12f38860b6..7c1e04343d 100644 --- a/deploy/eck-operator/charts/eck-operator-crds/templates/all-crds.yaml +++ b/deploy/eck-operator/charts/eck-operator-crds/templates/all-crds.yaml @@ -4520,6 +4520,15 @@ spec: single Association of a given type (for ex. single ES reference), this map contains a single entry. type: object + observedGeneration: + description: ObservedGeneration is the most recent generation observed + for this Elasticsearch cluster. It corresponds to the metadata generation, + which is updated on mutation by the API Server. If the generation + observed in status diverges from the generation in metadata, the + Elasticsearch controller has not yet processed the changes contained + in the Elasticsearch specification. + format: int64 + type: integer phase: description: ElasticsearchOrchestrationPhase is the phase Elasticsearch is in from the controller point of view. diff --git a/pkg/apis/elasticsearch/v1/elasticsearch_types.go b/pkg/apis/elasticsearch/v1/elasticsearch_types.go index 79d0e8ff15..c8d230eca7 100644 --- a/pkg/apis/elasticsearch/v1/elasticsearch_types.go +++ b/pkg/apis/elasticsearch/v1/elasticsearch_types.go @@ -437,6 +437,7 @@ const ( type ElasticsearchStatus struct { // AvailableNodes is the number of available instances. AvailableNodes int32 `json:"availableNodes,omitempty"` + // Version of the stack resource currently running. During version upgrades, multiple versions may run // in parallel: this value specifies the lowest version currently running. Version string `json:"version,omitempty"` @@ -444,6 +445,12 @@ type ElasticsearchStatus struct { Phase ElasticsearchOrchestrationPhase `json:"phase,omitempty"` MonitoringAssociationsStatus commonv1.AssociationStatusMap `json:"monitoringAssociationStatus,omitempty"` + + // ObservedGeneration is the most recent generation observed for this Elasticsearch cluster. + // It corresponds to the metadata generation, which is updated on mutation by the API Server. + // If the generation observed in status diverges from the generation in metadata, the Elasticsearch + // controller has not yet processed the changes contained in the Elasticsearch specification. + ObservedGeneration int64 `json:"observedGeneration,omitempty"` } type ZenDiscoveryStatus struct { diff --git a/pkg/controller/elasticsearch/elasticsearch_controller_test.go b/pkg/controller/elasticsearch/elasticsearch_controller_test.go new file mode 100644 index 0000000000..b45751d4eb --- /dev/null +++ b/pkg/controller/elasticsearch/elasticsearch_controller_test.go @@ -0,0 +1,211 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. +package elasticsearch + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" + "github.com/elastic/cloud-on-k8s/pkg/controller/common" + "github.com/elastic/cloud-on-k8s/pkg/controller/common/comparison" + "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/hints" + "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" +) + +// newTestReconciler returns a ReconcileElasticsearch struct, allowing the internal k8s client to +// contain certain runtime objects. +func newTestReconciler(objects ...runtime.Object) *ReconcileElasticsearch { + r := &ReconcileElasticsearch{ + Client: k8s.NewFakeClient(objects...), + recorder: record.NewFakeRecorder(100), + } + return r +} + +// esBuilder allows for a cleaner way to build a testable elasticsearch spec, +// minimizing repetition. +type esBuilder struct { + es *esv1.Elasticsearch +} + +// newBuilder returns a new elasticsearch test builder +// with given name/namespace combination. +func newBuilder(name, namespace string) *esBuilder { + return &esBuilder{ + es: &esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + }, + } +} + +// WithAnnotations adds the given annotations to the ES object. +func (e *esBuilder) WithAnnotations(annotations map[string]string) *esBuilder { + e.es.ObjectMeta.Annotations = annotations + return e +} + +// WithGeneration adds the metadata.generation to the ES object. +func (e *esBuilder) WithGeneration(generation int64) *esBuilder { + e.es.ObjectMeta.Generation = generation + return e +} + +// WithStatus adds the status subresource to the ES object. +func (e *esBuilder) WithStatus(status esv1.ElasticsearchStatus) *esBuilder { + e.es.Status = status + return e +} + +// WithVersion adds the ES version to the ES objects specification. +func (e *esBuilder) WithVersion(version string) *esBuilder { + e.es.Spec.Version = version + return e +} + +// Build builds the final ES object and returns a pointer. +func (e *esBuilder) Build() *esv1.Elasticsearch { + return e.es +} + +// BuildAndCopy builds the final ES object and returns a copy. +func (e *esBuilder) BuildAndCopy() esv1.Elasticsearch { + return *e.es +} + +func TestReconcileElasticsearch_Reconcile(t *testing.T) { + type k8sClientFields struct { + objects []runtime.Object + } + type args struct { + request reconcile.Request + } + tests := []struct { + name string + k8sClientFields k8sClientFields + args args + wantErr bool + expected esv1.Elasticsearch + }{ + { + name: "unmanaged ES has no error, and no observedGeneration update", + k8sClientFields: k8sClientFields{ + []runtime.Object{ + newBuilder("testES", "test"). + WithGeneration(2). + WithAnnotations(map[string]string{common.ManagedAnnotation: "false"}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).Build()}, + }, + args: args{ + request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "testES", + Namespace: "test", + }, + }, + }, + wantErr: false, + expected: newBuilder("testES", "test"). + WithGeneration(2). + WithAnnotations(map[string]string{common.ManagedAnnotation: "false"}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).BuildAndCopy(), + }, + { + name: "ES with too long name, fails initial reconcile, but has observedGeneration updated", + k8sClientFields: k8sClientFields{ + []runtime.Object{ + newBuilder("testESwithtoolongofanamereallylongname", "test"). + WithGeneration(2). + WithAnnotations(map[string]string{hints.OrchestrationsHintsAnnotation: `{"no_transient_settings":false}`}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).Build(), + }, + }, + args: args{ + request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "testESwithtoolongofanamereallylongname", + Namespace: "test", + }, + }, + }, + wantErr: false, + expected: newBuilder("testESwithtoolongofanamereallylongname", "test"). + WithGeneration(2). + WithAnnotations(map[string]string{hints.OrchestrationsHintsAnnotation: `{"no_transient_settings":false}`}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 2, Phase: esv1.ElasticsearchResourceInvalid, Health: esv1.ElasticsearchUnknownHealth}).BuildAndCopy(), + }, + { + name: "ES with too long name, and needing annotations update, fails initial reconcile, and does not have status.* updated because of a 409/resource conflict", + k8sClientFields: k8sClientFields{ + []runtime.Object{ + newBuilder("testESwithtoolongofanamereallylongname", "test"). + WithGeneration(2). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).Build()}, + }, + args: args{ + request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "testESwithtoolongofanamereallylongname", + Namespace: "test", + }, + }, + }, + wantErr: false, + expected: newBuilder("testESwithtoolongofanamereallylongname", "test"). + WithGeneration(2). + WithAnnotations(map[string]string{hints.OrchestrationsHintsAnnotation: `{"no_transient_settings":false}`}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).BuildAndCopy(), + }, + { + name: "Invalid ES version errors, and updates observedGeneration", + k8sClientFields: k8sClientFields{ + []runtime.Object{ + newBuilder("testES", "test"). + WithGeneration(2). + WithVersion("invalid"). + WithAnnotations(map[string]string{hints.OrchestrationsHintsAnnotation: `{"no_transient_settings":false}`}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 1}).Build()}, + }, + args: args{ + request: reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: "testES", + Namespace: "test", + }, + }, + }, + wantErr: false, + expected: newBuilder("testES", "test"). + WithGeneration(2). + WithVersion("invalid"). + WithAnnotations(map[string]string{hints.OrchestrationsHintsAnnotation: `{"no_transient_settings":false}`}). + WithStatus(esv1.ElasticsearchStatus{ObservedGeneration: 2, Phase: esv1.ElasticsearchResourceInvalid, Health: esv1.ElasticsearchUnknownHealth}).BuildAndCopy(), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := newTestReconciler(tt.k8sClientFields.objects...) + if _, err := r.Reconcile(context.Background(), tt.args.request); (err != nil) != tt.wantErr { + t.Errorf("ReconcileElasticsearch.Reconcile() error = %v, wantErr %v", err, tt.wantErr) + return + } + + var actualES esv1.Elasticsearch + if err := r.Client.Get(context.Background(), tt.args.request.NamespacedName, &actualES); err != nil { + t.Error(err) + return + } + comparison.AssertEqual(t, &actualES, &tt.expected) + }) + } +} diff --git a/pkg/controller/elasticsearch/reconcile/state.go b/pkg/controller/elasticsearch/reconcile/state.go index f3bafeabb7..ef51631443 100644 --- a/pkg/controller/elasticsearch/reconcile/state.go +++ b/pkg/controller/elasticsearch/reconcile/state.go @@ -22,8 +22,8 @@ import ( var log = ulog.Log.WithName("elasticsearch-controller") -// State holds the accumulated state during the reconcile loop including the response and a pointer to an -// Elasticsearch resource for status updates. +// State holds the accumulated state during the reconcile loop including the response and a copy of the +// Elasticsearch resource from the start of reconciliation, for status updates. type State struct { *events.Recorder cluster esv1.Elasticsearch @@ -38,6 +38,7 @@ func NewState(c esv1.Elasticsearch) (*State, error) { return nil, err } status := *c.Status.DeepCopy() + status.ObservedGeneration = c.Generation // reset the health to 'unknown' so that if reconciliation fails before the observer has had a chance to get it, // we stop reporting a health that may be out of date status.Health = esv1.ElasticsearchUnknownHealth diff --git a/pkg/controller/elasticsearch/reconcile/state_test.go b/pkg/controller/elasticsearch/reconcile/state_test.go index f56d35dca6..221d719112 100644 --- a/pkg/controller/elasticsearch/reconcile/state_test.go +++ b/pkg/controller/elasticsearch/reconcile/state_test.go @@ -201,6 +201,24 @@ func TestState_Apply(t *testing.T) { Phase: esv1.ElasticsearchApplyingChangesPhase, }, }, + { + name: "Status.observedGeneration is set from metadata.generation", + cluster: esv1.Elasticsearch{ + ObjectMeta: metav1.ObjectMeta{ + Generation: int64(1), + }, + }, + effects: func(s *State) { + s.UpdateElasticsearchApplyingChanges([]corev1.Pod{}) + }, + wantEvents: []events.Event{}, + wantStatus: &esv1.ElasticsearchStatus{ + ObservedGeneration: int64(1), + AvailableNodes: 0, + Health: esv1.ElasticsearchRedHealth, + Phase: esv1.ElasticsearchApplyingChangesPhase, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/test/e2e/test/elasticsearch/cluster_generation.go b/test/e2e/test/elasticsearch/cluster_generation.go new file mode 100644 index 0000000000..07cbb1597e --- /dev/null +++ b/test/e2e/test/elasticsearch/cluster_generation.go @@ -0,0 +1,86 @@ +// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one +// or more contributor license agreements. Licensed under the Elastic License 2.0; +// you may not use this file except in compliance with the Elastic License 2.0. + +package elasticsearch + +import ( + "context" + "errors" + "fmt" + + "k8s.io/apimachinery/pkg/types" + + esv1 "github.com/elastic/cloud-on-k8s/pkg/apis/elasticsearch/v1" + "github.com/elastic/cloud-on-k8s/test/e2e/test" +) + +func clusterGeneration(es esv1.Elasticsearch, k *test.K8sClient) (int64, error) { + if err := k.Client.Get(context.Background(), types.NamespacedName{Namespace: es.GetNamespace(), Name: es.GetName()}, &es); err != nil { + return 0, err + } + + return es.GetObjectMeta().GetGeneration(), nil +} + +func clusterObservedGeneration(es esv1.Elasticsearch, k *test.K8sClient) (int64, error) { + if err := k.Client.Get(context.Background(), types.NamespacedName{Namespace: es.GetNamespace(), Name: es.GetName()}, &es); err != nil { + return 0, err + } + + return es.Status.ObservedGeneration, nil +} + +// RetrieveClusterGenerationsStep stores the current metadata.Generation, and status.ObservedGeneration into the given fields. +func RetrieveClusterGenerationsStep(es esv1.Elasticsearch, k *test.K8sClient, generation, observedGeneration *int64) test.Step { + return test.Step{ + Name: "Retrieve Elasticsearch metadata.Generation, and status.ObservedGeneration for comparison purpose", + Test: test.Eventually(func() error { + clusterGeneration, err := clusterGeneration(es, k) + if err != nil { + return err + } + *generation = clusterGeneration + clusterObservedGeneration, err := clusterObservedGeneration(es, k) + if err != nil { + return err + } + *observedGeneration = clusterObservedGeneration + return nil + }), + } +} + +// CompareClusterGenerations compares the current metadata.generation, and status.observedGeneration +// and fails if they don't match expectations. +func CompareClusterGenerationsStep(es esv1.Elasticsearch, k *test.K8sClient, previousClusterGeneration, previousClusterObservedGeneration *int64) test.Step { + return test.Step{ + Name: "Cluster metadata.generation, and status.observedGeneration should have been incremented from previous state, and should be equal", + Test: test.Eventually(func() error { + newClusterGeneration, err := clusterGeneration(es, k) + if err != nil { + return err + } + if newClusterGeneration == 0 { + return errors.New("expected metadata.generation to not be empty") + } + newClusterObservedGeneration, err := clusterObservedGeneration(es, k) + if err != nil { + return err + } + if newClusterObservedGeneration == 0 { + return errors.New("expected status.observedGeneration to not be empty") + } + if newClusterGeneration <= *previousClusterGeneration { + return errors.New("expected metadata.generation to be incremented") + } + if newClusterObservedGeneration <= *previousClusterObservedGeneration { + return errors.New("expected status.observedGeneration to be incremented") + } + if newClusterGeneration != newClusterObservedGeneration { + return fmt.Errorf("expected metadata.generation and status.observedGeneration to be equal; generation: %d, observedGeneration: %d", newClusterGeneration, newClusterObservedGeneration) + } + return nil + }), + } +} diff --git a/test/e2e/test/elasticsearch/steps_mutation.go b/test/e2e/test/elasticsearch/steps_mutation.go index b84397e9f4..b23463648f 100644 --- a/test/e2e/test/elasticsearch/steps_mutation.go +++ b/test/e2e/test/elasticsearch/steps_mutation.go @@ -71,6 +71,7 @@ func (b Builder) UpgradeTestSteps(k *test.K8sClient) test.StepList { func (b Builder) MutationTestSteps(k *test.K8sClient) test.StepList { var clusterIDBeforeMutation string + var clusterGenerationBeforeMutation, clusterObservedGenerationBeforeMutation int64 var continuousHealthChecks *ContinuousHealthCheck var dataIntegrityCheck *DataIntegrityCheck mutatedFrom := b.MutatedFrom @@ -109,6 +110,7 @@ func (b Builder) MutationTestSteps(k *test.K8sClient) test.StepList { masterChangeBudgetWatcher.StartStep(k), changeBudgetWatcher.StartStep(k), RetrieveClusterUUIDStep(b.Elasticsearch, k, &clusterIDBeforeMutation), + RetrieveClusterGenerationsStep(b.Elasticsearch, k, &clusterGenerationBeforeMutation, &clusterObservedGenerationBeforeMutation), }. WithSteps(AnnotatePodsWithBuilderHash(*mutatedFrom, k)). WithSteps(b.UpgradeTestSteps(k)). @@ -116,6 +118,7 @@ func (b Builder) MutationTestSteps(k *test.K8sClient) test.StepList { WithSteps(b.CheckStackTestSteps(k)). WithSteps(test.StepList{ CompareClusterUUIDStep(b.Elasticsearch, k, &clusterIDBeforeMutation), + CompareClusterGenerationsStep(b.Elasticsearch, k, &clusterGenerationBeforeMutation, &clusterObservedGenerationBeforeMutation), masterChangeBudgetWatcher.StopStep(k), changeBudgetWatcher.StopStep(k), test.Step{