Merge pull request #188 from ewolinetz/cherrypick_185

[release-4.1] Bug 1712955: Fixing checking if nodes are different when scaling from 3 to 4
openshift · Aug 12, 2019 · a22f85e · a22f85e
2 parents dbfd918 + b3c8002
commit a22f85e
Show file tree

Hide file tree

Showing 2 changed files with 174 additions and 9 deletions.
diff --git 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{
@@ -287,9 +287,15 @@ func areNodesDifferent(current, desired []elasticsearch.ElasticsearchNode) ([]el
 	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
 			}
@@ -301,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
@@ -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"
 )
@@ -303,3 +304,155 @@ 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")
+	}
+}
+
+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)
+	}
+}