csi: fix volume updating behavior

fix for part of: c6dbba7
which allowed updating volumes while in use,
because CSI expand may occur while in use.
but it mistakenly stopped copying other
important values that may be updated
whether in use or not.

this moves some of the in-use validation to
happen during Merge(), before writing to state,
leaving UpsertCSIVolume with only minimal final
sanity-checking.

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,
-	// but the caller will reject updating an in-use volume
-	v.MountOptions = other.MountOptions
+	// MountOptions can be updated so long as the volume isn't in use
+	if v.InUse() {
+		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)
-				require.Equal(t, "R1", v.Topologies[0].Segments["rack"])
+				must.Len(t, 1, v.Topologies)
+				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 {
-					tc.expectFn(t, tc.v)
-				}
-				require.Error(t, err, tc.expected)
-				require.Contains(t, err.Error(), tc.expected)
+				must.Error(t, err)
+				must.ErrorContains(t, err, tc.expected)
 			}
 		})
 	}