Merge pull request istio-ecosystem#317 from adilfulara/oss-MESH-5373

Bug: Node locality is missing occasionally.
aattuluri · Aug 14, 2024 · 0e8cee3 · 0e8cee3
2 parents f79da4f + e18ee67
commit 0e8cee3
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 22 deletions.
diff --git a/admiral/pkg/controller/admiral/node.go b/admiral/pkg/controller/admiral/node.go
@@ -57,19 +57,29 @@ func NewNodeController(stopCh <-chan struct{}, handler NodeHandler, config *rest
 	return &nodeController, nil
 }
 
-func (p *NodeController) Added(ctx context.Context, obj interface{}) error {
+func process(ctx context.Context, obj interface{}) (string, error) {
 	node, ok := obj.(*k8sV1.Node)
 	if !ok {
-		return fmt.Errorf("type assertion failed, %v is not of type *v1.Node", obj)
+		return "", fmt.Errorf("type assertion failed, %v is not of type *v1.Node", obj)
 	}
-	if p.Locality == nil {
-		p.Locality = &Locality{Region: common.GetNodeLocality(node)}
+	return common.GetNodeLocality(node), nil
+}
+
+func (d *NodeController) Added(ctx context.Context, obj interface{}) error {
+	region, err := process(ctx, obj)
+	if err != nil {
+		return err
 	}
+	d.Locality = &Locality{Region: region}
 	return nil
 }
 
-func (p *NodeController) Updated(ctx context.Context, obj interface{}, oldObj interface{}) error {
-	//ignore
+func (d *NodeController) Updated(ctx context.Context, obj interface{}, oldObj interface{}) error {
+	region, err := process(ctx, obj)
+	if err != nil {
+		return err
+	}
+	d.Locality = &Locality{Region: region}
 	return nil
 }
 

diff --git a/admiral/pkg/controller/admiral/node_test.go b/admiral/pkg/controller/admiral/node_test.go
@@ -102,31 +102,23 @@ func TestNodeAddUpdateDelete(t *testing.T) {
 
 	if nodeController == nil {
 		t.Errorf("Node controller should never be nil without an error thrown")
+		return
 	}
 	region := "us-west-2"
 	nodeObj := &k8sV1.Node{Spec: k8sV1.NodeSpec{}, ObjectMeta: v1.ObjectMeta{Labels: map[string]string{common.NodeRegionLabel: region}}}
 
 	ctx := context.Background()
 
-	nodeController.Added(ctx, nodeObj)
+	_ = nodeController.Added(ctx, nodeObj)
+	assert.Equal(t, "us-west-2", nodeController.Locality.Region, "region expected %v, got: %v", region, nodeController.Locality.Region)
 
-	locality := nodeController.Locality
+	nodeObj.Labels[common.NodeRegionLabel] = "us-east-2"
+	_ = nodeController.Updated(ctx, nodeObj, nodeObj)
+	assert.Equal(t, "us-east-2", nodeController.Locality.Region, "region expected %v, got: %v", region, nodeController.Locality.Region)
 
-	if locality.Region != region {
-		t.Errorf("region expected %v, got: %v", region, locality.Region)
-	}
-
-	nodeController.Updated(ctx, nodeObj, nodeObj)
-	//update should make no difference
-	if locality.Region != region {
-		t.Errorf("region expected %v, got: %v", region, locality.Region)
-	}
-
-	nodeController.Deleted(ctx, nodeObj)
+	_ = nodeController.Deleted(ctx, nodeObj)
 	//delete should make no difference
-	if locality.Region != region {
-		t.Errorf("region expected %v, got: %v", region, locality.Region)
-	}
+	assert.Equal(t, "us-east-2", nodeController.Locality.Region, "region expected %v, got: %v", region, nodeController.Locality.Region)
 }
 
 // TODO: This is just a placeholder for when we add diff check for other types