Add uncertain state of volume attach-ability

During CSI volume reconstruction it's not possible to tell, if the volume
is attachable or not - CSIDriver instance may not be available, because
kubelet may not have connection to the API server at that time.

Adding uncertain state during reconstruction + adding a correct state when
the API server is available.

pkg/kubelet/volumemanager/cache/actual_state_of_world.go

@@ -169,14 +169,20 @@ type ActualStateOfWorld interface {
 	GetAttachedVolumes() []AttachedVolume
 
 	// SyncReconstructedVolume check the volume.outerVolumeSpecName in asw and
-	// the one populated from dsw , if they do not match, update this field from the value from dsw.
+	// the one populated from dsw, if they do not match, update this field from the value from dsw.
 	SyncReconstructedVolume(volumeName v1.UniqueVolumeName, podName volumetypes.UniquePodName, outerVolumeSpecName string)
 
+	// Add the specified volume to ASW as uncertainly attached.
+	AddAttachUncertainReconstructedVolume(volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, nodeName types.NodeName, devicePath string) error
+
 	// UpdateReconstructedDevicePath updates devicePath of a reconstructed volume
 	// from Node.Status.VolumesAttached. The ASW is updated only when the volume is still
 	// uncertain. If the volume got mounted in the meantime, its devicePath must have
 	// been fixed by such an update.
 	UpdateReconstructedDevicePath(volumeName v1.UniqueVolumeName, devicePath string)
+
+	// UpdateReconstructedVolumeAttachability updates volume attachability from the API server.
+	UpdateReconstructedVolumeAttachability(volumeName v1.UniqueVolumeName, volumeAttachable bool)
 }
 
 // MountedVolume represents a volume that has successfully been mounted to a pod.
@@ -251,6 +257,14 @@ type actualStateOfWorld struct {
 	sync.RWMutex
 }
 
+type volumeAttachability string
+
+const (
+	volumeAttachabilityTrue      volumeAttachability = "True"
+	volumeAttachabilityFalse     volumeAttachability = "False"
+	volumeAttachabilityUncertain volumeAttachability = "Uncertain"
+)
+
 // attachedVolume represents a volume the kubelet volume manager believes to be
 // successfully attached to a node it is managing. Volume types that do not
 // implement an attacher are assumed to be in this state.
