fix GenerateUnmapVolumeFunc missing globalUnmapPath

[carlory]

What type of PR is this?

/kind bug

What this PR does / why we need it:

GenerateUnmapVolumeFunc missing globalUnmapPath when kubelet tries to clean up all volumes that failed reconstruction.

func (rc *reconciler) cleanupMounts(volume podVolume) {
	klog.V(2).InfoS("Reconciler sync states: could not find volume information in desired state, clean up the mount points", "podName", volume.podName, "volumeSpecName", volume.volumeSpecName)
	mountedVolume := operationexecutor.MountedVolume{
		PodName: volume.podName,
		// VolumeName should be generated by `GetUniqueVolumeNameFromSpec` or `GetUniqueVolumeNameFromSpecWithPod`.
		// However, since we don't have the volume information in asw when cleanup mounts, it doesn't matter what we put here.
		VolumeName:          v1.UniqueVolumeName(volume.volumeSpecName),
		InnerVolumeSpecName: volume.volumeSpecName,
		PluginName:          volume.pluginName,
		PodUID:              types.UID(volume.podName),
	}
	metrics.ForceCleanedFailedVolumeOperationsTotal.Inc()
	// TODO: Currently cleanupMounts only includes UnmountVolume operation. In the next PR, we will add
	// to unmount both volume and device in the same routine.
	err := rc.operationExecutor.UnmountVolume(mountedVolume, rc.actualStateOfWorld, rc.kubeletPodsDir)
...
}

func (og *operationGenerator) GenerateUnmapVolumeFunc(
	volumeToUnmount MountedVolume,
	actualStateOfWorld ActualStateOfWorldMounterUpdater) (volumetypes.GeneratedOperations, error) {
...
	unmapVolumeFunc := func() volumetypes.OperationContext {

		migrated := getMigratedStatusBySpec(volumeToUnmount.VolumeSpec)

		// pods/{podUid}/volumeDevices/{escapeQualifiedPluginName}/{volumeName}
		podDeviceUnmapPath, volName := blockVolumeUnmapper.GetPodDeviceMapPath()
		// plugins/kubernetes.io/{PluginName}/volumeDevices/{volumePluginDependentPath}/{podUID}
		globalUnmapPath := volumeToUnmount.DeviceMountPath
...
}

Which issue(s) this PR fixes:

Ref #122960

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix Pod stuck in Terminating because of GenerateUnmapVolumeFunc missing globalUnmapPath when kubelet tries to clean up all volumes that failed reconstruction.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[carlory]

/cc @jsafrane

[carlory]

cc @gdiwate @kannon92

[carlory]

/assign @gnufied

[jsafrane]

Is there an unit tests that could check globalUnmapPath is filed correctly?

[carlory]

Only csi plugin and local volume plugin have unit test for GetGlobalMapPath

• kubernetes/pkg/volume/csi/csi_block_test.go

  Lines 54 to 55 in 46c9bd1

  	func TestBlockMapperGetGlobalMapPath(t *testing.T) {
  	plug, tmpDir := newTestPlugin(t, nil)

• kubernetes/pkg/volume/local/local_test.go

  Lines 426 to 427 in 46c9bd1

  	expectedGlobalPath := filepath.Join(tmpDir, testGlobalPath)
  	globalPath, err := mapper.GetGlobalMapPath(volSpec)

[carlory]

After I read the operation_generator_test.go, I cannot find an existing unit test for the GenerateUnmapVolumeFunc. And the other funcs are also lack of unit tests.

[gnufied]

Isn't this very similar to #122211 ? Which fixes the issue by ensuring that we don't lose global unmap path during unmap failures?

[carlory]

@gnufied Thanks for your fix. I believe this issue should be resolved by your PR. But this PR maybe still needed for a potential issue. i.e. if the volume is failed to be reconstructed, then the volume will be cleaned up. In this case, the global unmap path field is not filled in. You can see the code in the PR's description.

[jsafrane]

I think this is still useful. #122211 fixes a bug when a Pod is still available (and will end up Terminating), while in this case the Pod may not be available at all.

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 1cc5eac7c79b8ed06e054f3ca3c4a479cae0a4b1

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlory, jsafrane

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/volume/OWNERS [jsafrane]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment