Merge pull request #40126 from resouer/return-value

Automatic merge from submit-queue (batch tested with PRs 40126, 40565, 38777, 40564, 40572)

Do not swallow error in asw.updateNodeStatusUpdateNeeded

Ref #39056

Bubble the error up to `SetNodeUpdateStatusNeeded` and log it out.

NOTE: This does not modify interface of `SetNodeUpdateStatusNeeded`

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(
+		errMsg := fmt.Sprintf("Failed to set statusUpdateNeeded to needed %t because nodeName=%q  does not exist",
-			"Failed to set statusUpdateNeeded to needed %t because nodeName=%q  does not exist",
+			needed, nodeName)
-			needed,
+		glog.Errorf(errMsg)
-			nodeName)
+		return fmt.Errorf(errMsg)
-		return
 	}
 
 	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

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,