From cae36e49a6b17c93658047ce27cf28abffe611a6 Mon Sep 17 00:00:00 2001
From: Mahmood Ali <mahmood@hashicorp.com>
Date: Tue, 11 Dec 2018 12:20:51 -0500
Subject: [PATCH] client: update driver info on new fingerprint

Fixes a bug where a driver health and attributes are never updated from
their initial status.  If a driver started unhealthy, it may never go
into a healthy status.
---
 client/client.go      |  5 +--
 client/client_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/client/client.go b/client/client.go
index ce2e87573367..019234460939 100644
--- a/client/client.go
+++ b/client/client.go
@@ -1154,7 +1154,6 @@ func (c *Client) updateNodeFromDriver(name string, info *structs.DriverInfo) *st
 	if !hadDriver {
 		// If the driver info has not yet been set, do that here
 		hasChanged = true
-		c.config.Node.Drivers[name] = info
 		for attrName, newVal := range info.Attributes {
 			c.config.Node.Attributes[attrName] = newVal
 		}
@@ -1163,11 +1162,11 @@ func (c *Client) updateNodeFromDriver(name string, info *structs.DriverInfo) *st
 		// The driver info has already been set, fix it up
 		if oldVal.Detected != info.Detected {
 			hasChanged = true
-			c.config.Node.Drivers[name].Detected = info.Detected
 		}
 
 		if oldVal.Healthy != info.Healthy || oldVal.HealthDescription != info.HealthDescription {
 			hasChanged = true
+
 			if info.HealthDescription != "" {
 				event := &structs.NodeEvent{
 					Subsystem: "Driver",
@@ -1186,6 +1185,7 @@ func (c *Client) updateNodeFromDriver(name string, info *structs.DriverInfo) *st
 			}
 
 			hasChanged = true
+
 			if newVal == "" {
 				delete(c.config.Node.Attributes, attrName)
 			} else {
@@ -1205,6 +1205,7 @@ func (c *Client) updateNodeFromDriver(name string, info *structs.DriverInfo) *st
 	}
 
 	if hasChanged {
+		c.config.Node.Drivers[name] = info
 		c.config.Node.Drivers[name].UpdateTime = time.Now()
 		c.updateNodeLocked()
 	}
diff --git a/client/client_test.go b/client/client_test.go
index 3dadc3ce6505..e5e68ed1c361 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -1183,3 +1183,80 @@ func TestClient_computeAllocatedDeviceStats(t *testing.T) {
 
 	assert.EqualValues(t, expected, result)
 }
+
+func TestClient_updateNodeFromDriverUpdatesAll(t *testing.T) {
+	t.Parallel()
+	client, cleanup := TestClient(t, nil)
+	defer cleanup()
+
+	// initial update
+	{
+		info := &structs.DriverInfo{
+			Detected:          true,
+			Healthy:           false,
+			HealthDescription: "not healthy at start",
+			Attributes: map[string]string{
+				"node.mock.testattr1": "val1",
+			},
+		}
+		n := client.updateNodeFromDriver("mock", info)
+
+		updatedInfo := *n.Drivers["mock"]
+		// compare without update time
+		updatedInfo.UpdateTime = info.UpdateTime
+		assert.EqualValues(t, updatedInfo, *info)
+
+		// check node attributes
+		assert.Equal(t, "val1", n.Attributes["node.mock.testattr1"])
+	}
+
+	// initial update
+	{
+		info := &structs.DriverInfo{
+			Detected:          true,
+			Healthy:           true,
+			HealthDescription: "healthy",
+			Attributes: map[string]string{
+				"node.mock.testattr1": "val2",
+			},
+		}
+		n := client.updateNodeFromDriver("mock", info)
+
+		updatedInfo := *n.Drivers["mock"]
+		// compare without update time
+		updatedInfo.UpdateTime = info.UpdateTime
+		assert.EqualValues(t, updatedInfo, *info)
+
+		// check node attributes are updated
+		assert.Equal(t, "val2", n.Attributes["node.mock.testattr1"])
+
+		// update once more with the same info, updateTime shouldn't change
+		un := client.updateNodeFromDriver("mock", info)
+		assert.EqualValues(t, n, un)
+	}
+
+	// update once more to unhealthy because why not
+	{
+		info := &structs.DriverInfo{
+			Detected:          true,
+			Healthy:           false,
+			HealthDescription: "lost track",
+			Attributes: map[string]string{
+				"node.mock.testattr1": "",
+			},
+		}
+		n := client.updateNodeFromDriver("mock", info)
+
+		updatedInfo := *n.Drivers["mock"]
+		// compare without update time
+		updatedInfo.UpdateTime = info.UpdateTime
+		assert.EqualValues(t, updatedInfo, *info)
+
+		// check node attributes are updated
+		assert.Equal(t, "", n.Attributes["node.mock.testattr1"])
+
+		// update once more with the same info, updateTime shouldn't change
+		un := client.updateNodeFromDriver("mock", info)
+		assert.EqualValues(t, n, un)
+	}
+}