@@ -280,7 +294,7 @@ type attachedVolume struct {
 
 	// pluginIsAttachable indicates the volume plugin used to attach and mount
 	// this volume implements the volume.Attacher interface
-	pluginIsAttachable bool
+	pluginIsAttachable volumeAttachability
 
 	// deviceMountState stores information that tells us if device is mounted
 	// globally or not
@@ -361,7 +375,19 @@ type mountedPod struct {
 func (asw *actualStateOfWorld) MarkVolumeAsAttached(
 	logger klog.Logger,
 	volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, _ types.NodeName, devicePath string) error {
-	return asw.addVolume(volumeName, volumeSpec, devicePath)
+
+	pluginIsAttachable := volumeAttachabilityFalse
+	if attachablePlugin, err := asw.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec); err == nil && attachablePlugin != nil {
+		pluginIsAttachable = volumeAttachabilityTrue
+	}
+
+	return asw.addVolume(volumeName, volumeSpec, devicePath, pluginIsAttachable)
+}
+
+func (asw *actualStateOfWorld) AddAttachUncertainReconstructedVolume(
+	volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, _ types.NodeName, devicePath string) error {
+
+	return asw.addVolume(volumeName, volumeSpec, devicePath, volumeAttachabilityUncertain)
 }
 
 func (asw *actualStateOfWorld) MarkVolumeAsUncertain(
@@ -526,6 +552,28 @@ func (asw *actualStateOfWorld) UpdateReconstructedDevicePath(volumeName v1.Uniqu
 	asw.attachedVolumes[volumeName] = volumeObj
 }
 
+func (asw *actualStateOfWorld) UpdateReconstructedVolumeAttachability(volumeName v1.UniqueVolumeName, attachable bool) {
+	asw.Lock()
+	defer asw.Unlock()
+
+	volumeObj, volumeExists := asw.attachedVolumes[volumeName]
+	if !volumeExists {
+		return
+	}
+	if volumeObj.pluginIsAttachable != volumeAttachabilityUncertain {
+		// Reconciler must have updated volume state, i.e. when a pod uses the volume and
+		// succeeded mounting the volume. Such update has fixed the device path.
+		return
+	}
+
+	if attachable {
+		volumeObj.pluginIsAttachable = volumeAttachabilityTrue
+	} else {
+		volumeObj.pluginIsAttachable = volumeAttachabilityFalse
+	}
+	asw.attachedVolumes[volumeName] = volumeObj
+}
+
 func (asw *actualStateOfWorld) GetDeviceMountState(volumeName v1.UniqueVolumeName) operationexecutor.DeviceMountState {
 	asw.RLock()
 	defer asw.RUnlock()
@@ -592,7 +640,7 @@ func (asw *actualStateOfWorld) IsVolumeMountedElsewhere(volumeName v1.UniqueVolu
 // volume plugin can support the given volumeSpec or more than one plugin can
 // support it, an error is returned.
 func (asw *actualStateOfWorld) addVolume(
-	volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, devicePath string) error {
+	volumeName v1.UniqueVolumeName, volumeSpec *volume.Spec, devicePath string, attachability volumeAttachability) error {
 	asw.Lock()
 	defer asw.Unlock()
 
@@ -615,11 +663,6 @@ func (asw *actualStateOfWorld) addVolume(
 		}
 	}
 
-	pluginIsAttachable := false
-	if attachablePlugin, err := asw.volumePluginMgr.FindAttachablePluginBySpec(volumeSpec); err == nil && attachablePlugin != nil {
-		pluginIsAttachable = true
-	}
-
 	volumeObj, volumeExists := asw.attachedVolumes[volumeName]
 	if !volumeExists {
 		volumeObj = attachedVolume{
@@ -627,7 +670,7 @@ func (asw *actualStateOfWorld) addVolume(
 			spec:               volumeSpec,
 			mountedPods:        make(map[volumetypes.UniquePodName]mountedPod),
 			pluginName:         volumePlugin.GetPluginName(),
-			pluginIsAttachable: pluginIsAttachable,
+			pluginIsAttachable: attachability,
 			deviceMountState:   operationexecutor.DeviceNotMounted,
 			devicePath:         devicePath,
 		}
@@ -1094,7 +1137,7 @@ func (asw *actualStateOfWorld) newAttachedVolume(
 			VolumeName:          attachedVolume.volumeName,
 			VolumeSpec:          attachedVolume.spec,
 			NodeName:            asw.nodeName,
-			PluginIsAttachable:  attachedVolume.pluginIsAttachable,
+			PluginIsAttachable:  attachedVolume.pluginIsAttachable == volumeAttachabilityTrue,
 			DevicePath:          attachedVolume.devicePath,
 			DeviceMountPath:     attachedVolume.deviceMountPath,
 			PluginName:          attachedVolume.pluginName,

pkg/kubelet/volumemanager/cache/actual_state_of_world_test.go

@@ -525,6 +525,54 @@ func TestActualStateOfWorld_FoundDuringReconstruction(t *testing.T) {
 				return nil
 			},
 		},
+		{
+			name: "uncertain attachability is resolved to attachable",
+			opCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, true)
+				return nil
+			},
+			verifyCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				verifyVolumeAttachability(t, volumeOpts.VolumeName, asw, volumeAttachabilityTrue)
+				return nil
+			},
+		},
+		{
+			name: "uncertain attachability is resolved to non-attachable",
+			opCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, false)
+				return nil
+			},
+			verifyCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				verifyVolumeAttachability(t, volumeOpts.VolumeName, asw, volumeAttachabilityFalse)
+				return nil
+			},
+		},
+		{
+			name: "certain (false) attachability cannot be changed",
+			opCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, false)
+				// This function should be NOOP:
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, true)
+				return nil
+			},
+			verifyCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				verifyVolumeAttachability(t, volumeOpts.VolumeName, asw, volumeAttachabilityFalse)
+				return nil
+			},
+		},
+		{
+			name: "certain (true) attachability cannot be changed",
+			opCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, true)
+				// This function should be NOOP:
+				asw.UpdateReconstructedVolumeAttachability(volumeOpts.VolumeName, false)
+				return nil
+			},
+			verifyCallback: func(asw ActualStateOfWorld, volumeOpts operationexecutor.MarkVolumeOpts) error {
+				verifyVolumeAttachability(t, volumeOpts.VolumeName, asw, volumeAttachabilityTrue)
+				return nil
+			},
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -537,8 +585,7 @@ func TestActualStateOfWorld_FoundDuringReconstruction(t *testing.T) {
 			generatedVolumeName1, err := util.GetUniqueVolumeNameFromSpec(
 				plugin, volumeSpec1)
 			require.NoError(t, err)
-			logger, _ := ktesting.NewTestContext(t)
-			err = asw.MarkVolumeAsAttached(logger, generatedVolumeName1, volumeSpec1, "" /* nodeName */, devicePath)
+			err = asw.AddAttachUncertainReconstructedVolume(generatedVolumeName1, volumeSpec1, "" /* nodeName */, devicePath)
 			if err != nil {
 				t.Fatalf("MarkVolumeAsAttached failed. Expected: <no error> Actual: <%v>", err)
 			}
@@ -575,6 +622,7 @@ func TestActualStateOfWorld_FoundDuringReconstruction(t *testing.T) {
 			verifyVolumeExistsWithSpecNameInVolumeAsw(t, podName1, volumeSpec1.Name(), asw)
 			verifyVolumeSpecNameInVolumeAsw(t, podName1, []*volume.Spec{volumeSpec1}, asw)
 			verifyVolumeFoundInReconstruction(t, podName1, generatedVolumeName1, asw)
+			verifyVolumeAttachability(t, generatedVolumeName1, asw, volumeAttachabilityUncertain)
 
 			if tc.opCallback != nil {
 				err = tc.opCallback(asw, markVolumeOpts1)
@@ -1307,3 +1355,28 @@ func verifyVolumeFoundInReconstruction(t *testing.T, podToCheck volumetypes.Uniq
 		t.Fatalf("ASW IsVolumeReconstructed result invalid. expected <true> Actual <false>")
 	}
 }
+
+func verifyVolumeAttachability(t *testing.T, volumeToCheck v1.UniqueVolumeName, asw ActualStateOfWorld, expected volumeAttachability) {
+	attached := asw.GetAttachedVolumes()
+	attachable := false
+
+	for _, volume := range attached {
+		if volume.VolumeName == volumeToCheck {
+			attachable = volume.PluginIsAttachable
+			break
+		}
+	}
+
+	switch expected {
+	case volumeAttachabilityTrue:
+		if !attachable {
+			t.Errorf("ASW reports %s as not-attachable, when %s was expected", volumeToCheck, expected)
+		}
+	// ASW does not have any special difference between False and Uncertain.
+	// Uncertain only allows to be changed to True / False.
+	case volumeAttachabilityUncertain, volumeAttachabilityFalse:
+		if attachable {
+			t.Errorf("ASW reports %s as attachable, when %s was expected", volumeToCheck, expected)
+		}
+	}
+}

pkg/kubelet/volumemanager/reconciler/reconciler_new_test.go

@@ -0,0 +1,142 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package reconciler
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/record"
+	"k8s.io/kubernetes/pkg/kubelet/volumemanager/cache"
+	"k8s.io/kubernetes/pkg/volume"
+	volumetesting "k8s.io/kubernetes/pkg/volume/testing"
+	"k8s.io/kubernetes/pkg/volume/util"
+	"k8s.io/kubernetes/pkg/volume/util/hostutil"
+	"k8s.io/kubernetes/pkg/volume/util/operationexecutor"
+	"k8s.io/mount-utils"
+)
+
+func TestReconcileWithUpdateReconstructedFromAPIServer(t *testing.T) {
+	// Calls Run() with two reconstructed volumes.
+	// Verifies the devicePaths + volume attachability are reconstructed from node.status.
+
+	// Arrange
+	node := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: string(nodeName),
+		},
+		Status: v1.NodeStatus{
+			VolumesAttached: []v1.AttachedVolume{
+				{
+					Name:       "fake-plugin/fake-device1",
+					DevicePath: "fake/path",
+				},
+			},
+		},
+	}
+	volumePluginMgr, fakePlugin := volumetesting.GetTestKubeletVolumePluginMgrWithNode(t, node)
+	seLinuxTranslator := util.NewFakeSELinuxLabelTranslator()
+	dsw := cache.NewDesiredStateOfWorld(volumePluginMgr, seLinuxTranslator)
+	asw := cache.NewActualStateOfWorld(nodeName, volumePluginMgr)
+	kubeClient := createTestClient()
+	fakeRecorder := &record.FakeRecorder{}
+	fakeHandler := volumetesting.NewBlockVolumePathHandler()
+	oex := operationexecutor.NewOperationExecutor(operationexecutor.NewOperationGenerator(
+		kubeClient,
+		volumePluginMgr,
+		fakeRecorder,
+		fakeHandler))
+	rc := NewReconciler(
+		kubeClient,
+		true, /* controllerAttachDetachEnabled */
+		reconcilerLoopSleepDuration,
+		waitForAttachTimeout,
+		nodeName,
+		dsw,
+		asw,
+		hasAddedPods,
+		oex,
+		mount.NewFakeMounter(nil),
+		hostutil.NewFakeHostUtil(nil),
+		volumePluginMgr,
+		kubeletPodsDir)
+	reconciler := rc.(*reconciler)
+
+	// The pod has two volumes, fake-device1 is attachable, fake-device2 is not.
+	pod := &v1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "pod1",
+			UID:  "pod1uid",
+		},
+		Spec: v1.PodSpec{
+			Volumes: []v1.Volume{
+				{
+					Name: "volume-name",
+					VolumeSource: v1.VolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+							PDName: "fake-device1",
+						},
+					},
+				},
+				{
+					Name: "volume-name2",
+					VolumeSource: v1.VolumeSource{
+						GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{
+							PDName: "fake-device2",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	volumeSpec1 := &volume.Spec{Volume: &pod.Spec.Volumes[0]}
+	volumeName1 := util.GetUniqueVolumeName(fakePlugin.GetPluginName(), "fake-device1")
+	volumeSpec2 := &volume.Spec{Volume: &pod.Spec.Volumes[1]}
+	volumeName2 := util.GetUniqueVolumeName(fakePlugin.GetPluginName(), "fake-device2")
+
+	assert.NoError(t, asw.AddAttachUncertainReconstructedVolume(volumeName1, volumeSpec1, nodeName, ""))
+	assert.NoError(t, asw.MarkDeviceAsUncertain(volumeName1, "/dev/badly/reconstructed", "/var/lib/kubelet/plugins/global1", ""))
+	assert.NoError(t, asw.AddAttachUncertainReconstructedVolume(volumeName2, volumeSpec2, nodeName, ""))
+	assert.NoError(t, asw.MarkDeviceAsUncertain(volumeName2, "/dev/reconstructed", "/var/lib/kubelet/plugins/global2", ""))
+
+	reconciler.volumesNeedDevicePath = append(reconciler.volumesNeedDevicePath, volumeName1, volumeName2)
+
+	// Act - run reconcile loop just once.
+	// "volumesNeedDevicePath" is not empty, so no unmount will be triggered.
+	reconciler.reconcileNew()
+
+	// Assert
+	assert.Empty(t, reconciler.volumesNeedDevicePath)
+
+	attachedVolumes := asw.GetAttachedVolumes()
+	assert.Equalf(t, len(attachedVolumes), 2, "two volumes in ASW expected")
+	for _, vol := range attachedVolumes {
+		if vol.VolumeName == volumeName1 {
+			// devicePath + attachability must have been updated from node.status
+			assert.True(t, vol.PluginIsAttachable)
+			assert.Equal(t, vol.DevicePath, "fake/path")
+		}
+		if vol.VolumeName == volumeName2 {
+			// only attachability was updated from node.status
+			assert.False(t, vol.PluginIsAttachable)
+			assert.Equal(t, vol.DevicePath, "/dev/reconstructed")
+		}
+	}
+}

pkg/kubelet/volumemanager/reconciler/reconstruct_new.go

@@ -105,9 +105,9 @@ func (rc *reconciler) reconstructVolumes() {
 
 func (rc *reconciler) updateStatesNew(reconstructedVolumes map[v1.UniqueVolumeName]*globalVolumeInfo) {
 	for _, gvl := range reconstructedVolumes {
-		err := rc.actualStateOfWorld.MarkVolumeAsAttached(
+		err := rc.actualStateOfWorld.AddAttachUncertainReconstructedVolume(
 			//TODO: the devicePath might not be correct for some volume plugins: see issue #54108
-			klog.TODO(), gvl.volumeName, gvl.volumeSpec, rc.nodeName, gvl.devicePath)
+			gvl.volumeName, gvl.volumeSpec, rc.nodeName, gvl.devicePath)
 		if err != nil {
 			klog.ErrorS(err, "Could not add volume information to actual state of world", "volumeName", gvl.volumeName)
 			continue
@@ -196,14 +196,18 @@ func (rc *reconciler) updateReconstructedDevicePaths() {
 	}
 
 	for _, volumeID := range rc.volumesNeedDevicePath {
+		attachable := false
 		for _, attachedVolume := range node.Status.VolumesAttached {
 			if volumeID != attachedVolume.Name {
 				continue
 			}
 			rc.actualStateOfWorld.UpdateReconstructedDevicePath(volumeID, attachedVolume.DevicePath)
+			attachable = true
 			klog.V(4).InfoS("Updated devicePath from node status for volume", "volumeName", attachedVolume.Name, "path", attachedVolume.DevicePath)
 		}
+		rc.actualStateOfWorld.UpdateReconstructedVolumeAttachability(volumeID, attachable)
 	}
+
 	klog.V(2).InfoS("DevicePaths of reconstructed volumes updated")
 	rc.volumesNeedDevicePath = nil
 }