From 70941f65bf69a39d95b91ecccd6c5112b224789f Mon Sep 17 00:00:00 2001
From: Harry Zhang <harryz@hyper.sh>
Date: Thu, 19 Jan 2017 14:30:13 +0800
Subject: [PATCH] Do not swallow error in volume

---
 .../cache/actual_state_of_world.go            | 21 +++++---
 .../cache/actual_state_of_world_test.go       | 52 +++++++++++++++++++
 2 files changed, 65 insertions(+), 8 deletions(-)

diff --git a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go
index f092217b092..5387bec0d9d 100644
--- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go
+++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go
@@ -452,25 +452,28 @@ func (asw *actualStateOfWorld) addVolumeToReportAsAttached(
 // needs to be updated again by the node status updater.
 // If the specifed node does not exist in the nodesToUpdateStatusFor list, log the error and return
 // This is an internal function and caller should acquire and release the lock
-func (asw *actualStateOfWorld) updateNodeStatusUpdateNeeded(nodeName types.NodeName, needed bool) {
+func (asw *actualStateOfWorld) updateNodeStatusUpdateNeeded(nodeName types.NodeName, needed bool) error {
 	nodeToUpdate, nodeToUpdateExists := asw.nodesToUpdateStatusFor[nodeName]
 	if !nodeToUpdateExists {
 		// should not happen
-		glog.Errorf(
-			"Failed to set statusUpdateNeeded to needed %t because nodeName=%q  does not exist",
-			needed,
-			nodeName)
-		return
+		errMsg := fmt.Sprintf("Failed to set statusUpdateNeeded to needed %t because nodeName=%q  does not exist",
+			needed, nodeName)
+		glog.Errorf(errMsg)
+		return fmt.Errorf(errMsg)
 	}
 
 	nodeToUpdate.statusUpdateNeeded = needed
 	asw.nodesToUpdateStatusFor[nodeName] = nodeToUpdate
+
+	return nil
 }
 
 func (asw *actualStateOfWorld) SetNodeStatusUpdateNeeded(nodeName types.NodeName) {
 	asw.Lock()
 	defer asw.Unlock()
-	asw.updateNodeStatusUpdateNeeded(nodeName, true)
+	if err := asw.updateNodeStatusUpdateNeeded(nodeName, true); err != nil {
+		glog.Errorf("Failed to update statusUpdateNeeded field in actual state of world: %v", err)
+	}
 }
 
 func (asw *actualStateOfWorld) DeleteVolumeNode(
@@ -589,7 +592,9 @@ func (asw *actualStateOfWorld) GetVolumesToReportAttached() map[types.NodeName][
 		// When GetVolumesToReportAttached is called by node status updater, the current status
 		// of this node will be updated, so set the flag statusUpdateNeeded to false indicating
 		// the current status is already updated.
-		asw.updateNodeStatusUpdateNeeded(nodeName, false)
+		if err := asw.updateNodeStatusUpdateNeeded(nodeName, false); err != nil {
+			glog.Errorf("Failed to update statusUpdateNeeded field when getting volumes: %v", err)
+		}
 	}
 
 	return volumesToReportAttached
diff --git a/pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go b/pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go
index c3423c1e2e6..86f04614933 100644
--- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go
+++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go
@@ -1113,6 +1113,58 @@ func Test_SetNodeStatusUpdateNeededError(t *testing.T) {
 	}
 }
 
+// Test_updateNodeStatusUpdateNeeded expects statusUpdateNeeded to be properly updated if
+// updateNodeStatusUpdateNeeded is called on a node that exists in the actual state of the world
+func Test_updateNodeStatusUpdateNeeded(t *testing.T) {
+	// Arrange
+	volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t)
+	asw := &actualStateOfWorld{
+		attachedVolumes:        make(map[v1.UniqueVolumeName]attachedVolume),
+		nodesToUpdateStatusFor: make(map[types.NodeName]nodeToUpdateStatusFor),
+		volumePluginMgr:        volumePluginMgr,
+	}
+	nodeName := types.NodeName("node-1")
+	nodeToUpdate := nodeToUpdateStatusFor{
+		nodeName:                  nodeName,
+		statusUpdateNeeded:        true,
+		volumesToReportAsAttached: make(map[v1.UniqueVolumeName]v1.UniqueVolumeName),
+	}
+	asw.nodesToUpdateStatusFor[nodeName] = nodeToUpdate
+
+	// Act
+	err := asw.updateNodeStatusUpdateNeeded(nodeName, false)
+
+	// Assert
+	if err != nil {
+		t.Fatalf("updateNodeStatusUpdateNeeded should not return error, but got: %v", err)
+	}
+	nodesToUpdateStatusFor := asw.GetNodesToUpdateStatusFor()
+	if nodesToUpdateStatusFor[nodeName].statusUpdateNeeded {
+		t.Fatalf("nodesToUpdateStatusFor should be updated to: false, but got: true")
+	}
+}
+
+// Test_updateNodeStatusUpdateNeededError expects statusUpdateNeeded to report error if
+// updateNodeStatusUpdateNeeded is called on a node that does not exist in the actual state of the world
+func Test_updateNodeStatusUpdateNeededError(t *testing.T) {
+	// Arrange
+	volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t)
+	asw := &actualStateOfWorld{
+		attachedVolumes:        make(map[v1.UniqueVolumeName]attachedVolume),
+		nodesToUpdateStatusFor: make(map[types.NodeName]nodeToUpdateStatusFor),
+		volumePluginMgr:        volumePluginMgr,
+	}
+	nodeName := types.NodeName("node-1")
+
+	// Act
+	err := asw.updateNodeStatusUpdateNeeded(nodeName, false)
+
+	// Assert
+	if err == nil {
+		t.Fatalf("updateNodeStatusUpdateNeeded should return error, but got nothing")
+	}
+}
+
 func verifyAttachedVolume(
 	t *testing.T,
 	attachedVolumes []AttachedVolume,