From 47331cf0a270f37263cac9a23e45222e5eb57f19 Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Tue, 2 Oct 2018 13:24:28 -0700 Subject: [PATCH 1/3] WIP: Handle failed attach operation leave uncertain volume attach state This PR fixes issue #32727. When an attach operation fails, it is still possible that the volume will be attached to the node later. This PR adds the logic to record the volume to node with attached state no matter whether the operation succedded or not. If the operation fails, mark the attached state to false. If the operation succeeded, mark the attached state to true. The reconciler will still issue attach operation until it returns successfully. If the pod is removed in the mean time, the reconciler will issue detach operations for all the volumes no matter what is the attached state. --- .../cache/actual_state_of_world.go | 108 ++++++++++-------- .../operationexecutor/operation_executor.go | 2 + .../operationexecutor/operation_generator.go | 6 + 3 files changed, 67 insertions(+), 49 deletions(-) diff --git a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go index f25fb00881..33f0a9fd34 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go @@ -56,7 +56,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 @@ -97,21 +97,27 @@ type ActualStateOfWorld interface { // VolumeNodeExists 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 + // reflecting which volumes might attached to which nodes based on the // current actual state of the world. - GetAttachedVolumes() []AttachedVolume + GetAllVolumes() []AttachedVolume - // GetAttachedVolumes generates and returns a list of volumes attached to + // GetAttachedVolumes 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 + + // 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. - GetAttachedVolumesForNode(nodeName types.NodeName) []AttachedVolume - + // 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 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 // GetVolumesToReportAttached returns a map containing the set of nodes for @@ -185,7 +191,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 trying to be 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 +200,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 +210,8 @@ 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 + // attached indicates that the volume is confirmed to be attached to this node + attached bool // detachRequestedTime used to capture the desire to detach this volume detachRequestedTime time.Time @@ -235,9 +239,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) + 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 +272,12 @@ 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 == "" { attachableVolumePlugin, err := asw.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec) if err != nil || attachableVolumePlugin == nil { return "", fmt.Errorf( @@ -283,12 +294,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] @@ -311,22 +316,26 @@ func (asw *actualStateOfWorld) AddVolumeNode( } 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 + attached: isAttached, + detachRequestedTime: time.Time{}, } } else { + node.attached = isAttached klog.V(5).Infof("Volume %q is already added to attachedVolume list to the node %q", volumeName, nodeName) } - - asw.addVolumeToReportAsAttached(volumeName, nodeName) + volumeObj.nodesAttachedTo[nodeName] = node + + if isAttached { + asw.addVolumeToReportAsAttached(volumeName, nodeName) + } return volumeName, nil } @@ -340,11 +349,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,22 +519,24 @@ 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 { + if node.attached == true { + return true + } } } return false } -func (asw *actualStateOfWorld) GetAttachedVolumes() []AttachedVolume { +func (asw *actualStateOfWorld) GetAllVolumes() []AttachedVolume { asw.RLock() defer asw.RUnlock() @@ -546,7 +552,7 @@ func (asw *actualStateOfWorld) GetAttachedVolumes() []AttachedVolume { return attachedVolumes } -func (asw *actualStateOfWorld) GetAttachedVolumesForNode( +func (asw *actualStateOfWorld) GetAllVolumesForNode( nodeName types.NodeName) []AttachedVolume { asw.RLock() defer asw.RUnlock() @@ -574,9 +580,11 @@ 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.attached { + volumes := attachedVolumesPerNode[nodeName] + volumes = append(volumes, getAttachedVolume(&volumeObj, &nodeObj).AttachedVolume) + attachedVolumesPerNode[nodeName] = volumes + } } } @@ -593,8 +601,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.attached { + nodes = append(nodes, k) + } } return nodes } diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index 745910dc22..baf2b83f3d 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -192,6 +192,8 @@ type ActualStateOfWorldAttacherUpdater interface { // volumes. See issue 29695. MarkVolumeAsAttached(volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) error + 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 36c10b6775..ea4e16d55a 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -323,6 +323,12 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( } } + 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) + } // On failure, return error. Caller will log and retry. return volumeToAttach.GenerateError("AttachVolume.Attach failed", attachErr) } From 562d0fea53ded469368b4d2a0bdfd0e33ece1037 Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Mon, 8 Oct 2018 14:37:11 -0700 Subject: [PATCH 2/3] Handle failed attach operation leave uncertain volume attach state This commit adds the unit tests for the PR. It also includes some files that are affected by the function name changes. --- .../attachdetach/attach_detach_controller.go | 4 +- .../attach_detach_controller_test.go | 4 +- .../volume/attachdetach/cache/BUILD | 1 + .../cache/actual_state_of_world.go | 8 +- .../cache/actual_state_of_world_test.go | 380 ++++++++++++++---- .../volume/attachdetach/metrics/metrics.go | 2 +- .../attachdetach/metrics/metrics_test.go | 2 +- .../attachdetach/reconciler/reconciler.go | 9 +- .../reconciler/reconciler_test.go | 212 +++++++++- .../attachdetach/testing/testvolumespec.go | 2 +- .../cache/actual_state_of_world.go | 5 + pkg/volume/testing/testing.go | 81 +++- 12 files changed, 598 insertions(+), 112 deletions(-) diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index f76e3224a6..7a8fdb99b5 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) @@ -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.GetAttachedVolumesForNode(nodeName) { + for _, attachedVolume := range adc.actualStateOfWorld.GetAllVolumesForNode(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 f850302521..58894ddde8 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) } @@ -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.GetAttachedVolumes() { + for _, volumeList := range testPlugin.GetAllVolumes() { attachedVolumesNum += len(volumeList) } for _, volumeList := range testPlugin.GetDetachedVolumes() { 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 33f0a9fd34..11df3a4c8f 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. @@ -332,7 +334,7 @@ func (asw *actualStateOfWorld) AddVolumeNode( nodeName) } volumeObj.nodesAttachedTo[nodeName] = node - + if isAttached { asw.addVolumeToReportAsAttached(volumeName, nodeName) } 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..ab45693ec2 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,19 +40,19 @@ 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) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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.GetAllVolumes() + 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.GetAllVolumes() + 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) + 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.GetAllVolumes() + 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) + 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>", len(attachedVolumes)) } @@ -121,8 +299,8 @@ func Test_AddVolumeNode_Positive_ExistingVolumeExistingNode(t *testing.T) { devicePath := "fake/device/path" // Act - generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) - generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath) + generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) + generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeName, volumeSpec, nodeName, devicePath, true) // Assert if add1Err != nil { @@ -139,12 +317,12 @@ func Test_AddVolumeNode_Positive_ExistingVolumeExistingNode(t *testing.T) { generatedVolumeName2) } - volumeNodeComboExists := asw.VolumeNodeExists(generatedVolumeName1, nodeName) + volumeNodeComboExists := asw.IsVolumeAttachedToNode(generatedVolumeName1, nodeName) if !volumeNodeComboExists { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName1, nodeName) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -163,7 +341,7 @@ func Test_DeleteVolumeNode_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) } @@ -172,12 +350,12 @@ 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) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } @@ -196,12 +374,12 @@ 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) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } @@ -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,17 +417,17 @@ 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) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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,20 +446,20 @@ 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 { t.Fatalf("%q/%q volume/node combo does not exist, it should.", generatedVolumeName, nodeName) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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,20 +479,20 @@ 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 { t.Fatalf("%q/%q volume/node combo exists, it should not.", generatedVolumeName, node2Name) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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,20 +510,20 @@ 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 { t.Fatalf("%q/%q volume/node combo exists, it should not.", volumeName, nodeName) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 0 { t.Fatalf("len(attachedVolumes) Expected: <0> Actual: <%v>", len(attachedVolumes)) } } -// Calls GetAttachedVolumes() on empty data struct. +// Calls GetAllVolumes() on empty data struct. // Verifies no volume/node entries are returned. func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { // Arrange @@ -353,7 +531,7 @@ func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { asw := NewActualStateOfWorld(volumePluginMgr) // Act - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() // Assert if len(attachedVolumes) != 0 { @@ -362,7 +540,7 @@ func Test_GetAttachedVolumes_Positive_NoVolumesOrNodes(t *testing.T) { } // Populates data struct with one volume/node entry. -// Calls GetAttachedVolumes() to get list of entries. +// Calls GetAllVolumes() to get list of entries. // Verifies one volume/node entry is returned. func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { // Arrange @@ -372,13 +550,13 @@ 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) } // Act - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() // Assert if len(attachedVolumes) != 1 { @@ -389,7 +567,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { } // Populates data struct with two volume/node entries (different node and volume). -// Calls GetAttachedVolumes() to get list of entries. +// Calls GetAllVolumes() to get list of entries. // Verifies both volume/node entries are returned. func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { // Arrange @@ -399,20 +577,20 @@ 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) } // Act - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() // Assert if len(attachedVolumes) != 2 { @@ -424,7 +602,7 @@ func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { } // Populates data struct with two volume/node entries (same volume different node). -// Calls GetAttachedVolumes() to get list of entries. +// Calls GetAllVolumes() to get list of entries. // Verifies both volume/node entries are returned. func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { // Arrange @@ -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) } @@ -452,7 +638,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() // Assert if len(attachedVolumes) != 2 { @@ -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) } @@ -481,7 +667,7 @@ func Test_SetVolumeMountedByNode_Positive_Set(t *testing.T) { // Act: do not mark -- test default value // Assert - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -517,7 +703,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSet(t *testing.T) { t.Fatalf("SetVolumeMountedByNode2 failed. Expected Actual: <%v>", setVolumeMountedErr2) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -536,12 +722,12 @@ 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) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -556,7 +742,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithoutInitialSet(t *testing.T) { t.Fatalf("SetVolumeMountedByNode failed. Expected Actual: <%v>", setVolumeMountedErr) } - attachedVolumes = asw.GetAttachedVolumes() + attachedVolumes = asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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 { @@ -597,7 +783,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetAddVolumeNodeNotRes t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", addErr) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -629,7 +815,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest if err != nil { t.Fatalf("RemoveVolumeFromReportAsAttached failed. Expected: Actual: <%v>", err) } - expectedDetachRequestedTime := asw.GetAttachedVolumes()[0].DetachRequestedTime + expectedDetachRequestedTime := asw.GetAllVolumes()[0].DetachRequestedTime // Act setVolumeMountedErr1 := asw.SetVolumeMountedByNode(generatedVolumeName, nodeName, true /* mounted */) @@ -643,7 +829,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest t.Fatalf("SetVolumeMountedByNode2 failed. Expected Actual: <%v>", setVolumeMountedErr2) } - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -672,7 +858,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Set(t *testing.T) { // Act: do not mark -- test default value // Assert - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -707,7 +893,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Marked(t *testing.T) { } // Assert - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -750,7 +936,7 @@ func Test_MarkDesireToDetach_Positive_MarkedAddVolumeNodeReset(t *testing.T) { } // Assert - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -794,7 +980,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_UnsetWithInitialSetVolumeMou } // Assert - attachedVolumes := asw.GetAttachedVolumes() + attachedVolumes := asw.GetAllVolumes() if len(attachedVolumes) != 1 { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } @@ -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) } @@ -958,14 +1144,14 @@ func Test_SetDetachRequestTime_Positive(t *testing.T) { } } -func Test_GetAttachedVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { +func Test_GetAllVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) node := types.NodeName("random") // Act - attachedVolumes := asw.GetAttachedVolumesForNode(node) + attachedVolumes := asw.GetAllVolumesForNode(node) // Assert if len(attachedVolumes) != 0 { @@ -973,7 +1159,7 @@ func Test_GetAttachedVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { } } -func Test_GetAttachedVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { +func Test_GetAllVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -981,13 +1167,13 @@ 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) } // Act - attachedVolumes := asw.GetAttachedVolumesForNode(nodeName) + attachedVolumes := asw.GetAllVolumesForNode(nodeName) // Assert if len(attachedVolumes) != 1 { @@ -997,7 +1183,7 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } -func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { +func Test_GetAllVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -1005,20 +1191,20 @@ 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) } // Act - attachedVolumes := asw.GetAttachedVolumesForNode(node2Name) + attachedVolumes := asw.GetAllVolumesForNode(node2Name) // Assert if len(attachedVolumes) != 1 { @@ -1028,7 +1214,7 @@ func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } -func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { +func Test_GetAllVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) asw := NewActualStateOfWorld(volumePluginMgr) @@ -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) } @@ -1054,7 +1248,7 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { } // Act - attachedVolumes := asw.GetAttachedVolumesForNode(node1Name) + attachedVolumes := asw.GetAllVolumesForNode(node1Name) // Assert if len(attachedVolumes) != 1 { @@ -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) } @@ -1091,7 +1293,7 @@ func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) { } // Act - attachedVolumes := asw.GetAttachedVolumesForNode(node2Name) + attachedVolumes := asw.GetAllVolumesForNode(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 86f99c5e8b..1ff42733cf 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.GetAttachedVolumes() { + for _, v := range collector.asw.GetAllVolumes() { if plugin, err := collector.volumePluginMgr.FindPluginBySpec(v.VolumeSpec); err == nil { pluginName := pluginNameNotAvailable if plugin != nil { 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..7ccff1bb7f 100644 --- a/pkg/controller/volume/attachdetach/reconciler/reconciler.go +++ b/pkg/controller/volume/attachdetach/reconciler/reconciler.go @@ -175,16 +175,15 @@ 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.GetAttachedVolumes() { + for _, attachedVolume := range rc.actualStateOfWorld.GetAllVolumes() { 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 // double detach attempts if rc.attacherDetacher.IsOperationPending(attachedVolume.VolumeName, "") { - klog.V(10).Infof("Operation for volume %q is already running. Can't start detach for %q", attachedVolume.VolumeName, attachedVolume.NodeName) + klog.V(5).Infof("Operation for volume %q is already running. Can't start detach for %q", attachedVolume.VolumeName, attachedVolume.NodeName) continue } @@ -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", "")) diff --git a/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go b/pkg/controller/volume/attachdetach/reconciler/reconciler_test.go index 72e2db61ab..aa2d5ccd21 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) @@ -532,6 +533,72 @@ 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) + } + + // 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) + + // 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) + veriryVolumeAttachedToNode(t, generatedVolumeName, nodeName2, true, asw) + +} + func Test_ReportMultiAttachError(t *testing.T) { type nodeWithPods struct { name k8stypes.NodeName @@ -905,6 +972,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.GetAllVolumes() + 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.GetAllVolumes() + 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 veriryVolumeAttachedToNode( + 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 veriryVolumeReportedAsAttachedToNode( + 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/controller/volume/attachdetach/testing/testvolumespec.go b/pkg/controller/volume/attachdetach/testing/testvolumespec.go index a243e724b2..15af6eb182 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) GetAttachedVolumes() map[string][]string { +func (plugin *TestPlugin) GetAllVolumes() map[string][]string { plugin.pluginLock.RLock() defer plugin.pluginLock.RUnlock() ret := make(map[string][]string) diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index a7b06af94e..ef70976c4c 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -308,6 +308,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 4c116aa593..852373e1bd 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -46,9 +46,16 @@ 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 + // but the operation is actually succeeded. + UncertainAttachNode = "uncertain-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 +280,15 @@ var _ DeviceMountableVolumePlugin = &FakeVolumePlugin{} var _ FSResizableVolumePlugin = &FakeVolumePlugin{} func (plugin *FakeVolumePlugin) getFakeVolume(list *[]*FakeVolume) *FakeVolume { + volumeList := *list + if list != nil && len(volumeList) > 0 { + return volumeList[0] + } volume := &FakeVolume{ WaitForAttachHook: plugin.WaitForAttachHook, UnmountDeviceHook: plugin.UnmountDeviceHook, } + volume.VolumesAttached = make(map[string]types.NodeName) *list = append(*list, volume) return volume } @@ -329,6 +341,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 @@ -346,6 +360,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 @@ -364,6 +380,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 } @@ -384,6 +402,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 @@ -424,7 +444,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) { @@ -557,6 +586,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) @@ -577,6 +607,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, @@ -722,6 +766,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 "", fmt.Errorf("Timed out to attach volume %q to node %q", volumeName, nodeName) + } + 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 { + return "", fmt.Errorf("Timed out to attach volume %q to node %q", volumeName, nodeName) + } return "/dev/vdb-test", nil } @@ -771,6 +834,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 } @@ -937,7 +1004,7 @@ func VerifyAttachCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetAttachCallCount() - if actualCallCount == expectedAttachCallCount { + if actualCallCount >= expectedAttachCallCount { return nil } } @@ -970,7 +1037,7 @@ func VerifyWaitForAttachCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetWaitForAttachCallCount() - if actualCallCount == expectedWaitForAttachCallCount { + if actualCallCount >= expectedWaitForAttachCallCount { return nil } } @@ -1003,7 +1070,7 @@ func VerifyMountDeviceCallCount( fakeVolumePlugin *FakeVolumePlugin) error { for _, attacher := range fakeVolumePlugin.GetAttachers() { actualCallCount := attacher.GetMountDeviceCallCount() - if actualCallCount == expectedMountDeviceCallCount { + if actualCallCount >= expectedMountDeviceCallCount { return nil } } From 7bac6ca73ad6fa1b0b637e801a524d6a1df663ea Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Thu, 1 Nov 2018 16:43:05 -0700 Subject: [PATCH 3/3] Address comments This commit addressed the comment and also add a unit test. --- .../attachdetach/attach_detach_controller.go | 2 +- .../attach_detach_controller_test.go | 2 +- .../cache/actual_state_of_world.go | 54 +++++------ .../cache/actual_state_of_world_test.go | 90 +++++++++---------- .../volume/attachdetach/metrics/metrics.go | 2 +- .../attachdetach/reconciler/reconciler.go | 6 +- .../reconciler/reconciler_test.go | 87 +++++++++++++++--- .../attachdetach/testing/testvolumespec.go | 2 +- pkg/volume/testing/testing.go | 16 +++- .../operationexecutor/operation_executor.go | 4 + .../operationexecutor/operation_generator.go | 12 +-- 11 files changed, 180 insertions(+), 97 deletions(-) diff --git a/pkg/controller/volume/attachdetach/attach_detach_controller.go b/pkg/controller/volume/attachdetach/attach_detach_controller.go index 7a8fdb99b5..d2dd7b3de6 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 58894ddde8..ec66fab1f8 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 11df3a4c8f..4322f15a60 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 ab45693ec2..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 @@ -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 1ff42733cf..86f99c5e8b 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 7ccff1bb7f..24c4e802a9 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 aa2d5ccd21..a16a7af859 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 15af6eb182..a243e724b2 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 852373e1bd..81c46ec0c2 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 baf2b83f3d..0096e63945 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 ea4e16d55a..c55ab64e68 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)