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 23e39d8aa69..652d5421d9a 100644 --- a/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go +++ b/pkg/controller/volume/attachdetach/cache/actual_state_of_world.go @@ -675,7 +675,6 @@ func (asw *actualStateOfWorld) GetVolumesToReportAttachedForNode(nodeName types. asw.Lock() defer asw.Unlock() - var attachedVolumes []v1.AttachedVolume nodeToUpdateObj, ok := asw.nodesToUpdateStatusFor[nodeName] if !ok { return false, nil @@ -684,7 +683,7 @@ func (asw *actualStateOfWorld) GetVolumesToReportAttachedForNode(nodeName types. return false, nil } - attachedVolumes = asw.getAttachedVolumeFromUpdateObject(nodeToUpdateObj.volumesToReportAsAttached) + volumesToReportAttached := asw.getAttachedVolumeFromUpdateObject(nodeToUpdateObj.volumesToReportAsAttached) // When GetVolumesToReportAttached is called by node status updater, the current status // of this node will be updated, so set the flag statusUpdateNeeded to false indicating // the current status is already updated. @@ -692,7 +691,7 @@ func (asw *actualStateOfWorld) GetVolumesToReportAttachedForNode(nodeName types. klog.Errorf("Failed to update statusUpdateNeeded field when getting volumes: %v", err) } - return true, attachedVolumes + return true, volumesToReportAttached } func (asw *actualStateOfWorld) GetNodesToUpdateStatusFor() map[types.NodeName]nodeToUpdateStatusFor { 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 71406909a9a..984b4d7a7b1 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 @@ -1447,6 +1447,70 @@ func Test_MarkVolumeAsUncertain(t *testing.T) { verifyAttachedVolume(t, attachedVolumes, volumeName, string(volumeName), nodeName, "", true /* expectedMountedByNode */, false /* expectNonZeroDetachRequestedTime */) } +// Calls AddVolumeNode() once with attached set to true. +// Verifies GetVolumesToReportAttachedForNode has an update for the node. +// Call GetVolumesToReportAttachedForNode a second time for the node, verify it does not report +// an update is needed any more +// Then calls RemoveVolumeFromReportAsAttached() +// Verifies GetVolumesToReportAttachedForNode reports an update is needed +func Test_GetVolumesToReportAttachedForNode_Positive(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, true) + + // Assert + if err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", err) + } + + needsUpdate, attachedVolumes := asw.GetVolumesToReportAttachedForNode(nodeName) + if !needsUpdate { + t.Fatalf("GetVolumesToReportAttachedForNode_Positive_NewVolumeNewNodeWithTrueAttached failed. Actual: Expect: Actual: <%v>", len(attachedVolumes)) + } + + needsUpdate, _ = asw.GetVolumesToReportAttachedForNode(nodeName) + if needsUpdate { + t.Fatalf("GetVolumesToReportAttachedForNode_Positive_NewVolumeNewNodeWithTrueAttached failed. Actual: Expect: Actual: <%v>", removeVolumeDetachErr) + } + + needsUpdate, attachedVolumes = asw.GetVolumesToReportAttachedForNode(nodeName) + if !needsUpdate { + t.Fatalf("GetVolumesToReportAttachedForNode_Positive_NewVolumeNewNodeWithTrueAttached failed. Actual: Expect: Actual: <%v>", len(attachedVolumes)) + } +} + +// Verifies GetVolumesToReportAttachedForNode reports no update needed for an unknown node. +func Test_GetVolumesToReportAttachedForNode_UnknownNode(t *testing.T) { + // Arrange + volumePluginMgr, _ := volumetesting.GetTestVolumePluginMgr(t) + asw := NewActualStateOfWorld(volumePluginMgr) + nodeName := types.NodeName("node-name") + + needsUpdate, _ := asw.GetVolumesToReportAttachedForNode(nodeName) + if needsUpdate { + t.Fatalf("GetVolumesToReportAttachedForNode_UnknownNode failed. Actual: Expect: Actual: <%v>", err) + } + err = nodeInformer.Informer().GetStore().Add(&testNode2) + if err != nil { + t.Fatalf(".Informer().GetStore().Add failed. Expected: Actual: <%v>", err) + } + + volumeName1 := corev1.UniqueVolumeName("volume-name-1") + volumeName2 := corev1.UniqueVolumeName("volume-name-2") + volumeSpec1 := controllervolumetesting.GetTestVolumeSpec(string(volumeName1), volumeName1) + volumeSpec2 := controllervolumetesting.GetTestVolumeSpec(string(volumeName2), volumeName2) + + nodeName1 := types.NodeName("testnode-1") + nodeName2 := types.NodeName("testnode-2") + devicePath := "fake/device/path" + + _, err = asw.AddVolumeNode(volumeName1, volumeSpec1, nodeName1, devicePath, true) + if err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", err) + } + _, err = asw.AddVolumeNode(volumeName2, volumeSpec2, nodeName2, devicePath, true) + if err != nil { + t.Fatalf("AddVolumeNode failed. Expected: Actual: <%v>", err) + } + + return asw, fakeKubeClient, nsu +} + +// TestNodeStatusUpdater_UpdateNodeStatuses_TwoNodesUpdate calls setup +// calls UpdateNodeStatuses() +// check that asw.GetVolumesToReportAttached reports nothing left to attach +// checks that each node status.volumesAttached is of length 1 and contains the correct volume +func TestNodeStatusUpdater_UpdateNodeStatuses_TwoNodesUpdate(t *testing.T) { + ctx := context.Background() + asw, fakeKubeClient, nsu := setupNodeStatusUpdate(ctx, t) + + err := nsu.UpdateNodeStatuses() + if err != nil { + t.Fatalf("UpdateNodeStatuses failed. Expected: Actual: <%v>", err) + } + + needToReport := asw.GetVolumesToReportAttached() + if len(needToReport) != 0 { + t.Fatalf("len(asw.GetVolumesToReportAttached()) Expected: <0> Actual: <%v>", len(needToReport)) + } + + node, err := fakeKubeClient.CoreV1().Nodes().Get(ctx, "testnode-1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Nodes().Get failed. Expected: Actual: <%v>", err) + } + if len(node.Status.VolumesAttached) != 1 { + t.Fatalf("len(node.Status.VolumesAttached) Expected: <1> Actual: <%v>", len(node.Status.VolumesAttached)) + } + if node.Status.VolumesAttached[0].Name != "volume-name-1" { + t.Fatalf("volumeName Expected: Actual: <%s>", node.Status.VolumesAttached[0].Name) + } + + node, err = fakeKubeClient.CoreV1().Nodes().Get(ctx, "testnode-2", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Nodes().Get failed. Expected: Actual: <%v>", err) + } + if len(node.Status.VolumesAttached) != 1 { + t.Fatalf("len(node.Status.VolumesAttached) Expected: <1> Actual: <%v>", len(node.Status.VolumesAttached)) + } + if node.Status.VolumesAttached[0].Name != "volume-name-2" { + t.Fatalf("volumeName Expected: Actual: <%s>", node.Status.VolumesAttached[0].Name) + } +} + +func TestNodeStatusUpdater_UpdateNodeStatuses_FailureInFirstUpdate(t *testing.T) { + ctx := context.Background() + asw, fakeKubeClient, nsu := setupNodeStatusUpdate(ctx, t) + + var failedNode string + failedOnce := false + failureErr := fmt.Errorf("test generated error") + fakeKubeClient.PrependReactor("patch", "nodes", func(action core.Action) (handled bool, ret runtime.Object, err error) { + patchAction := action.(core.PatchAction) + if !failedOnce { + failedNode = patchAction.GetName() + failedOnce = true + return true, nil, failureErr + } + return false, nil, nil + }) + + err := nsu.UpdateNodeStatuses() + if errors.Is(err, failureErr) { + t.Fatalf("UpdateNodeStatuses failed. Expected: Actual: <%v>", err) + } + + needToReport := asw.GetVolumesToReportAttached() + if len(needToReport) != 1 { + t.Fatalf("len(asw.GetVolumesToReportAttached()) Expected: <1> Actual: <%v>", len(needToReport)) + } + if _, ok := needToReport[types.NodeName(failedNode)]; !ok { + t.Fatalf("GetVolumesToReportAttached() did not report correct node Expected: <%s> Actual: <%v>", failedNode, needToReport) + } + + nodes, err := fakeKubeClient.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) + if err != nil { + t.Fatalf("Nodes().List failed. Expected: Actual: <%v>", err) + } + + if len(nodes.Items) != 2 { + t.Fatalf("len(nodes.Items) Expected: <2> Actual: <%v>", len(nodes.Items)) + } + + for _, node := range nodes.Items { + if node.Name == failedNode { + if len(node.Status.VolumesAttached) != 0 { + t.Fatalf("len(node.Status.VolumesAttached) Expected: <0> Actual: <%v>", len(node.Status.VolumesAttached)) + } + } else { + if len(node.Status.VolumesAttached) != 1 { + t.Fatalf("len(node.Status.VolumesAttached) Expected: <1> Actual: <%v>", len(node.Status.VolumesAttached)) + } + } + } +} + +// TestNodeStatusUpdater_UpdateNodeStatusForNode calls setup +// calls UpdateNodeStatusesForNode on testnode-1 +// check that asw.GetVolumesToReportAttached reports testnode-2 needs to be reported +// checks that testnode-1 status.volumesAttached is of length 1 and contains the correct volume +func TestNodeStatusUpdater_UpdateNodeStatusForNode(t *testing.T) { + ctx := context.Background() + asw, fakeKubeClient, nsu := setupNodeStatusUpdate(ctx, t) + + err := nsu.UpdateNodeStatusForNode("testnode-1") + if err != nil { + t.Fatalf("UpdateNodeStatuses failed. Expected: Actual: <%v>", err) + } + + needToReport := asw.GetVolumesToReportAttached() + if len(needToReport) != 1 { + t.Fatalf("len(asw.GetVolumesToReportAttached()) Expected: <1> Actual: <%v>", len(needToReport)) + } + if _, ok := needToReport["testnode-2"]; !ok { + t.Fatalf("GetVolumesToReportAttached() did not report correct node Expected: Actual: <%v>", needToReport) + } + + node, err := fakeKubeClient.CoreV1().Nodes().Get(ctx, "testnode-1", metav1.GetOptions{}) + if err != nil { + t.Fatalf("Nodes().Get failed. Expected: Actual: <%v>", err) + } + if len(node.Status.VolumesAttached) != 1 { + t.Fatalf("len(node.Status.VolumesAttached) Expected: <1> Actual: <%v>", len(node.Status.VolumesAttached)) + } + if node.Status.VolumesAttached[0].Name != "volume-name-1" { + t.Fatalf("volumeName Expected: Actual: <%s>", node.Status.VolumesAttached[0].Name) + } +}