diff --git a/nomad/state/state_store.go b/nomad/state/state_store.go index d8ceac4c9..5d26959eb 100644 --- a/nomad/state/state_store.go +++ b/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 diff --git a/nomad/structs/csi.go b/nomad/structs/csi.go index 5bd3afb32..75d034016 100644 --- a/nomad/structs/csi.go +++ b/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 diff --git a/nomad/structs/csi_test.go b/nomad/structs/csi_test.go index 26b4b4f97..cdcb7c511 100644 --- a/nomad/structs/csi_test.go +++ b/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) } }) }