Repository: kubernetes/kubernetes (language: go)

Issue:
Should we remove `volumesNeedReportedInUse` for reconstructed volumes?
### What would you like to be added/removed?

remove [`volumesNeedReportedInUse`](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/reconciler/reconciler_common.go#L148)for reconstructed volumes and [corresponding reconciler code](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/reconciler/reconciler_new.go#L69-L74) and [reconstruction code](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/reconciler/reconstruct_new.go#L98)

### Why is this needed?

`volumesNeedReportedInUse` was introduced in https://github.com/kubernetes/kubernetes/pull/60888 to fix https://github.com/kubernetes/kubernetes/issues/60645. 

This is to overcome the situation that volume is added to DSW at first and then deleted before it is added to ASW, which causes mounted volume neither in DSW nor ASW and won't execute unmount/detach: https://github.com/kubernetes/kubernetes/issues/60645#issuecomment-370757877 

The reason the volume cannot be added to ASW in time is [because it is not marked as `reported in use` in DSW](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/reconciler/reconciler_common.go#L242) , which is [updated in an async goroutine](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_node_status.go#L579).

However, there is another PR https://github.com/kubernetes/kubernetes/pull/61071 also solved the issue. What it did is to check 
whether ASW already has the volume before deleting it from DSW, this makes sure that volume can be detached/unmounted after it is deleted from DSW.

I suggest that we can keep the second solution and remove `volumesNeedReportedInUse` related code to make the reconstruction process and reconcile loop cleaner?


Discussion / hints:
/sig storage
cc @jsafrane @jingxu97 @gnufied 
@cvvz Should I proceed with this issue? Your guidance is appreciated.
@mukeshmahato18 I think we'd better wait for the feedback from @kubernetes/sig-storage-feature-requests 
cc @jsafrane @jingxu97 
Sorry for a late reply. This is how behind we are with reviews :-(.

Reading https://github.com/kubernetes/kubernetes/pull/60888. Now that `NewVolumeManagerReconstruction` is on by default, it reconstructs volumes first and *always* puts them to ASW, it should never happen that a reconstructed volume is in DSW and not in ASW, right? So we can remove `volumesNeedReportedInUse` easily.

I would not be so sure without `NewVolumeManagerReconstruction`, because the code is 5 years old and who knows all the implications. You could be right though, it may not be necessary there too.
/triage accepted
/assign

Fix patch:
diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler.go b/pkg/kubelet/volumemanager/reconciler/reconciler.go
index 07b5f573cbfb2..e675d2b953add 100644
--- a/pkg/kubelet/volumemanager/reconciler/reconciler.go
+++ b/pkg/kubelet/volumemanager/reconciler/reconciler.go
@@ -62,11 +62,4 @@ func (rc *reconciler) reconcile() {
 		// This will start reconciliation of node.status.volumesInUse.
 		rc.updateLastSyncTime()
 	}
-
-	if len(rc.volumesNeedReportedInUse) != 0 && rc.populatorHasAddedPods() {
-		// Once DSW is populated, mark all reconstructed as reported in node.status,
-		// so they can proceed with MountDevice / SetUp.
-		rc.desiredStateOfWorld.MarkVolumesReportedInUse(rc.volumesNeedReportedInUse)
-		rc.volumesNeedReportedInUse = nil
-	}
 }
diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_common.go b/pkg/kubelet/volumemanager/reconciler/reconciler_common.go
index 4129247658b3d..29d57abf4bd94 100644
--- a/pkg/kubelet/volumemanager/reconciler/reconciler_common.go
+++ b/pkg/kubelet/volumemanager/reconciler/reconciler_common.go
@@ -119,7 +119,6 @@ func NewReconciler(
 		timeOfLastSync:                  time.Time{},
 		volumesFailedReconstruction:     make([]podVolume, 0),
 		volumesNeedUpdateFromNodeStatus: make([]v1.UniqueVolumeName, 0),
-		volumesNeedReportedInUse:        make([]v1.UniqueVolumeName, 0),
 	}
 }
 
@@ -143,7 +142,6 @@ type reconciler struct {
 	timeOfLastSync                  time.Time
 	volumesFailedReconstruction     []podVolume
 	volumesNeedUpdateFromNodeStatus []v1.UniqueVolumeName
-	volumesNeedReportedInUse        []v1.UniqueVolumeName
 }
 
 func (rc *reconciler) unmountVolumes() {
diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct.go b/pkg/kubelet/volumemanager/reconciler/reconstruct.go
index e95796d789579..c67686882911d 100644
--- a/pkg/kubelet/volumemanager/reconciler/reconstruct.go
+++ b/pkg/kubelet/volumemanager/reconciler/reconstruct.go
@@ -91,9 +91,6 @@ func (rc *reconciler) reconstructVolumes() {
 		// Add the volumes to ASW
 		rc.updateStates(reconstructedVolumes)
 
-		// The reconstructed volumes are mounted, hence a previous kubelet must have already put it into node.status.volumesInUse.
-		// Remember to update DSW with this information.
-		rc.volumesNeedReportedInUse = reconstructedVolumeNames
 		// Remember to update devicePath from node.status.volumesAttached
 		rc.volumesNeedUpdateFromNodeStatus = reconstructedVolumeNames
 	}


Test patch:
diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct_test.go b/pkg/kubelet/volumemanager/reconciler/reconstruct_test.go
index 49e2df96fa297..c5715d2ba2b7f 100644
--- a/pkg/kubelet/volumemanager/reconciler/reconstruct_test.go
+++ b/pkg/kubelet/volumemanager/reconciler/reconstruct_test.go
@@ -36,7 +36,6 @@ func TestReconstructVolumes(t *testing.T) {
 	tests := []struct {
 		name                                string
 		volumePaths                         []string
-		expectedVolumesNeedReportedInUse    []string
 		expectedVolumesNeedDevicePath       []string
 		expectedVolumesFailedReconstruction []string
 		verifyFunc                          func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error
@@ -47,7 +46,6 @@ func TestReconstructVolumes(t *testing.T) {
 				filepath.Join("pod1", "volumes", "fake-plugin", "pvc-abcdef"),
 				filepath.Join("pod2", "volumes", "fake-plugin", "pvc-abcdef"),
 			},
-			expectedVolumesNeedReportedInUse:    []string{"fake-plugin/pvc-abcdef", "fake-plugin/pvc-abcdef"},
 			expectedVolumesNeedDevicePath:       []string{"fake-plugin/pvc-abcdef", "fake-plugin/pvc-abcdef"},
 			expectedVolumesFailedReconstruction: []string{},
 			verifyFunc: func(rcInstance *reconciler, fakePlugin *volumetesting.FakeVolumePlugin) error {
@@ -75,7 +73,6 @@ func TestReconstructVolumes(t *testing.T) {
 			volumePaths: []string{
 				filepath.Join("pod1", "volumes", "missing-plugin", "pvc-abcdef"),
 			},
-			expectedVolumesNeedReportedInUse:    []string{},
 			expectedVolumesNeedDevicePath:       []string{},
 			expectedVolumesFailedReconstruction: []string{"pvc-abcdef"},
 		},
@@ -117,14 +114,6 @@ func TestReconstructVolumes(t *testing.T) {
 				t.Errorf("Expected expectedVolumesNeedDevicePath:\n%v\n got:\n%v", expectedVolumes, rcInstance.volumesNeedUpdateFromNodeStatus)
 			}
 
-			expectedVolumes = make([]v1.UniqueVolumeName, len(tc.expectedVolumesNeedReportedInUse))
-			for i := range tc.expectedVolumesNeedReportedInUse {
-				expectedVolumes[i] = v1.UniqueVolumeName(tc.expectedVolumesNeedReportedInUse[i])
-			}
-			if !reflect.DeepEqual(expectedVolumes, rcInstance.volumesNeedReportedInUse) {
-				t.Errorf("Expected volumesNeedReportedInUse:\n%v\n got:\n%v", expectedVolumes, rcInstance.volumesNeedReportedInUse)
-			}
-
 			volumesFailedReconstruction := sets.NewString()
 			for _, vol := range rcInstance.volumesFailedReconstruction {
 				volumesFailedReconstruction.Insert(vol.volumeSpecName)