From 03f5a7d0a99f5677cc7eee45af343acdcbf3f9e9 Mon Sep 17 00:00:00 2001 From: Eric Wolinetz Date: Wed, 22 May 2019 10:55:26 -0500 Subject: [PATCH 1/2] Fixing checking if nodes are different when scaling from 3 to 4. fixing ordering of nodes defined to prevent prior node recreation --- pkg/k8shandler/logstore.go | 20 ++++++---- pkg/k8shandler/logstore_test.go | 70 +++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/pkg/k8shandler/logstore.go b/pkg/k8shandler/logstore.go index f6967f481..831aba655 100644 --- a/pkg/k8shandler/logstore.go +++ b/pkg/k8shandler/logstore.go @@ -121,14 +121,6 @@ func (cluster *ClusterLogging) newElasticsearchCR(elasticsearchName string) *ela if cluster.Spec.LogStore.NodeCount > 3 { - dataNode := elasticsearch.ElasticsearchNode{ - Roles: []elasticsearch.ElasticsearchNodeRole{"client", "data"}, - NodeCount: cluster.Spec.LogStore.NodeCount - 3, - Storage: cluster.Spec.LogStore.ElasticsearchSpec.Storage, - } - - esNodes = append(esNodes, dataNode) - masterNode := elasticsearch.ElasticsearchNode{ Roles: []elasticsearch.ElasticsearchNodeRole{"client", "data", "master"}, NodeCount: 3, @@ -137,6 +129,14 @@ func (cluster *ClusterLogging) newElasticsearchCR(elasticsearchName string) *ela esNodes = append(esNodes, masterNode) + dataNode := elasticsearch.ElasticsearchNode{ + Roles: []elasticsearch.ElasticsearchNodeRole{"client", "data"}, + NodeCount: cluster.Spec.LogStore.NodeCount - 3, + Storage: cluster.Spec.LogStore.ElasticsearchSpec.Storage, + } + + esNodes = append(esNodes, dataNode) + } else { esNode := elasticsearch.ElasticsearchNode{ @@ -283,6 +283,10 @@ func areNodesDifferent(current, desired []elasticsearch.ElasticsearchNode) ([]el return desired, true } + if len(current) != len(desired) { + return desired, true + } + foundRoleMatch := false for nodeIndex := 0; nodeIndex < len(desired); nodeIndex++ { for _, node := range current { diff --git a/pkg/k8shandler/logstore_test.go b/pkg/k8shandler/logstore_test.go index 1aeab71d1..cd449ac0f 100644 --- a/pkg/k8shandler/logstore_test.go +++ b/pkg/k8shandler/logstore_test.go @@ -303,3 +303,73 @@ func createAndCheckSingleNodeWithNodeCount(t *testing.T, expectedNodeCount int32 } } } + +func TestDifferenceFoundWhenNodeCountExceeds3(t *testing.T) { + cluster := NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 3, + }, + }, + }, + }, + ) + elasticsearchCR := cluster.newElasticsearchCR("test-app-name") + + cluster = NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 4, + }, + }, + }, + }, + ) + elasticsearchCR2 := cluster.newElasticsearchCR("test-app-name") + + _, different := isElasticsearchCRDifferent(elasticsearchCR, elasticsearchCR2) + if !different { + t.Errorf("Expected that difference would be found due to node count change") + } +} + +func TestDifferenceFoundWhenNodeCountExceeds4(t *testing.T) { + cluster := NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 4, + }, + }, + }, + }, + ) + elasticsearchCR := cluster.newElasticsearchCR("test-app-name") + + cluster = NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 5, + }, + }, + }, + }, + ) + elasticsearchCR2 := cluster.newElasticsearchCR("test-app-name") + + _, different := isElasticsearchCRDifferent(elasticsearchCR, elasticsearchCR2) + if !different { + t.Errorf("Expected that difference would be found due to node count change") + } +} From b3c80028deb34affbab8b741f6a173a940c7b123 Mon Sep 17 00:00:00 2001 From: Eric Wolinetz Date: Wed, 26 Jun 2019 14:36:40 -0500 Subject: [PATCH 2/2] Ensure we preserve GenUUID for nodes when doing diffs --- pkg/k8shandler/logstore.go | 18 +++++-- pkg/k8shandler/logstore_test.go | 83 +++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/pkg/k8shandler/logstore.go b/pkg/k8shandler/logstore.go index 831aba655..03c344c7e 100644 --- a/pkg/k8shandler/logstore.go +++ b/pkg/k8shandler/logstore.go @@ -283,17 +283,19 @@ func areNodesDifferent(current, desired []elasticsearch.ElasticsearchNode) ([]el return desired, true } - if len(current) != len(desired) { - return desired, true - } - foundRoleMatch := false for nodeIndex := 0; nodeIndex < len(desired); nodeIndex++ { for _, node := range current { if areNodeRolesSame(node, desired[nodeIndex]) { - if updatedNode, isDifferent := isNodeDifferent(node, desired[nodeIndex]); isDifferent { + updatedNode, isDifferent := isNodeDifferent(node, desired[nodeIndex]) + if isDifferent { desired[nodeIndex] = updatedNode different = true + } else { + // ensure that we are setting the GenUUID if it existed + if desired[nodeIndex].GenUUID == nil { + desired[nodeIndex].GenUUID = updatedNode.GenUUID + } } foundRoleMatch = true } @@ -305,6 +307,12 @@ func areNodesDifferent(current, desired []elasticsearch.ElasticsearchNode) ([]el different = true } + // we don't use this to shortcut because the above loop will help to preserve + // any generated UUIDs + if len(current) != len(desired) { + return desired, true + } + return desired, different } diff --git a/pkg/k8shandler/logstore_test.go b/pkg/k8shandler/logstore_test.go index cd449ac0f..97c77deb0 100644 --- a/pkg/k8shandler/logstore_test.go +++ b/pkg/k8shandler/logstore_test.go @@ -6,6 +6,7 @@ import ( logging "github.com/openshift/cluster-logging-operator/pkg/apis/logging/v1" elasticsearch "github.com/openshift/elasticsearch-operator/pkg/apis/elasticsearch/v1" + esutils "github.com/openshift/elasticsearch-operator/test/utils" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" ) @@ -373,3 +374,85 @@ func TestDifferenceFoundWhenNodeCountExceeds4(t *testing.T) { t.Errorf("Expected that difference would be found due to node count change") } } + +func TestGenUUIDPreservedWhenNodeCountExceeds4(t *testing.T) { + cluster := NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 3, + }, + }, + }, + }, + ) + elasticsearchCR := cluster.newElasticsearchCR("test-app-name") + dataUUID := esutils.GenerateUUID() + elasticsearchCR.Spec.Nodes[0].GenUUID = &dataUUID + + cluster = NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 4, + }, + }, + }, + }, + ) + elasticsearchCR2 := cluster.newElasticsearchCR("test-app-name") + + diffCR, different := isElasticsearchCRDifferent(elasticsearchCR, elasticsearchCR2) + if !different { + t.Errorf("Expected that difference would be found due to node count change") + } + + if diffCR.Spec.Nodes[0].GenUUID == nil || *diffCR.Spec.Nodes[0].GenUUID != dataUUID { + t.Errorf("Expected that original GenUUID would be preserved as %v but was %v", dataUUID, diffCR.Spec.Nodes[0].GenUUID) + } +} + +func TestGenUUIDPreservedWhenNodeCountChanges(t *testing.T) { + cluster := NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 1, + }, + }, + }, + }, + ) + elasticsearchCR := cluster.newElasticsearchCR("test-app-name") + dataUUID := esutils.GenerateUUID() + elasticsearchCR.Spec.Nodes[0].GenUUID = &dataUUID + + cluster = NewClusterLogging( + &logging.ClusterLogging{ + Spec: logging.ClusterLoggingSpec{ + LogStore: logging.LogStoreSpec{ + Type: "elasticsearch", + ElasticsearchSpec: logging.ElasticsearchSpec{ + NodeCount: 3, + }, + }, + }, + }, + ) + elasticsearchCR2 := cluster.newElasticsearchCR("test-app-name") + + diffCR, different := isElasticsearchCRDifferent(elasticsearchCR, elasticsearchCR2) + if !different { + t.Errorf("Expected that difference would be found due to node count change") + } + + if diffCR.Spec.Nodes[0].GenUUID == nil || *diffCR.Spec.Nodes[0].GenUUID != dataUUID { + t.Errorf("Expected that original GenUUID would be preserved as %v but was %v", dataUUID, diffCR.Spec.Nodes[0].GenUUID) + } +}