diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index 7a8fdb99b5e..d2dd7b3de62 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller.go @@ -632,7 +632,7 @@ func (adc *attachDetachController) syncPVCByKey(key string) error { func (adc *attachDetachController) processVolumesInUse( nodeName types.NodeName, volumesInUse []v1.UniqueVolumeName) { klog.V(4).Infof("processVolumesInUse for node %q", nodeName) - for _, attachedVolume := range adc.actualStateOfWorld.GetAllVolumesForNode(nodeName) { + for _, attachedVolume := range adc.actualStateOfWorld.GetAttachedVolumesForNode(nodeName) { mounted := false for _, volumeInUse := range volumesInUse { if attachedVolume.VolumeName == volumeInUse { diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go index 58894ddde8a..ec66fab1f8e 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go @@ -278,7 +278,7 @@ func attachDetachRecoveryTestCase(t *testing.T, extraPods1 []*v1.Pod, extraPods2 var detachedVolumesNum int = 0 time.Sleep(time.Second * 1) // Wait for a second - for _, volumeList := range testPlugin.GetAllVolumes() { + for _, volumeList := range testPlugin.GetAttachedVolumes() { attachedVolumesNum += len(volumeList) } for _, volumeList := range testPlugin.GetDetachedVolumes() { 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 11df3a4c8fa..4322f15a60d 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go @@ -96,21 +96,22 @@ type ActualStateOfWorld interface { // nodes, the volume is also deleted. DeleteVolumeNode(volumeName v1.UniqueVolumeName, nodeName types.NodeName) - // VolumeNodeExists returns true if the specified volume/node combo exists + // IsVolumeAttachedToNode returns true if the specified volume/node combo exists // in the underlying store indicating the specified volume is attached to // the specified node. IsVolumeAttachedToNode(volumeName v1.UniqueVolumeName, nodeName types.NodeName) bool // GetAttachedVolumes generates and returns a list of volumes/node pairs // reflecting which volumes might attached to which nodes based on the - // current actual state of the world. - GetAllVolumes() []AttachedVolume + // current actual state of the world. This list includes all the volumes which return successful + // attach and also the volumes which return errors during attach. + GetAttachedVolumes() []AttachedVolume - // GetAttachedVolumes generates and returns a list of volumes that added to + // GetAttachedVolumesForNode generates and returns a list of volumes that added to // the specified node reflecting which volumes are/or might be attached to that node // based on the current actual state of the world. This function is currently used by // attach_detach_controller to process VolumeInUse - GetAllVolumesForNode(nodeName types.NodeName) []AttachedVolume + GetAttachedVolumesForNode(nodeName types.NodeName) []AttachedVolume // GetAttachedVolumesPerNode generates and returns a map of nodes and volumes that added to // the specified node reflecting which volumes are attached to that node @@ -118,9 +119,9 @@ type ActualStateOfWorld interface { // reconciler to verify whether the volume is still attached to the node. GetAttachedVolumesPerNode() map[types.NodeName][]operationexecutor.AttachedVolume - // GetNodesForVolume returns the nodes on which the volume is attached. + // GetNodesForAttachedVolume returns the nodes on which the volume is attached. // This function is used by reconciler for mutli-attach check. - GetNodesForVolume(volumeName v1.UniqueVolumeName) []types.NodeName + GetNodesForAttachedVolume(volumeName v1.UniqueVolumeName) []types.NodeName // GetVolumesToReportAttached returns a map containing the set of nodes for // which the VolumesAttached Status field in the Node API object should be @@ -193,7 +194,7 @@ type attachedVolume struct { spec *volume.Spec // nodesAttachedTo is a map containing the set of nodes this volume has - // been trying to be attached to. The key in this map is the name of the + // been attached to. The key in this map is the name of the // node and the value is a node object containing more information about // the node. nodesAttachedTo map[types.NodeName]nodeAttachedTo @@ -212,8 +213,9 @@ type nodeAttachedTo struct { // node and is unsafe to detach mountedByNode bool - // attached indicates that the volume is confirmed to be attached to this node - attached bool + // attachConfirmed indicates that the storage system verified the volume has been attached to this node. + // This value is set to false when an attach operation fails and the volume may be attached or not. + attachedConfirmed bool // detachRequestedTime used to capture the desire to detach this volume detachRequestedTime time.Time @@ -244,7 +246,7 @@ type nodeToUpdateStatusFor struct { func (asw *actualStateOfWorld) MarkVolumeAsUncertain( uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName) error { - _, err := asw.AddVolumeNode(uniqueName, volumeSpec, nodeName, "", false) + _, err := asw.AddVolumeNode(uniqueName, volumeSpec, nodeName, "", false /* isAttached */) return err } @@ -280,6 +282,9 @@ func (asw *actualStateOfWorld) AddVolumeNode( volumeName := uniqueName if volumeName == "" { + if volumeSpec == nil { + return volumeName, fmt.Errorf("volumeSpec cannot be nil if volumeName is empty") + } attachableVolumePlugin, err := asw.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) if err != nil || attachableVolumePlugin == nil { return "", fmt.Errorf( @@ -316,24 +321,25 @@ func (asw *actualStateOfWorld) AddVolumeNode( nodeName, devicePath) } - asw.attachedVolumes[volumeName] = volumeObj - node, nodeExists := volumeObj.nodesAttachedTo[nodeName] if !nodeExists { // Create object if it doesn't exist. node = nodeAttachedTo{ nodeName: nodeName, mountedByNode: true, // Assume mounted, until proven otherwise - attached: isAttached, + attachedConfirmed: isAttached, detachRequestedTime: time.Time{}, } } else { - node.attached = isAttached - klog.V(5).Infof("Volume %q is already added to attachedVolume list to the node %q", + node.attachedConfirmed = isAttached + klog.V(5).Infof("Volume %q is already added to attachedVolume list to the node %q, the current attach state is %t", volumeName, - nodeName) + nodeName, + isAttached) } + volumeObj.nodesAttachedTo[nodeName] = node + asw.attachedVolumes[volumeName] = volumeObj if isAttached { asw.addVolumeToReportAsAttached(volumeName, nodeName) @@ -529,16 +535,14 @@ func (asw *actualStateOfWorld) IsVolumeAttachedToNode( volumeObj, volumeExists := asw.attachedVolumes[volumeName] if volumeExists { if node, nodeExists := volumeObj.nodesAttachedTo[nodeName]; nodeExists { - if node.attached == true { - return true - } + return node.attachedConfirmed } } return false } -func (asw *actualStateOfWorld) GetAllVolumes() []AttachedVolume { +func (asw *actualStateOfWorld) GetAttachedVolumes() []AttachedVolume { asw.RLock() defer asw.RUnlock() @@ -554,7 +558,7 @@ func (asw *actualStateOfWorld) GetAllVolumes() []AttachedVolume { return attachedVolumes } -func (asw *actualStateOfWorld) GetAllVolumesForNode( +func (asw *actualStateOfWorld) GetAttachedVolumesForNode( nodeName types.NodeName) []AttachedVolume { asw.RLock() defer asw.RUnlock() @@ -582,7 +586,7 @@ func (asw *actualStateOfWorld) GetAttachedVolumesPerNode() map[types.NodeName][] attachedVolumesPerNode := make(map[types.NodeName][]operationexecutor.AttachedVolume) for _, volumeObj := range asw.attachedVolumes { for nodeName, nodeObj := range volumeObj.nodesAttachedTo { - if nodeObj.attached { + if nodeObj.attachedConfirmed { volumes := attachedVolumesPerNode[nodeName] volumes = append(volumes, getAttachedVolume(&volumeObj, &nodeObj).AttachedVolume) attachedVolumesPerNode[nodeName] = volumes @@ -593,7 +597,7 @@ func (asw *actualStateOfWorld) GetAttachedVolumesPerNode() map[types.NodeName][] return attachedVolumesPerNode } -func (asw *actualStateOfWorld) GetNodesForVolume(volumeName v1.UniqueVolumeName) []types.NodeName { +func (asw *actualStateOfWorld) GetNodesForAttachedVolume(volumeName v1.UniqueVolumeName) []types.NodeName { asw.RLock() defer asw.RUnlock() @@ -604,7 +608,7 @@ func (asw *actualStateOfWorld) GetNodesForVolume(volumeName v1.UniqueVolumeName) nodes := []types.NodeName{} for k, nodesAttached := range volumeObj.nodesAttachedTo { - if nodesAttached.attached { + if nodesAttached.attachedConfirmed { nodes = append(nodes, k) } } 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 ab45693ec23..20882c61b60 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 @@ -52,7 +52,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNode(t *testing.T) { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -87,7 +87,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T) t.Fatalf("%q/%q volume/node combo does exist, it should not.", generatedVolumeName, nodeName) } - allVolumes := asw.GetAllVolumes() + allVolumes := asw.GetAttachedVolumes() if len(allVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(allVolumes)) } @@ -99,7 +99,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T) t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: Expect: Actual: <%v>", len(volumesForNode)) } @@ -111,7 +111,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T) t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: Expect: 0 { t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect no nodes returned.") } @@ -136,14 +136,14 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T) t.Fatalf("%q/%q combo does not exist, it should.", generatedVolumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) - nodes = asw.GetNodesForVolume(volumeName) + nodes = asw.GetNodesForAttachedVolume(volumeName) if len(nodes) != 1 { t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect one node returned.") } @@ -206,14 +206,14 @@ func Test_AddVolumeNode_Positive_NewVolumeTwoNodesWithFalseAttached(t *testing.T t.Fatalf("%q/%q combo does not exist, it should.", generatedVolumeName, node2Name) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 2 { t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(attachedVolumes)) } verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) - volumesForNode := asw.GetAllVolumesForNode(node2Name) + volumesForNode := asw.GetAttachedVolumesForNode(node2Name) if len(volumesForNode) != 1 { t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(volumesForNode)) } @@ -225,7 +225,7 @@ func Test_AddVolumeNode_Positive_NewVolumeTwoNodesWithFalseAttached(t *testing.T t.Fatalf("AddVolumeNode_Positive_NewVolumeTwoNodesWithFalseAttached failed. Actual: Expect: Actual: <%v>", len(attachedVolumes)) } @@ -322,7 +322,7 @@ func Test_AddVolumeNode_Positive_ExistingVolumeExistingNode(t *testing.T) { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName1, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -355,7 +355,7 @@ func Test_DeleteVolumeNode_Positive_VolumeExistsNodeExists(t *testing.T) { t.Fatalf("%q/%q volume/node combo exists, it should not.", generatedVolumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } @@ -379,7 +379,7 @@ func Test_DeleteVolumeNode_Positive_VolumeDoesntExistNodeDoesntExist(t *testing. t.Fatalf("%q/%q volume/node combo exists, it should not.", volumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } @@ -427,7 +427,7 @@ func Test_DeleteVolumeNode_Positive_TwoNodesOneDeleted(t *testing.T) { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName1, node2Name) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -459,7 +459,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeExists(t *testing.T) { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -492,7 +492,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeDoesntExist(t *testing.T) { t.Fatalf("%q/%q volume/node combo exists, it should not.", generatedVolumeName, node2Name) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -517,13 +517,13 @@ func Test_VolumeNodeExists_Positive_VolumeAndNodeDontExist(t *testing.T) { t.Fatalf("%q/%q volume/node combo exists, it should not.", volumeName, nodeName) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } } -// Calls GetAllVolumes() on empty data struct. +// Calls GetAttachedVolumes() on empty data struct. // Verifies no volume/node entries are returned. func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { // Arrange @@ -531,7 +531,7 @@ func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { asw := NewActualStateOfWorld(volumePluginMgr) // Act - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() // Assert if len(attachedVolumes) != 0 { @@ -540,7 +540,7 @@ func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { } // Populates data struct with one volume/node entry. -// Calls GetAllVolumes() to get list of entries. +// Calls GetAttachedVolumes() to get list of entries. // Verifies one volume/node entry is returned. func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { // Arrange @@ -556,7 +556,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() // Assert if len(attachedVolumes) != 1 { @@ -567,7 +567,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { } // Populates data struct with two volume/node entries (different node and volume). -// Calls GetAllVolumes() to get list of entries. +// Calls GetAttachedVolumes() to get list of entries. // Verifies both volume/node entries are returned. func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { // Arrange @@ -590,7 +590,7 @@ func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() // Assert if len(attachedVolumes) != 2 { @@ -602,7 +602,7 @@ func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { } // Populates data struct with two volume/node entries (same volume different node). -// Calls GetAllVolumes() to get list of entries. +// Calls GetAttachedVolumes() to get list of entries. // Verifies both volume/node entries are returned. func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { // Arrange @@ -638,7 +638,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() // Assert if len(attachedVolumes) != 2 { @@ -667,7 +667,7 @@ func Test_SetVolumeMountedByNode_Positive_Set(t *testing.T) { // Act: do not mark -- test default value // Assert - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -703,7 +703,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSet(t *testing.T) { t.Fatalf("SetVolumeMountedByNode2 failed. Expected Actual: <%v>", setVolumeMountedErr2) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -727,7 +727,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithoutInitialSet(t *testing.T) { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -742,7 +742,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithoutInitialSet(t *testing.T) { t.Fatalf("SetVolumeMountedByNode failed. Expected Actual: <%v>", setVolumeMountedErr) } - attachedVolumes = asw.GetAllVolumes() + attachedVolumes = asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -783,7 +783,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetAddVolumeNodeNotRes t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -815,7 +815,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest if err != nil { t.Fatalf("RemoveVolumeFromReportAsAttached failed. Expected: Actual: <%v>", err) } - expectedDetachRequestedTime := asw.GetAllVolumes()[0].DetachRequestedTime + expectedDetachRequestedTime := asw.GetAttachedVolumes()[0].DetachRequestedTime // Act setVolumeMountedErr1 := asw.SetVolumeMountedByNode(generatedVolumeName, nodeName, true /* mounted */) @@ -829,7 +829,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest t.Fatalf("SetVolumeMountedByNode2 failed. Expected Actual: <%v>", setVolumeMountedErr2) } - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -858,7 +858,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Set(t *testing.T) { // Act: do not mark -- test default value // Assert - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -893,7 +893,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Marked(t *testing.T) { } // Assert - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -936,7 +936,7 @@ func Test_MarkDesireToDetach_Positive_MarkedAddVolumeNodeReset(t *testing.T) { } // Assert - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -980,7 +980,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_UnsetWithInitialSetVolumeMou } // Assert - attachedVolumes := asw.GetAllVolumes() + attachedVolumes := asw.GetAttachedVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -1144,14 +1144,14 @@ func Test_SetDetachRequestTime_Positive(t *testing.T) { } } -func Test_GetAllVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { +func Test_GetAttachedVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) node := types.NodeName("random") // Act - attachedVolumes := asw.GetAllVolumesForNode(node) + attachedVolumes := asw.GetAttachedVolumesForNode(node) // Assert if len(attachedVolumes) != 0 { @@ -1159,7 +1159,7 @@ func Test_GetAllVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { } } -func Test_GetAllVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { +func Test_GetAttachedVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -1173,7 +1173,7 @@ func Test_GetAllVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumesForNode(nodeName) + attachedVolumes := asw.GetAttachedVolumesForNode(nodeName) // Assert if len(attachedVolumes) != 1 { @@ -1183,7 +1183,7 @@ func Test_GetAllVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } -func Test_GetAllVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { +func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -1204,7 +1204,7 @@ func Test_GetAllVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumesForNode(node2Name) + attachedVolumes := asw.GetAttachedVolumesForNode(node2Name) // Assert if len(attachedVolumes) != 1 { @@ -1214,7 +1214,7 @@ func Test_GetAllVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } -func Test_GetAllVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { +func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -1248,7 +1248,7 @@ func Test_GetAllVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumesForNode(node1Name) + attachedVolumes := asw.GetAttachedVolumesForNode(node1Name) // Assert if len(attachedVolumes) != 1 { @@ -1293,7 +1293,7 @@ func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) { } // Act - attachedVolumes := asw.GetAllVolumesForNode(node2Name) + attachedVolumes := asw.GetAttachedVolumesForNode(node2Name) // Assert if len(attachedVolumes) != 1 { diff --git a/pkg/controller/volume/attachdetach/metrics/metrics.go b/pkg/controller/volume/attachdetach/metrics/metrics.go index 1ff42733cf1..86f99c5e8b6 100644 --- a/pkg/controller/volume/attachdetach/metrics/metrics.go +++ b/pkg/controller/volume/attachdetach/metrics/metrics.go @@ -185,7 +185,7 @@ func (collector *attachDetachStateCollector) getTotalVolumesCount() volumeCount stateVolumeMap.add("desired_state_of_world", pluginName) } } - for _, v := range collector.asw.GetAllVolumes() { + for _, v := range collector.asw.GetAttachedVolumes() { if plugin, err := collector.volumePluginMgr.FindPluginBySpec(v.VolumeSpec); err == nil { pluginName := pluginNameNotAvailable if plugin != nil { diff --git a/pkg/controller/volume/attachdetach/reconciler/reconciler.go b/pkg/controller/volume/attachdetach/reconciler/reconciler.go index 7ccff1bb7fd..24c4e802a96 100644 --- a/pkg/controller/volume/attachdetach/reconciler/reconciler.go +++ b/pkg/controller/volume/attachdetach/reconciler/reconciler.go @@ -175,7 +175,7 @@ func (rc *reconciler) reconcile() { // pods that are rescheduled to a different node are detached first. // Ensure volumes that should be detached are detached. - for _, attachedVolume := range rc.actualStateOfWorld.GetAllVolumes() { + for _, attachedVolume := range rc.actualStateOfWorld.GetAttachedVolumes() { if !rc.desiredStateOfWorld.VolumeExists( attachedVolume.VolumeName, attachedVolume.NodeName) { // Don't even try to start an operation if there is already one running @@ -183,7 +183,7 @@ func (rc *reconciler) reconcile() { // may pass while at the same time the volume leaves the pending state, resulting in // double detach attempts if rc.attacherDetacher.IsOperationPending(attachedVolume.VolumeName, "") { - klog.V(5).Infof("Operation for volume %q is already running. Can't start detach for %q", attachedVolume.VolumeName, attachedVolume.NodeName) + klog.V(10).Infof("Operation for volume %q is already running. Can't start detach for %q", attachedVolume.VolumeName, attachedVolume.NodeName) continue } @@ -269,7 +269,7 @@ func (rc *reconciler) attachDesiredVolumes() { } if rc.isMultiAttachForbidden(volumeToAttach.VolumeSpec) { - nodes := rc.actualStateOfWorld.GetNodesForVolume(volumeToAttach.VolumeName) + nodes := rc.actualStateOfWorld.GetNodesForAttachedVolume(volumeToAttach.VolumeName) if len(nodes) > 0 { if !volumeToAttach.MultiAttachErrorReported { rc.reportMultiAttachError(volumeToAttach, nodes) diff --git a/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go b/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go index aa2d5ccd219..a16a7af8595 100644 --- a/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go +++ b/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go @@ -497,7 +497,7 @@ func Test_Run_OneVolumeAttachAndDetachMultipleNodesWithReadWriteOnce(t *testing. waitForDetachCallCount(t, 0 /* expectedDetachCallCount */, fakePlugin) waitForAttachedToNodesCount(t, 1 /* expectedNodeCount */, generatedVolumeName, asw) - nodesForVolume := asw.GetNodesForVolume(generatedVolumeName) + nodesForVolume := asw.GetNodesForAttachedVolume(generatedVolumeName) // check if multiattach is marked // at least one volume+node should be marked with multiattach error @@ -576,10 +576,11 @@ func Test_Run_OneVolumeAttachAndDetachUncertainNodesWithReadWriteOnce(t *testing t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) } + time.Sleep(1 * time.Second) // Volume is added to asw. Because attach operation fails, volume should not reported as attached to the node. waitForVolumeAddedToNode(t, generatedVolumeName, nodeName1, asw) - veriryVolumeAttachedToNode(t, generatedVolumeName, nodeName1, false, asw) - veriryVolumeReportedAsAttachedToNode(t, generatedVolumeName, nodeName1, false, asw) + verifyVolumeAttachedToNode(t, generatedVolumeName, nodeName1, true, asw) + verifyVolumeReportedAsAttachedToNode(t, generatedVolumeName, nodeName1, true, asw) // When volume is added to the node, it is set to mounted by default. Then the status will be updated by checking node status VolumeInUse. // Without this, the delete operation will be delayed due to mounted status @@ -595,7 +596,73 @@ func Test_Run_OneVolumeAttachAndDetachUncertainNodesWithReadWriteOnce(t *testing t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) } waitForVolumeAttachedToNode(t, generatedVolumeName, nodeName2, asw) - veriryVolumeAttachedToNode(t, generatedVolumeName, nodeName2, true, asw) + verifyVolumeAttachedToNode(t, generatedVolumeName, nodeName2, true, asw) + +} + +// Creates a volume with accessMode ReadWriteOnce +// First create a pod which will try to attach the volume to the a node named "timeout-node". The attach call for this node will +// fail for timeout, but the volume will be actually attached to the node after the call. +// Secondly, delete the this pod. +// Lastly, create a pod scheduled to a normal node which will trigger attach volume to the node. The attach should return successfully. +func Test_Run_OneVolumeAttachAndDetachTimeoutNodesWithReadWriteOnce(t *testing.T) { + // Arrange + volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) + dsw := cache.NewDesiredStateOfWorld(volumePluginMgr) + asw := cache.NewActualStateOfWorld(volumePluginMgr) + fakeKubeClient := controllervolumetesting.CreateTestClient() + fakeRecorder := &record.FakeRecorder{} + fakeHandler := volumetesting.NewBlockVolumePathHandler() + ad := operationexecutor.NewOperationExecutor(operationexecutor.NewOperationGenerator( + fakeKubeClient, + volumePluginMgr, + fakeRecorder, + false, /* checkNodeCapabilitiesBeforeMount */ + fakeHandler)) + nsu := statusupdater.NewFakeNodeStatusUpdater(false /* returnError */) + reconciler := NewReconciler( + reconcilerLoopPeriod, maxWaitForUnmountDuration, syncLoopPeriod, false, dsw, asw, ad, nsu, fakeRecorder) + podName1 := "pod-uid1" + podName2 := "pod-uid2" + volumeName := v1.UniqueVolumeName("volume-name") + volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) + volumeSpec.PersistentVolume.Spec.AccessModes = []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce} + nodeName1 := k8stypes.NodeName(volumetesting.TimeoutAttachNode) + nodeName2 := k8stypes.NodeName("node-name2") + dsw.AddNode(nodeName1, false /*keepTerminatedPodVolumes*/) + dsw.AddNode(nodeName2, false /*keepTerminatedPodVolumes*/) + + // Act + ch := make(chan struct{}) + go reconciler.Run(ch) + defer close(ch) + + // Add the pod in which the volume is attached to the timeout node + generatedVolumeName, podAddErr := dsw.AddPod(types.UniquePodName(podName1), controllervolumetesting.NewPod(podName1, podName1), volumeSpec, nodeName1) + if podAddErr != nil { + t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) + } + + // Volume is added to asw. Because attach operation fails, volume should not reported as attached to the node. + waitForVolumeAddedToNode(t, generatedVolumeName, nodeName1, asw) + verifyVolumeAttachedToNode(t, generatedVolumeName, nodeName1, false, asw) + verifyVolumeReportedAsAttachedToNode(t, generatedVolumeName, nodeName1, false, asw) + + // When volume is added to the node, it is set to mounted by default. Then the status will be updated by checking node status VolumeInUse. + // Without this, the delete operation will be delayed due to mounted status + asw.SetVolumeMountedByNode(generatedVolumeName, nodeName1, false /* mounted */) + + dsw.DeletePod(types.UniquePodName(podName1), generatedVolumeName, nodeName1) + + waitForVolumeRemovedFromNode(t, generatedVolumeName, nodeName1, asw) + + // Add a second pod which tries to attach the volume to a different node. + generatedVolumeName, podAddErr = dsw.AddPod(types.UniquePodName(podName2), controllervolumetesting.NewPod(podName2, podName2), volumeSpec, nodeName2) + if podAddErr != nil { + t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) + } + waitForVolumeAttachedToNode(t, generatedVolumeName, nodeName2, asw) + verifyVolumeAttachedToNode(t, generatedVolumeName, nodeName2, true, asw) } @@ -935,7 +1002,7 @@ func waitForAttachedToNodesCount( err := retryWithExponentialBackOff( time.Duration(5*time.Millisecond), func() (bool, error) { - count := len(asw.GetNodesForVolume(volumeName)) + count := len(asw.GetNodesForAttachedVolume(volumeName)) if count == expectedNodeCount { return true, nil } @@ -950,7 +1017,7 @@ func waitForAttachedToNodesCount( ) if err != nil { - count := len(asw.GetNodesForVolume(volumeName)) + count := len(asw.GetNodesForAttachedVolume(volumeName)) t.Fatalf( "Wrong number of nodes having <%v> attached. Expected: <%v> Actual: <%v>", volumeName, @@ -1010,7 +1077,7 @@ func waitForVolumeAddedToNode( err := retryWithExponentialBackOff( time.Duration(500*time.Millisecond), func() (bool, error) { - volumes := asw.GetAllVolumes() + volumes := asw.GetAttachedVolumes() for _, volume := range volumes { if volume.VolumeName == volumeName && volume.NodeName == nodeName { return true, nil @@ -1042,7 +1109,7 @@ func waitForVolumeRemovedFromNode( err := retryWithExponentialBackOff( time.Duration(500*time.Millisecond), func() (bool, error) { - volumes := asw.GetAllVolumes() + volumes := asw.GetAttachedVolumes() exist := false for _, volume := range volumes { if volume.VolumeName == volumeName && volume.NodeName == nodeName { @@ -1070,7 +1137,7 @@ func waitForVolumeRemovedFromNode( } } -func veriryVolumeAttachedToNode( +func verifyVolumeAttachedToNode( t *testing.T, volumeName v1.UniqueVolumeName, nodeName k8stypes.NodeName, @@ -1089,7 +1156,7 @@ func veriryVolumeAttachedToNode( } -func veriryVolumeReportedAsAttachedToNode( +func verifyVolumeReportedAsAttachedToNode( t *testing.T, volumeName v1.UniqueVolumeName, nodeName k8stypes.NodeName, diff --git a/pkg/controller/volume/attachdetach/testing/testvolumespec.go b/pkg/controller/volume/attachdetach/testing/testvolumespec.go index 15af6eb1827..a243e724b25 100644 --- a/pkg/controller/volume/attachdetach/testing/testvolumespec.go +++ b/pkg/controller/volume/attachdetach/testing/testvolumespec.go @@ -328,7 +328,7 @@ func (plugin *TestPlugin) GetErrorEncountered() bool { return plugin.ErrorEncountered } -func (plugin *TestPlugin) GetAllVolumes() map[string][]string { +func (plugin *TestPlugin) GetAttachedVolumes() map[string][]string { plugin.pluginLock.RLock() defer plugin.pluginLock.RUnlock() ret := make(map[string][]string) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 852373e1bd4..81c46ec0c22 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -51,8 +51,11 @@ const ( // is expected to fail. ExpectProvisionFailureKey = "expect-provision-failure" // The node is marked as uncertain. The attach operation will fail and return timeout error - // but the operation is actually succeeded. + // for the first attach call. The following call will return sucesssfully. UncertainAttachNode = "uncertain-attach-node" + // The node is marked as timeout. The attach operation will always fail and return timeout error + // but the operation is actually succeeded. + TimeoutAttachNode = "timeout-attach-node" // The node is marked as multi-attach which means it is allowed to attach the volume to multiple nodes. MultiAttachNode = "multi-attach-node" ) @@ -282,7 +285,12 @@ var _ FSResizableVolumePlugin = &FakeVolumePlugin{} func (plugin *FakeVolumePlugin) getFakeVolume(list *[]*FakeVolume) *FakeVolume { volumeList := *list if list != nil && len(volumeList) > 0 { - return volumeList[0] + volume := volumeList[0] + volume.Lock() + defer volume.Unlock() + volume.WaitForAttachHook = plugin.WaitForAttachHook + volume.UnmountDeviceHook = plugin.UnmountDeviceHook + return volume } volume := &FakeVolume{ WaitForAttachHook: plugin.WaitForAttachHook, @@ -773,7 +781,7 @@ func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error volumeNode, exist := fv.VolumesAttached[volumeName] if exist { if nodeName == UncertainAttachNode { - return "", fmt.Errorf("Timed out to attach volume %q to node %q", volumeName, nodeName) + return "/dev/vdb-test", nil } if volumeNode == nodeName || volumeNode == MultiAttachNode || nodeName == MultiAttachNode { return "/dev/vdb-test", nil @@ -782,7 +790,7 @@ func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error } fv.VolumesAttached[volumeName] = nodeName - if nodeName == UncertainAttachNode { + if nodeName == UncertainAttachNode || nodeName == TimeoutAttachNode { return "", fmt.Errorf("Timed out to attach volume %q to node %q", volumeName, nodeName) } return "/dev/vdb-test", nil diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index baf2b83f3d0..0096e639457 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -192,6 +192,10 @@ type ActualStateOfWorldAttacherUpdater interface { // volumes. See issue 29695. MarkVolumeAsAttached(volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) error + // Marks the specified volume as *possibly* attached to the specified node. + // If an attach operation fails, the attach/detach controller does not know for certain if the volume is attached or not. + // If the volume name is supplied, that volume name will be used. If not, the + // volume name is computed using the result from querying the plugin. MarkVolumeAsUncertain(volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName) error // Marks the specified volume as detached from the specified node diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index ea4e16d55ac..c55ab64e68a 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -322,12 +322,12 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( klog.Errorf("AttachVolume.MarkVolumeAsAttached failed to fix dangling volume error for volume %q with %s", volumeToAttach.VolumeName, addErr) } - } - addVolumeNodeErr := actualStateOfWorld.MarkVolumeAsUncertain( - v1.UniqueVolumeName(""), volumeToAttach.VolumeSpec, volumeToAttach.NodeName) - if addVolumeNodeErr != nil { - // On failure, return error. Caller will log and retry. - return volumeToAttach.GenerateError("AttachVolume.MarkVolumeAsUncertain failed", addVolumeNodeErr) + } else { + addErr := actualStateOfWorld.MarkVolumeAsUncertain( + v1.UniqueVolumeName(""), volumeToAttach.VolumeSpec, volumeToAttach.NodeName) + if addErr != nil { + klog.Errorf("AttachVolume.MarkVolumeAsUncertain fail to add the volume %q to actual state with %s", volumeToAttach.VolumeName, addErr) + } } // On failure, return error. Caller will log and retry. return volumeToAttach.GenerateError("AttachVolume.Attach failed", attachErr)