Skip to content

Commit

Permalink
Node status updater now deletes the node entry in attach updates when…
Browse files Browse the repository at this point in the history
… node is missing in NodeInformer cache. Fixes kubernetes#42438.

- Added RemoveNodeFromAttachUpdates as part of node status updater operations.
  • Loading branch information
verult committed May 24, 2017
1 parent 3776d6b commit 256469e
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 5 deletions.
17 changes: 17 additions & 0 deletions pkg/controller/volume/attachdetach/cache/actual_state_of_world.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ 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.
Expand Down Expand Up @@ -254,6 +258,19 @@ 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(
volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) (api.UniqueVolumeName, error) {
asw.Lock()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1113,6 +1113,89 @@ func Test_SetNodeStatusUpdateNeededError(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[api.UniqueVolumeName]attachedVolume),
nodesToUpdateStatusFor: make(map[types.NodeName]nodeToUpdateStatusFor),
volumePluginMgr: volumePluginMgr,
}
nodeName := types.NodeName("node-1")
nodeToUpdate := nodeToUpdateStatusFor{
nodeName: nodeName,
statusUpdateNeeded: true,
volumesToReportAsAttached: make(map[api.UniqueVolumeName]api.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[api.UniqueVolumeName]attachedVolume),
nodesToUpdateStatusFor: make(map[types.NodeName]nodeToUpdateStatusFor),
volumePluginMgr: volumePluginMgr,
}
nodeName := types.NodeName("node-1")
nodeToUpdate := nodeToUpdateStatusFor{
nodeName: nodeName,
statusUpdateNeeded: true,
volumesToReportAsAttached: make(map[api.UniqueVolumeName]api.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[api.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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,20 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
nodesToUpdate := nsu.actualStateOfWorld.GetVolumesToReportAttached()
for nodeName, attachedVolumes := range nodesToUpdate {
nodeObj, exists, err := nsu.nodeInformer.GetStore().GetByKey(string(nodeName))
if nodeObj == nil || !exists || err != nil {
// If node does not exist, its status cannot be updated, log error and
// reset flag statusUpdateNeeded back to true to indicate this node status
// needs to be updated again
if nodeObj == nil || !exists {
// 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.
glog.V(2).Infof(
"Could not update node status. Failed to find node %q in NodeInformer cache. %v",
"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 {
// Log error and reset flag statusUpdateNeeded back to true
// to indicate this node status needs to be updated again.
glog.V(2).Infof("Error retrieving nodes from node lister. Error: %v", err)
nsu.actualStateOfWorld.SetNodeStatusUpdateNeeded(nodeName)
continue
}
Expand Down

0 comments on commit 256469e

Please sign in to comment.