Merge pull request #50806 from verult/VolumeNotYetAttached

Automatic merge from submit-queue (batch tested with PRs 50806, 48789, 49922, 49935, 50438)

On AttachDetachController node status update, do not retry when node …

…doesn't exist but keep the node entry in cache.



**What this PR does / why we need it**: An alternative fix for https://github.com/kubernetes/kubernetes/issues/42438 which also fixes #50721.

Instead of removing the node entry entirely from the node status update cache (which prevents the node from ever being updated even when it recovers), here the node status updater does nothing, so that there won't be an update retry until the node is re-added, where the cache entry is set to true.

Will cherry pick to prior versions after this is merged.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #50721 

**Release Note**:
``` release-note
On AttachDetachController node status update, do not retry when node doesn't exist but keep the node entry in cache.
```

/assign @jingxu97 
/cc @saad-ali 
/sig storage
/release-note

pkg/controller/volume/attachdetach/cache/actual_state_of_world.go

@@ -125,10 +125,6 @@ type ActualStateOfWorld interface {
 
 	// GetNodesToUpdateStatusFor returns the map of nodeNames to nodeToUpdateStatusFor
 	GetNodesToUpdateStatusFor() map[types.NodeName]nodeToUpdateStatusFor
-
-	// Removes the given node from the record of attach updates. The node's entire
-	// volumesToReportAsAttached list is removed.
-	RemoveNodeFromAttachUpdates(nodeName types.NodeName) error
 }
 
 // AttachedVolume represents a volume that is attached to a node.
@@ -264,19 +260,6 @@ func (asw *actualStateOfWorld) AddVolumeToReportAsAttached(
 	asw.addVolumeToReportAsAttached(volumeName, nodeName)
 }
 
-func (asw *actualStateOfWorld) RemoveNodeFromAttachUpdates(nodeName types.NodeName) error {
-	asw.Lock()
-	defer asw.Unlock()
-
-	_, nodeToUpdateExists := asw.nodesToUpdateStatusFor[nodeName]
-	if nodeToUpdateExists {
-		delete(asw.nodesToUpdateStatusFor, nodeName)
-		return nil
-	}
-	return fmt.Errorf("node %q does not exist in volumesToReportAsAttached list",
-		nodeName)
-}
-
 func (asw *actualStateOfWorld) AddVolumeNode(
 	uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) (v1.UniqueVolumeName, error) {
 	asw.Lock()

pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go

@@ -1165,89 +1165,6 @@ func Test_updateNodeStatusUpdateNeededError(t *testing.T) {
 	}
 }
 
-// Test_RemoveNodeFromAttachUpdates_Positive expects an entire node entry to be removed
-// from nodesToUpdateStatusFor
-func Test_RemoveNodeFromAttachUpdates_Positive(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.RemoveNodeFromAttachUpdates(nodeName)
-
-	// Assert
-	if err != nil {
-		t.Fatalf("RemoveNodeFromAttachUpdates should not return error, but got: %v", err)
-	}
-	if len(asw.nodesToUpdateStatusFor) > 0 {
-		t.Fatal("nodesToUpdateStatusFor should be empty as its only entry has been deleted.")
-	}
-}
-
-// Test_RemoveNodeFromAttachUpdates_Negative_NodeDoesNotExist expects an error to be thrown
-// when nodeName is not in nodesToUpdateStatusFor.
-func Test_RemoveNodeFromAttachUpdates_Negative_NodeDoesNotExist(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.RemoveNodeFromAttachUpdates("node-2")
-
-	// Assert
-	if err == nil {
-		t.Fatal("RemoveNodeFromAttachUpdates should return an error as the nodeName doesn't exist.")
-	}
-	if len(asw.nodesToUpdateStatusFor) != 1 {
-		t.Fatal("The length of nodesToUpdateStatusFor should not change because no operation was performed.")
-	}
-}
-
-// Test_RemoveNodeFromAttachUpdates_Negative_Empty expects an error to be thrown
-// when a nodesToUpdateStatusFor is empty.
-func Test_RemoveNodeFromAttachUpdates_Negative_Empty(t *testing.T) {
-	// Arrange
-	volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t)
-	asw := &actualStateOfWorld{
-		attachedVolumes:        make(map[v1.UniqueVolumeName]attachedVolume),
-		nodesToUpdateStatusFor: make(map[types.NodeName]nodeToUpdateStatusFor),
-		volumePluginMgr:        volumePluginMgr,
-	}
-
-	// Act
-	err := asw.RemoveNodeFromAttachUpdates("node-1")
-
-	// Assert
-	if err == nil {
-		t.Fatal("RemoveNodeFromAttachUpdates should return an error as nodeToUpdateStatusFor is empty.")
-	}
-	if len(asw.nodesToUpdateStatusFor) != 0 {
-		t.Fatal("The length of nodesToUpdateStatusFor should be 0.")
-	}
-}
-
 func verifyAttachedVolume(
 	t *testing.T,
 	attachedVolumes []AttachedVolume,

pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go

@@ -68,13 +68,11 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
 		nodeObj, err := nsu.nodeLister.Get(string(nodeName))
 		if errors.IsNotFound(err) {
 			// If node does not exist, its status cannot be updated.
-			// Remove the node entry from the collection of attach updates, preventing the
-			// status updater from unnecessarily updating the node.
+			// Do nothing so that there is no retry until node is created.
 			glog.V(2).Infof(
 				"Could not update node status. Failed to find node %q in NodeInformer cache. Error: '%v'",
 				nodeName,
 				err)
-			nsu.actualStateOfWorld.RemoveNodeFromAttachUpdates(nodeName)
 			continue
 		} else if err != nil {
 			// For all other errors, log error and reset flag statusUpdateNeeded