diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index f76e3224a6..d2dd7b3de6 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller.go @@ -435,7 +435,7 @@ func (adc *attachDetachController) populateDesiredStateOfWorld() error { err) continue } - if adc.actualStateOfWorld.VolumeNodeExists(volumeName, nodeName) { + if adc.actualStateOfWorld.IsVolumeAttachedToNode(volumeName, nodeName) { devicePath, err := adc.getNodeVolumeDevicePath(volumeName, nodeName) if err != nil { klog.Errorf("Failed to find device path: %v", err) diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go index f850302521..ec66fab1f8 100644 --- a/pkg/controller/volume/attachdetach/attach_detach_controller_test.go +++ b/pkg/controller/volume/attachdetach/attach_detach_controller_test.go @@ -110,7 +110,7 @@ func Test_AttachDetachControllerStateOfWolrdPopulators_Positive(t *testing.T) { for _, node := range nodes { nodeName := types.NodeName(node.Name) for _, attachedVolume := range node.Status.VolumesAttached { - found := adc.actualStateOfWorld.VolumeNodeExists(attachedVolume.Name, nodeName) + found := adc.actualStateOfWorld.IsVolumeAttachedToNode(attachedVolume.Name, nodeName) if !found { t.Fatalf("Run failed with error. Node %s, volume %s not found", nodeName, attachedVolume.Name) } diff --git a/pkg/controller/volume/attachdetach/cache/BUILD b/pkg/controller/volume/attachdetach/cache/BUILD index a2890223e0..9b86e8711f 100644 --- a/pkg/controller/volume/attachdetach/cache/BUILD +++ b/pkg/controller/volume/attachdetach/cache/BUILD @@ -34,6 +34,7 @@ go_test( deps = [ "//pkg/controller/volume/attachdetach/testing:go_default_library", "//pkg/volume/testing:go_default_library", + "//pkg/volume/util:go_default_library", "//pkg/volume/util/types:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", 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 f25fb00881..4322f15a60 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go @@ -47,8 +47,10 @@ type ActualStateOfWorld interface { // operationexecutor to interact with it. operationexecutor.ActualStateOfWorldAttacherUpdater - // AddVolumeNode adds the given volume and node to the underlying store - // indicating the specified volume is attached to the specified node. + // AddVolumeNode adds the given volume and node to the underlying store. + // If attached is set to true, it indicates the specified volume is already + // attached to the specified node. If attached set to false, it means that + // the volume is not confirmed to be attached to the node yet. // A unique volume name is generated from the volumeSpec and returned on // success. // If volumeSpec is not an attachable volume plugin, an error is returned. @@ -56,7 +58,7 @@ type ActualStateOfWorld interface { // added. // If no node with the name nodeName exists in list of attached nodes for // the specified volume, the node is added. - AddVolumeNode(uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) (v1.UniqueVolumeName, error) + AddVolumeNode(uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string, attached bool) (v1.UniqueVolumeName, error) // SetVolumeMountedByNode sets the MountedByNode value for the given volume // and node. When set to true the mounted parameter indicates the volume @@ -94,25 +96,32 @@ 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. - VolumeNodeExists(volumeName v1.UniqueVolumeName, nodeName types.NodeName) bool + IsVolumeAttachedToNode(volumeName v1.UniqueVolumeName, nodeName types.NodeName) bool // GetAttachedVolumes generates and returns a list of volumes/node pairs - // reflecting which volumes are attached to which nodes based on the - // current actual state of the world. + // reflecting which volumes might attached to which nodes based on the + // 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 attached to - // the specified node reflecting which volumes are attached to that node - // based on the current actual state of the world. + // 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 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 + // based on the current actual state of the world. This function is currently used by + // 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 - GetNodesForVolume(volumeName v1.UniqueVolumeName) []types.NodeName + // GetNodesForAttachedVolume returns the nodes on which the volume is attached. + // This function is used by reconciler for mutli-attach check. + 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 @@ -185,7 +194,7 @@ type attachedVolume struct { spec *volume.Spec // nodesAttachedTo is a map containing the set of nodes this volume has - // successfully been 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 @@ -194,7 +203,8 @@ type attachedVolume struct { devicePath string } -// The nodeAttachedTo object represents a node that has volumes attached to it. +// The nodeAttachedTo object represents a node that has volumes attached to it +// or trying to attach to it. type nodeAttachedTo struct { // nodeName contains the name of this node. nodeName types.NodeName @@ -203,11 +213,9 @@ type nodeAttachedTo struct { // node and is unsafe to detach mountedByNode bool - // number of times SetVolumeMountedByNode has been called to set the value - // of mountedByNode to true. This is used to prevent mountedByNode from - // being reset during the period between attach and mount when volumesInUse - // status for the node may not be set. - mountedByNodeSetCount uint + // 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 @@ -235,9 +243,16 @@ type nodeToUpdateStatusFor struct { volumesToReportAsAttached map[v1.UniqueVolumeName]v1.UniqueVolumeName } +func (asw *actualStateOfWorld) MarkVolumeAsUncertain( + uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName) error { + + _, err := asw.AddVolumeNode(uniqueName, volumeSpec, nodeName, "", false /* isAttached */) + return err +} + func (asw *actualStateOfWorld) MarkVolumeAsAttached( uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) error { - _, err := asw.AddVolumeNode(uniqueName, volumeSpec, nodeName, devicePath) + _, err := asw.AddVolumeNode(uniqueName, volumeSpec, nodeName, devicePath, true) return err } @@ -261,12 +276,15 @@ func (asw *actualStateOfWorld) AddVolumeToReportAsAttached( } func (asw *actualStateOfWorld) AddVolumeNode( - uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) (v1.UniqueVolumeName, error) { + uniqueName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string, isAttached bool) (v1.UniqueVolumeName, error) { asw.Lock() defer asw.Unlock() - var volumeName v1.UniqueVolumeName - if volumeSpec != nil { + 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( @@ -283,12 +301,6 @@ func (asw *actualStateOfWorld) AddVolumeNode( volumeSpec.Name(), err) } - } else { - // volumeSpec is nil - // This happens only on controller startup when reading the volumes from node - // status; if the pods using the volume have been removed and are unreachable - // the volumes should be detached immediately and the spec is not needed - volumeName = uniqueName } volumeObj, volumeExists := asw.attachedVolumes[volumeName] @@ -309,24 +321,29 @@ func (asw *actualStateOfWorld) AddVolumeNode( nodeName, devicePath) } - asw.attachedVolumes[volumeName] = volumeObj - - _, nodeExists := volumeObj.nodesAttachedTo[nodeName] + node, nodeExists := volumeObj.nodesAttachedTo[nodeName] if !nodeExists { // Create object if it doesn't exist. - volumeObj.nodesAttachedTo[nodeName] = nodeAttachedTo{ - nodeName: nodeName, - mountedByNode: true, // Assume mounted, until proven otherwise - mountedByNodeSetCount: 0, - detachRequestedTime: time.Time{}, + node = nodeAttachedTo{ + nodeName: nodeName, + mountedByNode: true, // Assume mounted, until proven otherwise + attachedConfirmed: isAttached, + detachRequestedTime: time.Time{}, } } else { - 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) } - asw.addVolumeToReportAsAttached(volumeName, nodeName) + volumeObj.nodesAttachedTo[nodeName] = node + asw.attachedVolumes[volumeName] = volumeObj + + if isAttached { + asw.addVolumeToReportAsAttached(volumeName, nodeName) + } return volumeName, nil } @@ -340,11 +357,6 @@ func (asw *actualStateOfWorld) SetVolumeMountedByNode( return fmt.Errorf("Failed to SetVolumeMountedByNode with error: %v", err) } - if mounted { - // Increment set count - nodeObj.mountedByNodeSetCount = nodeObj.mountedByNodeSetCount + 1 - } - nodeObj.mountedByNode = mounted volumeObj.nodesAttachedTo[nodeName] = nodeObj klog.V(4).Infof("SetVolumeMountedByNode volume %v to the node %q mounted %t", @@ -515,15 +527,15 @@ func (asw *actualStateOfWorld) DeleteVolumeNode( asw.removeVolumeFromReportAsAttached(volumeName, nodeName) } -func (asw *actualStateOfWorld) VolumeNodeExists( +func (asw *actualStateOfWorld) IsVolumeAttachedToNode( volumeName v1.UniqueVolumeName, nodeName types.NodeName) bool { asw.RLock() defer asw.RUnlock() volumeObj, volumeExists := asw.attachedVolumes[volumeName] if volumeExists { - if _, nodeExists := volumeObj.nodesAttachedTo[nodeName]; nodeExists { - return true + if node, nodeExists := volumeObj.nodesAttachedTo[nodeName]; nodeExists { + return node.attachedConfirmed } } @@ -574,16 +586,18 @@ 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 { - volumes := attachedVolumesPerNode[nodeName] - volumes = append(volumes, getAttachedVolume(&volumeObj, &nodeObj).AttachedVolume) - attachedVolumesPerNode[nodeName] = volumes + if nodeObj.attachedConfirmed { + volumes := attachedVolumesPerNode[nodeName] + volumes = append(volumes, getAttachedVolume(&volumeObj, &nodeObj).AttachedVolume) + attachedVolumesPerNode[nodeName] = volumes + } } } 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() @@ -593,8 +607,10 @@ func (asw *actualStateOfWorld) GetNodesForVolume(volumeName v1.UniqueVolumeName) } nodes := []types.NodeName{} - for k := range volumeObj.nodesAttachedTo { - nodes = append(nodes, k) + for k, nodesAttached := range volumeObj.nodesAttachedTo { + if nodesAttached.attachedConfirmed { + nodes = append(nodes, k) + } } return nodes } 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 14a531ba10..20882c61b6 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 @@ -24,9 +24,10 @@ import ( "k8s.io/apimachinery/pkg/types" controllervolumetesting "k8s.io/kubernetes/pkg/controller/volume/attachdetach/testing" volumetesting "k8s.io/kubernetes/pkg/volume/testing" + volumeutil "k8s.io/kubernetes/pkg/volume/util" ) -// Calls AddVolumeNode() once. +// Calls AddVolumeNode() once with attached set to true. // Verifies a single volume/node entry exists. func Test_AddVolumeNode_Positive_NewVolumeNewNode(t *testing.T) { // Arrange @@ -39,14 +40,14 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNode(t *testing.T) { devicePath := "fake/device/path" // Act - generatedVolumeName, err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) // Assert if err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", err) } - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, nodeName) if !volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName, nodeName) } @@ -59,6 +60,183 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNode(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } +// Calls AddVolumeNode() once with attached set to false. +// Verifies a single volume/node entry exists. +// Then calls AddVolumeNode() with attached set to true +// Verifies volume is attached to the node according to asw. +func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T) { + // Arrange + volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) + asw := NewActualStateOfWorld(volumePluginMgr) + volumeName := v1.UniqueVolumeName("volume-name") + volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) + + nodeName := types.NodeName("node-name") + devicePath := "fake/device/path" + + // Act + generatedVolumeName, err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, false) + + // Assert + if err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", err) + } + + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, nodeName) + if volumeNodeComboExists { + t.Fatalf("%q/%q volume/node combo does exist, it should not.", generatedVolumeName, nodeName) + } + + allVolumes := asw.GetAttachedVolumes() + if len(allVolumes) != 1 { + t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(allVolumes)) + } + verifyAttachedVolume(t, allVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + + reportAsAttachedVolumesMap := asw.GetVolumesToReportAttached() + _, exists := reportAsAttachedVolumesMap[nodeName] + if exists { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: Expect: Actual: <%v>", len(volumesForNode)) + } + verifyAttachedVolume(t, volumesForNode, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + + attachedVolumesMap := asw.GetAttachedVolumesPerNode() + _, exists = attachedVolumesMap[nodeName] + if exists { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: Expect: 0 { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect no nodes returned.") + } + + // Add the volume to the node second time with attached set to true + generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) + + // Assert + if add2Err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) + } + + if generatedVolumeName != generatedVolumeName2 { + t.Fatalf( + "Generated volume names for the same volume should be the same but they are not: %q and %q", + generatedVolumeName, + generatedVolumeName2) + } + + volumeNodeComboExists = asw.IsVolumeAttachedToNode(generatedVolumeName, nodeName) + if !volumeNodeComboExists { + t.Fatalf("%q/%q combo does not exist, it should.", generatedVolumeName, nodeName) + } + + 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.GetNodesForAttachedVolume(volumeName) + if len(nodes) != 1 { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect one node returned.") + } + if nodes[0] != nodeName { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect node %v, Actual node %v", nodeName, nodes[0]) + } + + attachedVolumesMap = asw.GetAttachedVolumesPerNode() + _, exists = attachedVolumesMap[nodeName] + if !exists { + t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: Expect: Actual: <%v>", err) + } + + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, node1Name) + if volumeNodeComboExists { + t.Fatalf("%q/%q volume/node combo does exist, it should not.", generatedVolumeName, node1Name) + } + + generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, node2Name, devicePath, true) + + // Assert + if add2Err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) + } + + if generatedVolumeName != generatedVolumeName2 { + t.Fatalf( + "Generated volume names for the same volume should be the same but they are not: %q and %q", + generatedVolumeName, + generatedVolumeName2) + } + + volumeNodeComboExists = asw.IsVolumeAttachedToNode(generatedVolumeName, node2Name) + if !volumeNodeComboExists { + t.Fatalf("%q/%q combo does not exist, it should.", generatedVolumeName, node2Name) + } + + 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.GetAttachedVolumesForNode(node2Name) + if len(volumesForNode) != 1 { + t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(volumesForNode)) + } + verifyAttachedVolume(t, volumesForNode, generatedVolumeName, string(volumeName), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + + attachedVolumesMap := asw.GetAttachedVolumesPerNode() + attachedVolumesPerNode, exists := attachedVolumesMap[node2Name] + if !exists || len(attachedVolumesPerNode) != 1 { + t.Fatalf("AddVolumeNode_Positive_NewVolumeTwoNodesWithFalseAttached failed. Actual: Expect: Expect: Actual: <%v>", addErr) } @@ -172,7 +350,7 @@ func Test_DeleteVolumeNode_Positive_VolumeExistsNodeExists(t *testing.T) { asw.DeleteVolumeNode(generatedVolumeName, nodeName) // Assert - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, nodeName) if volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo exists, it should not.", generatedVolumeName, nodeName) } @@ -196,7 +374,7 @@ func Test_DeleteVolumeNode_Positive_VolumeDoesntExistNodeDoesntExist(t *testing. asw.DeleteVolumeNode(volumeName, nodeName) // Assert - volumeNodeComboExists := asw.VolumeNodeExists(volumeName, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(volumeName, nodeName) if volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo exists, it should not.", volumeName, nodeName) } @@ -220,11 +398,11 @@ func Test_DeleteVolumeNode_Positive_TwoNodesOneDeleted(t *testing.T) { node1Name := types.NodeName("node1-name") node2Name := types.NodeName("node2-name") devicePath := "fake/device/path" - generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath) + generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } - generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, node2Name, devicePath) + generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, node2Name, devicePath, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } @@ -239,12 +417,12 @@ func Test_DeleteVolumeNode_Positive_TwoNodesOneDeleted(t *testing.T) { asw.DeleteVolumeNode(generatedVolumeName1, node1Name) // Assert - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName1, node1Name) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName1, node1Name) if volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo exists, it should not.", generatedVolumeName1, node1Name) } - volumeNodeComboExists = asw.VolumeNodeExists(generatedVolumeName1, node2Name) + volumeNodeComboExists = asw.IsVolumeAttachedToNode(generatedVolumeName1, node2Name) if !volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName1, node2Name) } @@ -258,7 +436,7 @@ func Test_DeleteVolumeNode_Positive_TwoNodesOneDeleted(t *testing.T) { } // Populates data struct with one volume/node entry. -// Calls VolumeNodeExists() to verify entry. +// Calls IsVolumeAttachedToNode() to verify entry. // Verifies the populated volume/node entry exists. func Test_VolumeNodeExists_Positive_VolumeExistsNodeExists(t *testing.T) { // Arrange @@ -268,13 +446,13 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeExists(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } // Act - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, nodeName) // Assert if !volumeNodeComboExists { @@ -290,7 +468,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeExists(t *testing.T) { } // Populates data struct with one volume1/node1 entry. -// Calls VolumeNodeExists() with volume1/node2. +// Calls IsVolumeAttachedToNode() with volume1/node2. // Verifies requested entry does not exist, but populated entry does. func Test_VolumeNodeExists_Positive_VolumeExistsNodeDoesntExist(t *testing.T) { // Arrange @@ -301,13 +479,13 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeDoesntExist(t *testing.T) { node1Name := types.NodeName("node1-name") node2Name := types.NodeName("node2-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } // Act - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName, node2Name) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName, node2Name) // Assert if volumeNodeComboExists { @@ -322,7 +500,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeDoesntExist(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } -// Calls VolumeNodeExists() on empty data struct. +// Calls IsVolumeAttachedToNode() on empty data struct. // Verifies requested entry does not exist. func Test_VolumeNodeExists_Positive_VolumeAndNodeDontExist(t *testing.T) { // Arrange @@ -332,7 +510,7 @@ func Test_VolumeNodeExists_Positive_VolumeAndNodeDontExist(t *testing.T) { nodeName := types.NodeName("node-name") // Act - volumeNodeComboExists := asw.VolumeNodeExists(volumeName, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(volumeName, nodeName) // Assert if volumeNodeComboExists { @@ -372,7 +550,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -399,14 +577,14 @@ func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { volume1Spec := controllervolumetesting.GetTestVolumeSpec(string(volume1Name), volume1Name) node1Name := types.NodeName("node1-name") devicePath := "fake/device/path" - generatedVolumeName1, add1Err := asw.AddVolumeNode(volume1Name, volume1Spec, node1Name, devicePath) + generatedVolumeName1, add1Err := asw.AddVolumeNode(volume1Name, volume1Spec, node1Name, devicePath, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } volume2Name := v1.UniqueVolumeName("volume2-name") volume2Spec := controllervolumetesting.GetTestVolumeSpec(string(volume2Name), volume2Name) node2Name := types.NodeName("node2-name") - generatedVolumeName2, add2Err := asw.AddVolumeNode(volume2Name, volume2Spec, node2Name, devicePath) + generatedVolumeName2, add2Err := asw.AddVolumeNode(volume2Name, volume2Spec, node2Name, devicePath, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } @@ -434,12 +612,20 @@ func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) node1Name := types.NodeName("node1-name") devicePath := "fake/device/path" - generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath) + plugin, err := volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get volume plugin from spec %v, %v", volumeSpec, err) + } + uniqueVolumeName, err := volumeutil.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get uniqueVolumeName from spec %v, %v", volumeSpec, err) + } + generatedVolumeName1, add1Err := asw.AddVolumeNode(uniqueVolumeName, volumeSpec, node1Name, devicePath, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } node2Name := types.NodeName("node2-name") - generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath) + generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } @@ -473,7 +659,7 @@ func Test_SetVolumeMountedByNode_Positive_Set(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -500,7 +686,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSet(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -536,7 +722,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithoutInitialSet(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -576,7 +762,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetAddVolumeNodeNotRes volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -584,7 +770,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetAddVolumeNodeNotRes // Act setVolumeMountedErr1 := asw.SetVolumeMountedByNode(generatedVolumeName, nodeName, true /* mounted */) setVolumeMountedErr2 := asw.SetVolumeMountedByNode(generatedVolumeName, nodeName, false /* mounted */) - generatedVolumeName, addErr = asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr = asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) // Assert if setVolumeMountedErr1 != nil { @@ -617,7 +803,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -664,7 +850,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Set(t *testing.T) { devicePath := "fake/device/path" volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -691,7 +877,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Marked(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -727,7 +913,7 @@ func Test_MarkDesireToDetach_Positive_MarkedAddVolumeNodeReset(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -770,7 +956,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_UnsetWithInitialSetVolumeMou volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -813,7 +999,7 @@ func Test_RemoveVolumeFromReportAsAttached(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -846,7 +1032,7 @@ func Test_RemoveVolumeFromReportAsAttached_AddVolumeToReportAsAttached_Positive( volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -889,7 +1075,7 @@ func Test_RemoveVolumeFromReportAsAttached_Delete_AddVolumeNode(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -910,7 +1096,7 @@ func Test_RemoveVolumeFromReportAsAttached_Delete_AddVolumeNode(t *testing.T) { asw.DeleteVolumeNode(generatedVolumeName, nodeName) - asw.AddVolumeNode(volumeName, volumeSpec, nodeName, "" /*device path*/) + asw.AddVolumeNode(volumeName, volumeSpec, nodeName, "" /*device path*/, true) reportAsAttachedVolumesMap = asw.GetVolumesToReportAttached() volumes, exists = reportAsAttachedVolumesMap[nodeName] @@ -934,7 +1120,7 @@ func Test_SetDetachRequestTime_Positive(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -981,7 +1167,7 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) nodeName := types.NodeName("node-name") devicePath := "fake/device/path" - generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName, addErr := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) if addErr != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } @@ -1005,14 +1191,14 @@ func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { volume1Spec := controllervolumetesting.GetTestVolumeSpec(string(volume1Name), volume1Name) node1Name := types.NodeName("node1-name") devicePath := "fake/device/path" - _, add1Err := asw.AddVolumeNode(volume1Name, volume1Spec, node1Name, devicePath) + _, add1Err := asw.AddVolumeNode(volume1Name, volume1Spec, node1Name, devicePath, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } volume2Name := v1.UniqueVolumeName("volume2-name") volume2Spec := controllervolumetesting.GetTestVolumeSpec(string(volume2Name), volume2Name) node2Name := types.NodeName("node2-name") - generatedVolumeName2, add2Err := asw.AddVolumeNode(volume2Name, volume2Spec, node2Name, devicePath) + generatedVolumeName2, add2Err := asw.AddVolumeNode(volume2Name, volume2Spec, node2Name, devicePath, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } @@ -1036,12 +1222,20 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) node1Name := types.NodeName("node1-name") devicePath := "fake/device/path" - generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath) + plugin, err := volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get volume plugin from spec %v, %v", volumeSpec, err) + } + uniqueVolumeName, err := volumeutil.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get uniqueVolumeName from spec %v, %v", volumeSpec, err) + } + generatedVolumeName1, add1Err := asw.AddVolumeNode(uniqueVolumeName, volumeSpec, node1Name, devicePath, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } node2Name := types.NodeName("node2-name") - generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath) + generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } @@ -1072,13 +1266,21 @@ func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) { volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) node1Name := types.NodeName("node1-name") devicePath1 := "fake/device/path1" - generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, node1Name, devicePath1) + plugin, err := volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get volume plugin from spec %v, %v", volumeSpec, err) + } + uniqueVolumeName, err := volumeutil.GetUniqueVolumeNameFromSpec(plugin, volumeSpec) + if err != nil || plugin == nil { + t.Fatalf("Failed to get uniqueVolumeName from spec %v, %v", volumeSpec, err) + } + generatedVolumeName1, add1Err := asw.AddVolumeNode(uniqueVolumeName, volumeSpec, node1Name, devicePath1, true) if add1Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) } node2Name := types.NodeName("node2-name") devicePath2 := "fake/device/path2" - generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath2) + generatedVolumeName2, add2Err := asw.AddVolumeNode(v1.UniqueVolumeName(""), volumeSpec, node2Name, devicePath2, true) if add2Err != nil { t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) } diff --git a/pkg/controller/volume/attachdetach/metrics/metrics_test.go b/pkg/controller/volume/attachdetach/metrics/metrics_test.go index 00efe32c1a..239e5cfc5e 100644 --- a/pkg/controller/volume/attachdetach/metrics/metrics_test.go +++ b/pkg/controller/volume/attachdetach/metrics/metrics_test.go @@ -143,7 +143,7 @@ func TestTotalVolumesMetricCollection(t *testing.T) { if err != nil { t.Fatalf("Expected no error, got %v", err) } - asw.AddVolumeNode(volumeName, volumeSpec, nodeName, "") + asw.AddVolumeNode(volumeName, volumeSpec, nodeName, "", true) metricCollector := newAttachDetachStateCollector( nil, diff --git a/pkg/controller/volume/attachdetach/reconciler/reconciler.go b/pkg/controller/volume/attachdetach/reconciler/reconciler.go index deb257e00f..24c4e802a9 100644 --- a/pkg/controller/volume/attachdetach/reconciler/reconciler.go +++ b/pkg/controller/volume/attachdetach/reconciler/reconciler.go @@ -178,7 +178,6 @@ func (rc *reconciler) reconcile() { 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 // This check must be done before we do any other checks, as otherwise the other checks // may pass while at the same time the volume leaves the pending state, resulting in @@ -198,7 +197,7 @@ func (rc *reconciler) reconcile() { timeout := elapsedTime > rc.maxWaitForUnmountDuration // Check whether volume is still mounted. Skip detach if it is still mounted unless timeout if attachedVolume.MountedByNode && !timeout { - klog.V(12).Infof(attachedVolume.GenerateMsgDetailed("Cannot detach volume because it is still mounted", "")) + klog.V(5).Infof(attachedVolume.GenerateMsgDetailed("Cannot detach volume because it is still mounted", "")) continue } @@ -253,7 +252,7 @@ func (rc *reconciler) reconcile() { func (rc *reconciler) attachDesiredVolumes() { // Ensure volumes that should be attached are attached. for _, volumeToAttach := range rc.desiredStateOfWorld.GetVolumesToAttach() { - if rc.actualStateOfWorld.VolumeNodeExists(volumeToAttach.VolumeName, volumeToAttach.NodeName) { + if rc.actualStateOfWorld.IsVolumeAttachedToNode(volumeToAttach.VolumeName, volumeToAttach.NodeName) { // Volume/Node exists, touch it to reset detachRequestedTime if klog.V(5) { klog.Infof(volumeToAttach.GenerateMsgDetailed("Volume attached--touching", "")) @@ -270,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 72e2db61ab..a16a7af859 100644 --- a/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go +++ b/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go @@ -377,7 +377,7 @@ func Test_Run_OneVolumeAttachAndDetachMultipleNodesWithReadWriteMany(t *testing. volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) volumeSpec.PersistentVolume.Spec.AccessModes = []v1.PersistentVolumeAccessMode{v1.ReadWriteMany} nodeName1 := k8stypes.NodeName("node-name1") - nodeName2 := k8stypes.NodeName("node-name2") + nodeName2 := k8stypes.NodeName(volumetesting.MultiAttachNode) dsw.AddNode(nodeName1, false /*keepTerminatedPodVolumes*/) dsw.AddNode(nodeName2, false /*keepTerminatedPodVolumes*/) @@ -385,6 +385,7 @@ func Test_Run_OneVolumeAttachAndDetachMultipleNodesWithReadWriteMany(t *testing. if podAddErr != nil { t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) } + _, podAddErr = dsw.AddPod(types.UniquePodName(podName2), controllervolumetesting.NewPod(podName2, podName2), volumeSpec, nodeName2) if podAddErr != nil { t.Fatalf("AddPod failed. Expected: Actual: <%v>", podAddErr) @@ -496,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 @@ -532,6 +533,139 @@ func Test_Run_OneVolumeAttachAndDetachMultipleNodesWithReadWriteOnce(t *testing. waitForTotalAttachCallCount(t, 2 /* expectedAttachCallCount */, fakePlugin) } +// Creates a volume with accessMode ReadWriteOnce +// First create a pod which will try to attach the volume to the a node named "uncertain-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_OneVolumeAttachAndDetachUncertainNodesWithReadWriteOnce(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.UncertainAttachNode) + 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 uncertain 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) + } + + 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) + 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 + 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) + +} + +// 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) + +} + func Test_ReportMultiAttachError(t *testing.T) { type nodeWithPods struct { name k8stypes.NodeName @@ -868,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 } @@ -883,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, @@ -905,6 +1039,149 @@ func verifyNewAttacherCallCount( } } +func waitForVolumeAttachedToNode( + t *testing.T, + volumeName v1.UniqueVolumeName, + nodeName k8stypes.NodeName, + asw cache.ActualStateOfWorld) { + + err := retryWithExponentialBackOff( + time.Duration(500*time.Millisecond), + func() (bool, error) { + if asw.IsVolumeAttachedToNode(volumeName, nodeName) { + return true, nil + } + t.Logf( + "Warning: Volume <%v> is not attached to node <%v> yet. Will retry.", + volumeName, + nodeName) + + return false, nil + }, + ) + + if err != nil && !asw.IsVolumeAttachedToNode(volumeName, nodeName) { + t.Fatalf( + "Volume <%v> is not attached to node <%v>.", + volumeName, + nodeName) + } +} + +func waitForVolumeAddedToNode( + t *testing.T, + volumeName v1.UniqueVolumeName, + nodeName k8stypes.NodeName, + asw cache.ActualStateOfWorld) { + + err := retryWithExponentialBackOff( + time.Duration(500*time.Millisecond), + func() (bool, error) { + volumes := asw.GetAttachedVolumes() + for _, volume := range volumes { + if volume.VolumeName == volumeName && volume.NodeName == nodeName { + return true, nil + } + } + t.Logf( + "Warning: Volume <%v> is not added to node <%v> yet. Will retry.", + volumeName, + nodeName) + + return false, nil + }, + ) + + if err != nil { + t.Fatalf( + "Volume <%v> is not added to node <%v>. %v", + volumeName, + nodeName, err) + } +} + +func waitForVolumeRemovedFromNode( + t *testing.T, + volumeName v1.UniqueVolumeName, + nodeName k8stypes.NodeName, + asw cache.ActualStateOfWorld) { + + err := retryWithExponentialBackOff( + time.Duration(500*time.Millisecond), + func() (bool, error) { + volumes := asw.GetAttachedVolumes() + exist := false + for _, volume := range volumes { + if volume.VolumeName == volumeName && volume.NodeName == nodeName { + exist = true + } + } + if exist { + t.Logf( + "Warning: Volume <%v> is not removed from the node <%v> yet. Will retry.", + volumeName, + nodeName) + + return false, nil + } + return true, nil + + }, + ) + + if err != nil { + t.Fatalf( + "Volume <%v> is not removed from node <%v>. %v", + volumeName, + nodeName, err) + } +} + +func verifyVolumeAttachedToNode( + t *testing.T, + volumeName v1.UniqueVolumeName, + nodeName k8stypes.NodeName, + isAttached bool, + asw cache.ActualStateOfWorld, +) { + result := asw.IsVolumeAttachedToNode(volumeName, nodeName) + if result == isAttached { + return + } + t.Fatalf("Check volume <%v> is attached to node <%v>, got %v, expected %v", + volumeName, + nodeName, + result, + isAttached) + +} + +func verifyVolumeReportedAsAttachedToNode( + t *testing.T, + volumeName v1.UniqueVolumeName, + nodeName k8stypes.NodeName, + isAttached bool, + asw cache.ActualStateOfWorld, +) { + result := false + volumes := asw.GetVolumesToReportAttached() + for _, volume := range volumes[nodeName] { + if volume.Name == volumeName { + result = true + } + } + + if result == isAttached { + return + } + t.Fatalf("Check volume <%v> is reported as attached to node <%v>, got %v, expected %v", + volumeName, + nodeName, + result, + isAttached) + +} + func verifyNewDetacherCallCount( t *testing.T, expectZeroNewDetacherCallCount bool, diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index 43a9b0f8cd..360d3ab0b7 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -313,6 +313,11 @@ func (asw *actualStateOfWorld) MarkVolumeAsAttached( return asw.addVolume(volumeName, volumeSpec, devicePath) } +func (asw *actualStateOfWorld) MarkVolumeAsUncertain( + volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, _ types.NodeName) error { + return nil +} + func (asw *actualStateOfWorld) MarkVolumeAsDetached( volumeName v1.UniqueVolumeName, nodeName types.NodeName) { asw.DeleteVolume(volumeName) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 26acb12811..129b8de73c 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -46,9 +46,19 @@ import ( "k8s.io/kubernetes/pkg/volume/util/volumepathhandler" ) -// A hook specified in storage class to indicate it's provisioning -// is expected to fail. -const ExpectProvisionFailureKey = "expect-provision-failure" +const ( + // A hook specified in storage class to indicate it's provisioning + // is expected to fail. + ExpectProvisionFailureKey = "expect-provision-failure" + // The node is marked as uncertain. The attach operation will fail and return timeout error + // 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" +) // fakeVolumeHost is useful for testing volume plugins. type fakeVolumeHost struct { @@ -273,10 +283,20 @@ var _ DeviceMountableVolumePlugin = &FakeVolumePlugin{} var _ FSResizableVolumePlugin = &FakeVolumePlugin{} func (plugin *FakeVolumePlugin) getFakeVolume(list *[]*FakeVolume) *FakeVolume { + volumeList := *list + if list != nil && len(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, UnmountDeviceHook: plugin.UnmountDeviceHook, } + volume.VolumesAttached = make(map[string]types.NodeName) *list = append(*list, volume) return volume } @@ -333,6 +353,8 @@ func (plugin *FakeVolumePlugin) NewMounter(spec *Spec, pod *v1.Pod, opts VolumeO plugin.Lock() defer plugin.Unlock() volume := plugin.getFakeVolume(&plugin.Mounters) + volume.Lock() + defer volume.Unlock() volume.PodUID = pod.UID volume.VolName = spec.Name() volume.Plugin = plugin @@ -350,6 +372,8 @@ func (plugin *FakeVolumePlugin) NewUnmounter(volName string, podUID types.UID) ( plugin.Lock() defer plugin.Unlock() volume := plugin.getFakeVolume(&plugin.Unmounters) + volume.Lock() + defer volume.Unlock() volume.PodUID = podUID volume.VolName = volName volume.Plugin = plugin @@ -368,6 +392,8 @@ func (plugin *FakeVolumePlugin) NewBlockVolumeMapper(spec *Spec, pod *v1.Pod, op plugin.Lock() defer plugin.Unlock() volume := plugin.getFakeVolume(&plugin.BlockVolumeMappers) + volume.Lock() + defer volume.Unlock() if pod != nil { volume.PodUID = pod.UID } @@ -388,6 +414,8 @@ func (plugin *FakeVolumePlugin) NewBlockVolumeUnmapper(volName string, podUID ty plugin.Lock() defer plugin.Unlock() volume := plugin.getFakeVolume(&plugin.BlockVolumeUnmappers) + volume.Lock() + defer volume.Unlock() volume.PodUID = podUID volume.VolName = volName volume.Plugin = plugin @@ -428,7 +456,16 @@ func (plugin *FakeVolumePlugin) NewDetacher() (Detacher, error) { plugin.Lock() defer plugin.Unlock() plugin.NewDetacherCallCount = plugin.NewDetacherCallCount + 1 - return plugin.getFakeVolume(&plugin.Detachers), nil + detacher := plugin.getFakeVolume(&plugin.Detachers) + attacherList := plugin.Attachers + if attacherList != nil && len(attacherList) > 0 { + detacherList := plugin.Detachers + if detacherList != nil && len(detacherList) > 0 { + detacherList[0].VolumesAttached = attacherList[0].VolumesAttached + } + + } + return detacher, nil } func (plugin *FakeVolumePlugin) NewDeviceUnmounter() (DeviceUnmounter, error) { @@ -657,6 +694,7 @@ type FakeVolume struct { VolName string Plugin *FakeVolumePlugin MetricsNil + VolumesAttached map[string]types.NodeName // Add callbacks as needed WaitForAttachHook func(spec *Spec, devicePath string, pod *v1.Pod, spectimeout time.Duration) (string, error) @@ -677,6 +715,20 @@ type FakeVolume struct { PodDeviceMapPathCallCount int } +func getUniqueVolumeName(spec *Spec) (string, error) { + var volumeName string + if spec.Volume != nil && spec.Volume.GCEPersistentDisk != nil { + volumeName = spec.Volume.GCEPersistentDisk.PDName + } else if spec.PersistentVolume != nil && + spec.PersistentVolume.Spec.GCEPersistentDisk != nil { + volumeName = spec.PersistentVolume.Spec.GCEPersistentDisk.PDName + } + if volumeName == "" { + volumeName = spec.Name() + } + return volumeName, nil +} + func (_ *FakeVolume) GetAttributes() Attributes { return Attributes{ ReadOnly: false, @@ -822,6 +874,25 @@ func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error fv.Lock() defer fv.Unlock() fv.AttachCallCount++ + volumeName, err := getUniqueVolumeName(spec) + if err != nil { + return "", err + } + volumeNode, exist := fv.VolumesAttached[volumeName] + if exist { + if nodeName == UncertainAttachNode { + return "/dev/vdb-test", nil + } + if volumeNode == nodeName || volumeNode == MultiAttachNode || nodeName == MultiAttachNode { + return "/dev/vdb-test", nil + } + return "", fmt.Errorf("volume %q trying to attach to node %q is already attached to node %q", volumeName, nodeName, volumeNode) + } + + fv.VolumesAttached[volumeName] = nodeName + if nodeName == UncertainAttachNode || nodeName == TimeoutAttachNode { + return "", fmt.Errorf("Timed out to attach volume %q to node %q", volumeName, nodeName) + } return "/dev/vdb-test", nil } @@ -871,6 +942,10 @@ func (fv *FakeVolume) Detach(volumeName string, nodeName types.NodeName) error { fv.Lock() defer fv.Unlock() fv.DetachCallCount++ + if _, exist := fv.VolumesAttached[volumeName]; !exist { + return fmt.Errorf("Trying to detach volume %q that is not attached to the node %q", volumeName, nodeName) + } + delete(fv.VolumesAttached, volumeName) return nil } @@ -1037,7 +1112,7 @@ func VerifyAttachCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetAttachCallCount() - if actualCallCount == expectedAttachCallCount { + if actualCallCount >= expectedAttachCallCount { return nil } } @@ -1070,7 +1145,7 @@ func VerifyWaitForAttachCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetWaitForAttachCallCount() - if actualCallCount == expectedWaitForAttachCallCount { + if actualCallCount >= expectedWaitForAttachCallCount { return nil } } @@ -1103,7 +1178,7 @@ func VerifyMountDeviceCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetMountDeviceCallCount() - if actualCallCount == expectedMountDeviceCallCount { + if actualCallCount >= expectedMountDeviceCallCount { return nil } } diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index cfcc249f8b..ad24f1c1ef 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -192,6 +192,12 @@ 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 MarkVolumeAsDetached(volumeName v1.UniqueVolumeName, nodeName types.NodeName) diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 9fec3f9f3f..182792378d 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -327,6 +327,12 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( klog.Errorf("AttachVolume.MarkVolumeAsAttached failed to fix dangling volume error for volume %q with %s", volumeToAttach.VolumeName, addErr) } + } 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)