backport of commit 9b74e11f064ecc53a53f13e82419927b533a9e4a (#18589)

Co-authored-by: Daniel Bennett <dbennett@hashicorp.com>

nomad/state/state_store.go

@@ -2428,18 +2428,10 @@ func (s *StateStore) UpsertCSIVolume(index uint64, volumes []*structs.CSIVolume)
 				old.Provider != v.Provider {
 				return fmt.Errorf("volume identity cannot be updated: %s", v.ID)
 			}
-
-			// Update fields that are safe to change while volume is being used.
-			if err := old.UpdateSafeFields(v); err != nil {
-				return fmt.Errorf("unable to update in-use volume: %w", err)
-			}
-			v = old
-			v.ModifyIndex = index
-
 		} else {
 			v.CreateIndex = index
-			v.ModifyIndex = index
 		}
+		v.ModifyIndex = index
 
 		// Allocations are copy on write, so we want to keep the Allocation ID
 		// but we need to clear the pointer so that we don't store it when we

nomad/structs/csi.go

@@ -813,9 +813,15 @@ func (v *CSIVolume) Merge(other *CSIVolume) error {
 		}
 	}
 
-	// MountOptions can be updated so long as the volume isn't in use,
+	// MountOptions can be updated so long as the volume isn't in use
-	// but the caller will reject updating an in-use volume
+	if v.InUse() {
-	v.MountOptions = other.MountOptions
+		if !v.MountOptions.Equal(other.MountOptions) {
+			errs = multierror.Append(errs, errors.New(
+				"can not update mount options while volume is in use"))
+		}
+	} else {
+		v.MountOptions = other.MountOptions
+	}
 
 	// Secrets can be updated freely
 	v.Secrets = other.Secrets
@@ -833,20 +839,6 @@ func (v *CSIVolume) Merge(other *CSIVolume) error {
 	return errs.ErrorOrNil()
 }
 
-// UpdateSafeFields updates fields that may be mutated while the volume is in use.
-func (v *CSIVolume) UpdateSafeFields(other *CSIVolume) error {
-	if v == nil || other == nil {
-		return errors.New("unexpected nil volume (this is a bug)")
-	}
-
-	// Expand operation can sometimes happen while in-use.
-	v.Capacity = other.Capacity
-	v.RequestedCapacityMin = other.RequestedCapacityMin
-	v.RequestedCapacityMax = other.RequestedCapacityMax
-
-	return nil
-}
-
 // Request and response wrappers
 type CSIVolumeRegisterRequest struct {
 	Volumes []*CSIVolume

nomad/structs/csi_test.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/hashicorp/nomad/ci"
+	"github.com/shoenig/test"
 	"github.com/shoenig/test/must"
 	"github.com/stretchr/testify/require"
 )
@@ -626,7 +627,7 @@ func TestCSIVolume_Merge(t *testing.T) {
 			update:   &CSIVolume{},
 			expected: "volume topology request update was not compatible with existing topology",
 			expectFn: func(t *testing.T, v *CSIVolume) {
-				require.Len(t, v.Topologies, 1)
+				must.Len(t, 1, v.Topologies)
 			},
 		},
 		{
@@ -646,8 +647,8 @@ func TestCSIVolume_Merge(t *testing.T) {
 			},
 			expected: "volume topology request update was not compatible with existing topology",
 			expectFn: func(t *testing.T, v *CSIVolume) {
-				require.Len(t, v.Topologies, 1)
+				must.Len(t, 1, v.Topologies)
-				require.Equal(t, "R1", v.Topologies[0].Segments["rack"])
+				must.Eq(t, "R1", v.Topologies[0].Segments["rack"])
 			},
 		},
 		{
@@ -675,6 +676,20 @@ func TestCSIVolume_Merge(t *testing.T) {
 			},
 			expected: "volume topology request update was not compatible with existing topology",
 		},
+		{
+			name: "invalid mount options while in use",
+			v: &CSIVolume{
+				// having any allocs means it's "in use"
+				ReadAllocs: map[string]*Allocation{
+					"test-alloc": {ID: "anything"},
+				},
+			},
+			update: &CSIVolume{
+				MountOptions: &CSIMountOptions{
+					MountFlags: []string{"any flags"}},
+			},
+			expected: "can not update mount options while volume is in use",
+		},
 		{
 			name: "valid update",
 			v: &CSIVolume{
@@ -704,7 +719,7 @@ func TestCSIVolume_Merge(t *testing.T) {
 				},
 				MountOptions: &CSIMountOptions{
 					FSType:     "ext4",
-					MountFlags: []string{"noatime"},
+					MountFlags: []string{"noatime", "another"},
 				},
 				RequestedTopologies: &CSITopologyRequest{
 					Required: []*CSITopology{
@@ -725,20 +740,26 @@ func TestCSIVolume_Merge(t *testing.T) {
 					},
 				},
 			},
+			expectFn: func(t *testing.T, v *CSIVolume) {
+				test.Len(t, 2, v.RequestedCapabilities,
+					test.Sprint("should add 2 requested capabilities"))
+				test.Eq(t, []string{"noatime", "another"}, v.MountOptions.MountFlags,
+					test.Sprint("should add mount flag"))
+			},
 		},
 	}
 	for _, tc := range testCases {
-		tc = tc
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			err := tc.v.Merge(tc.update)
+			if tc.expectFn != nil {
+				tc.expectFn(t, tc.v)
+			}
 			if tc.expected == "" {
-				require.NoError(t, err)
+				must.NoError(t, err)
 			} else {
-				if tc.expectFn != nil {
+				must.Error(t, err)
-					tc.expectFn(t, tc.v)
+				must.ErrorContains(t, err, tc.expected)
-				}
-				require.Error(t, err, tc.expected)
-				require.Contains(t, err.Error(), tc.expected)
 			}
 		})
 	}