From 1234d2f500c63bf4d61f17f05e3c35a9da11d257 Mon Sep 17 00:00:00 2001 From: Cheng Xing Date: Wed, 16 Aug 2017 15:35:33 -0700 Subject: [PATCH] On AttachDetachController node status update, do not retry when node doesn't exist but keep the node entry in cache --- .../cache/actual_state_of_world.go | 17 ---- .../cache/actual_state_of_world_test.go | 83 ------------------- .../statusupdater/node_status_updater.go | 4 +- 3 files changed, 1 insertion(+), 103 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 5e96b4c433..4ee4e26c9b 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/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() 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 dcc9fd837b..22a4e3df72 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 @@ -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, diff --git a/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go b/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go index acfc2b97bb..2ebb1706e9 100644 --- a/pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go +++ b/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