From 0a2272dc68d58d4f750d4eebea5ec6ad0cc147b9 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Fri, 5 May 2023 12:40:53 +0200 Subject: [PATCH] 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. --- .../cache/actual_state_of_world.go | 65 ++++++-- .../cache/actual_state_of_world_test.go | 77 +++++++++- .../reconciler/reconciler_new_test.go | 142 ++++++++++++++++++ .../reconciler/reconstruct_new.go | 8 +- 4 files changed, 277 insertions(+), 15 deletions(-) create mode 100644 pkg/kubelet/volumemanager/reconciler/reconciler_new_test.go diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world.go index 90217a102d7..ada8c4415e4 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world.go +++ b/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, diff --git a/pkg/kubelet/volumemanager/cache/actual_state_of_world_test.go b/pkg/kubelet/volumemanager/cache/actual_state_of_world_test.go index 2fe292e7c98..a0ed93f00a1 100644 --- a/pkg/kubelet/volumemanager/cache/actual_state_of_world_test.go +++ b/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: 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 Actual ") } } + +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) + } + } +} diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler_new_test.go b/pkg/kubelet/volumemanager/reconciler/reconciler_new_test.go new file mode 100644 index 00000000000..e4b22cbe7e8 --- /dev/null +++ b/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") + } + } +} diff --git a/pkg/kubelet/volumemanager/reconciler/reconstruct_new.go b/pkg/kubelet/volumemanager/reconciler/reconstruct_new.go index 51d52c0b1be..ba220bd003f 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconstruct_new.go +++ b/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 }