From 9e8edf6baf06cdbf662dab3399c61fb5da6861ca Mon Sep 17 00:00:00 2001 From: Jing Xu Date: Thu, 29 Sep 2016 15:30:34 -0700 Subject: [PATCH] Fix issue in updating device path when volume is attached multiple times When volume is attached, it is possible that the actual state already has this volume object (e.g., the volume is attached to multiple nodes, or volume was detached and attached again). We need to update the device path in such situation, otherwise, the device path would be stale information and cause kubelet mount to the wrong device. This PR partially fixes issue #29324 --- .../cache/actual_state_of_world.go | 14 +- .../cache/actual_state_of_world_test.go | 142 +++++++++++------- .../cache/actual_state_of_world.go | 9 +- 3 files changed, 111 insertions(+), 54 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 3cc8964f5e1..2826125b024 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go @@ -279,8 +279,17 @@ func (asw *actualStateOfWorld) AddVolumeNode( nodesAttachedTo: make(map[types.NodeName]nodeAttachedTo), devicePath: devicePath, } - asw.attachedVolumes[volumeName] = volumeObj + } else { + // If volume object already exists, it indicates that the information would be out of date. + // Update the fields for volume object except the nodes attached to the volumes. + volumeObj.devicePath = devicePath + volumeObj.spec = volumeSpec + glog.V(2).Infof("Volume %q is already added to attachedVolume list to node %q, update device path %q", + volumeName, + nodeName, + devicePath) } + asw.attachedVolumes[volumeName] = volumeObj _, nodeExists := volumeObj.nodesAttachedTo[nodeName] if !nodeExists { @@ -323,7 +332,7 @@ func (asw *actualStateOfWorld) SetVolumeMountedByNode( nodeObj.mountedByNode = mounted volumeObj.nodesAttachedTo[nodeName] = nodeObj - glog.V(4).Infof("SetVolumeMountedByNode volume %v to the node %q mounted %q", + glog.V(4).Infof("SetVolumeMountedByNode volume %v to the node %q mounted %t", volumeName, nodeName, mounted) @@ -569,6 +578,7 @@ func getAttachedVolume( VolumeName: attachedVolume.volumeName, VolumeSpec: attachedVolume.spec, NodeName: nodeAttachedTo.nodeName, + DevicePath: attachedVolume.devicePath, PluginIsAttachable: true, }, MountedByNode: nodeAttachedTo.mountedByNode, 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 36a2c84597c..456cd0c32e5 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 @@ -56,7 +56,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNode(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Calls AddVolumeNode() twice. Second time use a different node name. @@ -105,8 +105,8 @@ func Test_AddVolumeNode_Positive_ExistingVolumeNewNode(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Calls AddVolumeNode() twice. Uses the same volume and node both times. @@ -149,7 +149,7 @@ func Test_AddVolumeNode_Positive_ExistingVolumeExistingNode(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -254,7 +254,7 @@ func Test_DeleteVolumeNode_Positive_TwoNodesOneDeleted(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -286,7 +286,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeExists(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume1/node1 entry. @@ -319,7 +319,7 @@ func Test_VolumeNodeExists_Positive_VolumeExistsNodeDoesntExist(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), node1Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Calls VolumeNodeExists() on empty data struct. @@ -385,7 +385,7 @@ func Test_GetAttachedVolumes_Positive_OneVolumeOneNode(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with two volume/node entries (different node and volume). @@ -419,8 +419,8 @@ func Test_GetAttachedVolumes_Positive_TwoVolumeTwoNodes(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volume1Name), node1Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volume1Name), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with two volume/node entries (same volume different node). @@ -459,8 +459,8 @@ func Test_GetAttachedVolumes_Positive_OneVolumeTwoNodes(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <2> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -486,7 +486,7 @@ func Test_SetVolumeMountedByNode_Positive_Set(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -522,7 +522,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSet(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, false /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, false /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -554,7 +554,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithoutInitialSet(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -595,7 +595,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetAddVolumeNodeNotRes t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, false /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, false /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -641,7 +641,7 @@ func Test_SetVolumeMountedByNode_Positive_UnsetWithInitialSetVerifyDetachRequest t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, false /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, false /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) if !expectedDetachRequestedTime.Equal(attachedVolumes[0].DetachRequestedTime) { t.Fatalf("DetachRequestedTime changed. Expected: <%v> Actual: <%v>", expectedDetachRequestedTime, attachedVolumes[0].DetachRequestedTime) } @@ -670,7 +670,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Set(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -705,7 +705,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_Marked(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -748,7 +748,7 @@ func Test_MarkDesireToDetach_Positive_MarkedAddVolumeNodeReset(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -792,7 +792,7 @@ func Test_RemoveVolumeFromReportAsAttached_Positive_UnsetWithInitialSetVolumeMou t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, false /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, false /* expectedMountedByNode */, true /* expectNonZeroDetachRequestedTime */) } // Populates data struct with one volume/node entry. @@ -951,33 +951,6 @@ func Test_SetDetachRequestTime_Positive(t *testing.T) { } } -func verifyAttachedVolume( - t *testing.T, - attachedVolumes []AttachedVolume, - expectedVolumeName api.UniqueVolumeName, - expectedVolumeSpecName string, - expectedNodeName types.NodeName, - expectedMountedByNode, - expectNonZeroDetachRequestedTime bool) { - for _, attachedVolume := range attachedVolumes { - if attachedVolume.VolumeName == expectedVolumeName && - attachedVolume.VolumeSpec.Name() == expectedVolumeSpecName && - attachedVolume.NodeName == expectedNodeName && - attachedVolume.MountedByNode == expectedMountedByNode && - attachedVolume.DetachRequestedTime.IsZero() == !expectNonZeroDetachRequestedTime { - return - } - } - - t.Fatalf( - "attachedVolumes (%v) should contain the volume/node combo %q/%q with MountedByNode=%v and NonZeroDetachRequestedTime=%v. It does not.", - attachedVolumes, - expectedVolumeName, - expectedNodeName, - expectedMountedByNode, - expectNonZeroDetachRequestedTime) -} - func Test_GetAttachedVolumesForNode_Positive_NoVolumesOrNodes(t *testing.T) { // Arrange volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) @@ -1014,7 +987,7 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeOneNode(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName, string(volumeName), nodeName, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { @@ -1045,7 +1018,7 @@ func Test_GetAttachedVolumesForNode_Positive_TwoVolumeTwoNodes(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volume2Name), node2Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { @@ -1081,5 +1054,72 @@ func Test_GetAttachedVolumesForNode_Positive_OneVolumeTwoNodes(t *testing.T) { t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) } - verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName1, string(volumeName), node1Name, devicePath, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) +} + +func Test_OneVolumeTwoNodes_TwoDevicePaths(t *testing.T) { + // Arrange + volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) + asw := NewActualStateOfWorld(volumePluginMgr) + volumeName := api.UniqueVolumeName("volume-name") + volumeSpec := controllervolumetesting.GetTestVolumeSpec(string(volumeName), volumeName) + node1Name := types.NodeName("node1-name") + devicePath1 := "fake/device/path1" + generatedVolumeName1, add1Err := asw.AddVolumeNode(volumeSpec, node1Name, devicePath1) + if add1Err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add1Err) + } + node2Name := types.NodeName("node2-name") + devicePath2 := "fake/device/path2" + generatedVolumeName2, add2Err := asw.AddVolumeNode(volumeSpec, node2Name, devicePath2) + if add2Err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", add2Err) + } + + if generatedVolumeName1 != generatedVolumeName2 { + t.Fatalf( + "Generated volume names for the same volume should be the same but they are not: %q and %q", + generatedVolumeName1, + generatedVolumeName2) + } + + // Act + attachedVolumes := asw.GetAttachedVolumesForNode(node2Name) + + // Assert + if len(attachedVolumes) != 1 { + t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(attachedVolumes)) + } + + verifyAttachedVolume(t, attachedVolumes, generatedVolumeName2, string(volumeName), node2Name, devicePath2, true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) +} + +func verifyAttachedVolume( + t *testing.T, + attachedVolumes []AttachedVolume, + expectedVolumeName api.UniqueVolumeName, + expectedVolumeSpecName string, + expectedNodeName types.NodeName, + expectedDevicePath string, + expectedMountedByNode, + expectNonZeroDetachRequestedTime bool) { + for _, attachedVolume := range attachedVolumes { + if attachedVolume.VolumeName == expectedVolumeName && + attachedVolume.VolumeSpec.Name() == expectedVolumeSpecName && + attachedVolume.NodeName == expectedNodeName && + attachedVolume.DevicePath == expectedDevicePath && + attachedVolume.MountedByNode == expectedMountedByNode && + attachedVolume.DetachRequestedTime.IsZero() == !expectNonZeroDetachRequestedTime { + return + } + } + + t.Fatalf( + "attachedVolumes (%v) should contain the volume/node combo %q/%q with DevicePath=%q MountedByNode=%v and NonZeroDetachRequestedTime=%v. It does not.", + attachedVolumes, + expectedVolumeName, + expectedNodeName, + expectedDevicePath, + expectedMountedByNode, + expectNonZeroDetachRequestedTime) } diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index 2b3061b8cc2..03a6eee739c 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go @@ -366,8 +366,15 @@ func (asw *actualStateOfWorld) addVolume( globallyMounted: false, devicePath: devicePath, } - asw.attachedVolumes[volumeName] = volumeObj + } else { + // If volume object already exists, update the fields such as device path + volumeObj.devicePath = devicePath + volumeObj.spec = volumeSpec + glog.V(2).Infof("Volume %q is already added to attachedVolume list, update device path %q", + volumeName, + devicePath) } + asw.attachedVolumes[volumeName] = volumeObj return nil }