Attached Volume data structure issue in volume manager's actual state

[jingxu97]

Two or more pods are allowed to use the same volume. If pods are using in-line volume spec (directly put the volume spec into the pod spec), the volume specs in different pods' spec could have different names. With current data structure in volume manager, we might hit conflicts as explained below.

Give two pod spec example

	   apiVersion: v1
	   kind: Pod
	   metadata:
	     name: test-pd1
	  spec:
	     containers:
	     - image: k8s.gcr.io/test-webserver
	     	 name: test-container
	     	 volumeMounts:
	     	 - mountPath: /test-pd
	     	   name: test-volume
	     volumes:
	     - name: test-volume-0			<- InnerVolumeSpecName
	     	 gcePersistentDisk:
	     	   pdName: my-data-disk
	     	   fsType: ext4

	   apiVersion: v1
	   kind: Pod
	   metadata:
	     name: test-pd2
	  spec:
	     containers:
	     - image: k8s.gcr.io/test-webserver
	     	 name: test-container
	     	 volumeMounts:
	     	 - mountPath: /test-pd
	     	   name: test-volume
	     volumes:
	     - name: test-volume-1			<- InnerVolumeSpecName
	     	 gcePersistentDisk:
	     	   pdName: my-data-disk
	     	   fsType: ext4

Suppose test-p1 is created first. The pod volume dir would be /var/lib/kubelet/pods/{pod-uid}/volumes/kubernetes.io~gce-pd/test-volume-0. This volume spec will be added into actual state https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumemanager/cache/actual_state_of_world.go#L376

When test-p2 is created, the pod volume dir would be /var/lib/kubelet/pods/{pod-uid}/volumes/kubernetes.io~gce-pd/test-volume-1. But when it tries to add to the actual state, since the volume was added to the state for pod test-p1, the volume spec will not be updated.
The problem caused by this is when pod test-p2 is deleted, it will construct the wrong volume path which is the path for the first pod volume and fail to finish the tear down process correctly.

[jingxu97]

cc @saad-ali

[tpepper]

Did #61071 resolve this issue? Should it be closed?

[jingxu97]

@tpepper No, we found this bug when working on #61071. It is not resolved yet.

[jberkus]

@jingxu97 is this issue specifically in 1.10? Or has it existed longer than that?

[jingxu97]

@jberkus it exists for a long time

[jingxu97]

The possible solution is in actual_state_of_world.go, move the volume spec information into the mountedPods data structure.

[saad-ali]

Moving attachedVolume.spec in kubelet/volumemanager/cache/actual_state_of_world.go#L229 to mountedPod makes sense.

[mlmhl]

There is a problem if we are just moving attachedVolume.spec from attachedVolume to mountedPods:
Currently we use attachedVolume.spec to generate operationexecutor.AttachedVolume(inside the newAttachedVolume method), which will be used to umount according volume device(inside the reconciler).
Inside the UmountDevice function, the GenerateErrorDetailed method(or something else like this) of operationexecutor.AttachedVolume is invoked to generate successful or failed messages, and these methods invokes AttachedVolume.VolumeSpec.Name() to get volumeSpecName, so if we removed attachedVolume.spec, according AttachedVolume.VolumeSpec will be empty, and the volumeSpecName is printed as nil.

I think we have three alternative approaches to handle this problem:

1. Just moving attachedVolume.spec from attachedVolume to mountedPods, and let the volumeSpecName is printed as nil.

2. Instead of moving attachedVolume.spec from attachedVolume to mountedPods, we keep this field both in attachedVolume and mountedPods, mountedPods.spec is used to umount(tear down) volumes and attachedVolume.spec is used to umount devices. Strictly speaking, this is not the right way, because if more than one pods use a same volume as inline volume spec, the umount device operation will just print out the last volumeSpecName.

3. A better way is that we print all volumeSpecNames inside umount device operation, but it's hard to achieve it in the current way of implementation.

@jingxu97 @saad-ali Any thoughts?

[saad-ali]

AttachedVolume.VolumeSpec should be removed. VolumeSpec is unique per pod not per volume, and since more than 1 pod can reference the same volume there maybe more then one VolumeSpec. The AttachedVolume should only be used by volume operations (like AttachVolume, MountDevice, UnmountDevice, DetachVolume) not for pod operations (like Setup and Teardown) -- so it errors on these operations should print out volume id information not volumeSpec specific info.

[jingxu97]

@mlmhl @saad-ali,
as @mlmhl mentioned, currently GenerateErrorDetailed method will print volumeSpec.Name as volume information. I am wondering why we didn't choose to use volume UniqueVolumeName before?

[mlmhl]

@jingxu97 @saad-ali
What about add a GenerateErrorDetailedForDevice(or something else like this) method which only print UniqueVolumeName, then we can use GenerateErrorDetailed for pod operations(SetUp/TearDown) and use GenerateErrorDetailedForDevice for device operations(MountDevice/UmountDevice)？

[jingxu97]

@mlmhl you are thinking the spec name is also very useful so we should keep them for mount operation's log?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[redbaron]

There is #61549 which claims this issue was fixed

[redbaron]

/remove-lifecycle rotten

[jingxu97]

#61549 fixed the issue. Close it