Merge pull request #139045 from aramase/aramase/i/fix_121271

fix(csi): preserve mount dir when NodePublish fails on a remount

pkg/volume/csi/csi_mounter.go

@@ -315,7 +315,11 @@ func (c *csiMountMgr) SetUpAt(dir string, mounterArgs volume.MounterArgs) error
 
 	if csiRPCError != nil {
 		// If operation finished with error then we can remove the mount directory.
-		if volumetypes.IsOperationFinishedError(csiRPCError) && !mounterArgs.ReconstructedVolume {
+		// Skip on remount (e.g. CSIDriver.spec.requiresRepublish=true): the volume
+		// was already published and the pod is observing the existing bind mount,
+		// so removing the mount dir here would leave the pod with stale contents
+		// that a subsequent successful republish cannot repair (#121271).
+		if volumetypes.IsOperationFinishedError(csiRPCError) && !mounterArgs.ReconstructedVolume && !mounterArgs.IsRemount {
 			if removeMountDirErr := removeMountDir(c.plugin, dir); removeMountDirErr != nil {
 				klog.Error(log("mounter.SetupAt failed to remove mount dir after a NodePublish() error [%s]: %v", dir, removeMountDirErr))
 			}

pkg/volume/csi/csi_mounter_test.go

@@ -1177,11 +1177,13 @@ func TestMounterSetUpWithFSGroup(t *testing.T) {
 
 func TestMounterSetUpFWithNodePublishFinalError(t *testing.T) {
 	testCases := []struct {
-		name                string
-		podUID              types.UID
-		options             []string
-		spec                func(string, []string) *volume.Spec
-		reconstructedVolume bool
+		name                 string
+		podUID               types.UID
+		options              []string
+		spec                 func(string, []string) *volume.Spec
+		reconstructedVolume  bool
+		isRemount            bool
+		expectDataFileExists bool
 	}{
 		{
 			name:   "setup with reconstructed volume",
@@ -1192,7 +1194,8 @@ func TestMounterSetUpFWithNodePublishFinalError(t *testing.T) {
 				pvSrc.Spec.MountOptions = options
 				return volume.NewSpecFromPersistentVolume(pvSrc, false)
 			},
-			reconstructedVolume: true,
+			reconstructedVolume:  true,
+			expectDataFileExists: true,
 		},
 		{
 			name:   "setup with new volume",
@@ -1203,7 +1206,27 @@ func TestMounterSetUpFWithNodePublishFinalError(t *testing.T) {
 				pvSrc.Spec.MountOptions = options
 				return volume.NewSpecFromPersistentVolume(pvSrc, false)
 			},
-			reconstructedVolume: false,
+			reconstructedVolume:  false,
+			expectDataFileExists: false,
+		},
+		{
+			// Regression test for https://github.com/kubernetes/kubernetes/issues/121271:
+			// when NodePublishVolume fails on a remount (e.g. a republish
+			// triggered by CSIDriver.spec.requiresRepublish=true), the
+			// mount directory and vol_data.json must be preserved so the
+			// pod continues to see the previously-published contents and
+			// a subsequent successful republish can refresh them in place.
+			name:   "setup with remount preserves mount dir on final error",
+			podUID: types.UID(fmt.Sprintf("%08X", rand.Uint64())),
+			spec: func(fsType string, options []string) *volume.Spec {
+				pvSrc := makeTestPV("pv1", 20, testDriver, "vol1")
+				pvSrc.Spec.CSI.FSType = fsType
+				pvSrc.Spec.MountOptions = options
+				return volume.NewSpecFromPersistentVolume(pvSrc, false)
+			},
+			reconstructedVolume:  false,
+			isRemount:            true,
+			expectDataFileExists: true,
 		},
 	}
 
@@ -1240,21 +1263,20 @@ func TestMounterSetUpFWithNodePublishFinalError(t *testing.T) {
 			csiMounter.csiClient.(*fakeCsiDriverClient).nodeClient.SetNextError(status.Errorf(codes.InvalidArgument, "mount failed"))
 
 			// Mounter.SetUp()
-			if err := csiMounter.SetUp(volume.MounterArgs{ReconstructedVolume: tc.reconstructedVolume}); err == nil {
+			if err := csiMounter.SetUp(volume.MounterArgs{
+				ReconstructedVolume: tc.reconstructedVolume,
+				IsRemount:           tc.isRemount,
+			}); err == nil {
 				t.Fatalf("mounter.Setup expected err but succeed")
 			}
 
 			mountPath := csiMounter.GetPath()
 			volPath := filepath.Dir(mountPath)
 			dataFile := filepath.Join(volPath, volDataFileName)
-			if tc.reconstructedVolume {
-				if _, err := os.Stat(dataFile); os.IsNotExist(err) {
-					t.Errorf("volume file [%s] expects to be exists, but removed", dataFile)
-				}
-				return
-			}
-			if _, err := os.Stat(dataFile); err == nil {
-				t.Errorf("volume file [%s] expects to be removed, but exists", dataFile)
+			_, statErr := os.Stat(dataFile)
+			exists := statErr == nil
+			if exists != tc.expectDataFileExists {
+				t.Errorf("volume file [%s]: exists=%v, want=%v (statErr=%v)", dataFile, exists, tc.expectDataFileExists, statErr)
 			}
 		})
 	}

pkg/volume/util/operationexecutor/operation_generator.go

@@ -588,6 +588,7 @@ func (og *operationGenerator) GenerateMountVolumeFunc(
 			Recorder:            og.recorder,
 			SELinuxLabel:        volumeToMount.SELinuxLabel,
 			ReconstructedVolume: actualStateOfWorld.IsVolumeReconstructed(volumeToMount.VolumeName, volumeToMount.PodName),
+			IsRemount:           isRemount,
 		})
 		// Update actual state of world
 		markOpts := MarkVolumeOpts{

pkg/volume/volume.go

@@ -139,6 +139,16 @@ type MounterArgs struct {
 	// mainly used by unit tests
 	VolumeOwnershipApplicator VolumeOwnershipChanger
 	ReconstructedVolume       bool
+
+	// IsRemount is true when SetUp is being invoked on a volume that the
+	// reconciler considers already mounted to the pod, e.g. a periodic
+	// republish triggered by CSIDriver.spec.requiresRepublish=true. Volume
+	// plugins should use this to avoid destroying state (e.g. mount
+	// directories, volume metadata files) that the pod is currently
+	// observing through an existing bind mount, since teardown on a
+	// failed remount cannot be repaired by a subsequent successful
+	// remount and would leave the pod with stale contents.
+	IsRemount bool
 }
 
 type VolumeOwnershipChanger interface {