github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-29 22:46:12 +00:00
Code Issues Projects Releases Wiki Activity

Address comments

Browse Source 
This commit addressed the comment and also add a unit test.
This commit is contained in:
Jing Xu 2018-11-01 16:43:05 -07:00
parent 562d0fea53
commit 7bac6ca73a
11 changed files with 180 additions and 97 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
pkg/controller/volume/attachdetach/attach_detach_controller.go
View File

@ -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 {

2
pkg/controller/volume/attachdetach/attach_detach_controller_test.go
View File

@ -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() {

54
pkg/controller/volume/attachdetach/cache/actual_state_of_world.go vendored
View File

@ -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)
}
}

90
pkg/controller/volume/attachdetach/cache/actual_state_of_world_test.go vendored
View File

@ -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: <node %q exist> Expect: <node does not exist in the reportedAsAttached map", nodeName)
}
volumesForNode := asw.GetAllVolumesForNode(nodeName)
volumesForNode := asw.GetAttachedVolumesForNode(nodeName)
if len(volumesForNode) != 1 {
t.Fatalf("len(attachedVolumes) Expected: <1> Actual: <%v>", len(volumesForNode))
}
@ -111,7 +111,7 @@ func Test_AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached(t *testing.T)
t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Actual: <node %q exist> Expect: <node does not exist in the reportedAsAttached map", nodeName)
}
nodes := asw.GetNodesForVolume(volumeName)
nodes := asw.GetNodesForAttachedVolume(volumeName)
if len(nodes) > 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: <node %q does not exist> Expect: <node does exist in the reportedAsAttached map", node2Name)
}
nodes := asw.GetNodesForVolume(volumeName)
nodes := asw.GetNodesForAttachedVolume(volumeName)
if len(nodes) != 1 {
t.Fatalf("AddVolumeNode_Positive_NewVolumeNewNodeWithFalseAttached failed. Expect one node returned.")
}
@ -278,7 +278,7 @@ func Test_AddVolumeNode_Positive_ExistingVolumeNewNode(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) != 2 {
t.Fatalf("len(attachedVolumes) Expected: <2> 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 <no error> 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: <no error> 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 <no error> 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: <no error> 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: <no error> 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 <no error> 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 {

2
pkg/controller/volume/attachdetach/metrics/metrics.go
View File

@ -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 {

6
pkg/controller/volume/attachdetach/reconciler/reconciler.go
View File

@ -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)

87
pkg/controller/volume/attachdetach/reconciler/reconciler_test.go
View File

@ -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: <no error> 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: <no error> 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: <no error> 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: <no error> 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,

2
pkg/controller/volume/attachdetach/testing/testvolumespec.go
View File

@ -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)

16
pkg/volume/testing/testing.go
View File

@ -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

4
pkg/volume/util/operationexecutor/operation_executor.go
View File

@ -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

12
pkg/volume/util/operationexecutor/operation_generator.go
View File

@ -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)