Skip to content

Commit b107469

Browse files
jwebster7jwebster7
authored
Handle CSI publish during volume move
This commit fixes a race condition between CSI publish and TVM workflows which resulted in CSI attaching with stale access info.
1 parent ebeccf2 commit b107469

8 files changed

Lines changed: 577 additions & 423 deletions

File tree

‎core/concurrent_core.go‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4607,9 +4607,10 @@ func (o *ConcurrentTridentOrchestrator) MoveVolume(
46074607
return errors.InvalidInputError("volume move info is nil")
46084608
}
46094609

4610-
_, results, unlocker, err := db.Lock(
4611-
ctx, db.Query(db.UpsertVolume(volumeMoveInfo.VolumeName, ""), db.ReadBackend("")),
4612-
)
4610+
_, results, unlocker, err := db.Lock(ctx, db.Query(
4611+
db.UpsertVolume(volumeMoveInfo.VolumeName, ""),
4612+
db.ReadBackend(""),
4613+
))
46134614
defer unlocker()
46144615
if err != nil {
46154616
return err
@@ -4640,18 +4641,16 @@ func (o *ConcurrentTridentOrchestrator) MoveVolume(
46404641
// Backend moves should return transient errors until a terminal failure
46414642
// is hit, or the move succeeds which results in a nil return from the
46424643
// backend.
4643-
defer func() {
4644-
upserter(volume)
4645-
}()
4646-
volume.Config.MoveInfo = volumeMoveInfo.DeepCopy()
4647-
46484644
if err != nil {
46494645
return
46504646
}
4651-
// If no error is returned, the move is complete.
4647+
if volumeMoveInfo.DryRun {
4648+
return
4649+
}
46524650

4653-
// Persist the moved pool only when the backend reports the move complete.
4654-
if !volumeMoveInfo.DryRun && volumeMoveInfo.TargetPool != "" {
4651+
// Persist the moved pool only when the move succeeds.
4652+
volume.Config.MoveInfo = volumeMoveInfo.DeepCopy()
4653+
if volumeMoveInfo.TargetPool != "" {
46554654
volume.Pool = volumeMoveInfo.TargetPool
46564655
}
46574656

@@ -4662,8 +4661,11 @@ func (o *ConcurrentTridentOrchestrator) MoveVolume(
46624661
Logc(ctx).WithFields(LogFields{
46634662
"volume": volume.Config.Name,
46644663
"backendUUID": volume.BackendUUID,
4665-
}).WithError(err).Error("Failed to update volume after staging volume move.")
4664+
}).WithError(err).Error("Failed to update volume after volume move.")
4665+
return
46664666
}
4667+
4668+
upserter(volume)
46674669
}()
46684670

46694671
err = backend.MoveVolume(ctx, volume.Config, volumeMoveInfo)
@@ -4734,7 +4736,7 @@ func (o *ConcurrentTridentOrchestrator) UnstageVolumeMove(
47344736
}).WithError(err).Error("Failed to unstage volume move.")
47354737
return fmt.Errorf("failed to unstage volume move for %s: %w", volume.Config.Name, err)
47364738
}
4737-
volume.Config.MoveInfo = nil
4739+
volume.Config.MoveInfo = volumeMoveInfo.DeepCopy()
47384740
upserter(volume)
47394741

47404742
Logc(ctx).WithFields(LogFields{

‎core/concurrent_core_test.go‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -576,6 +576,8 @@ func TestBootstrapConcurrentCore(t *testing.T) {
576576
mockStoreClient.EXPECT().GetNodes(gomock.Any()).Return(nodes, nil).AnyTimes()
577577
mockStoreClient.EXPECT().IsBackendDeleting(gomock.Any(), gomock.Any()).Return(false).AnyTimes()
578578
mockStoreClient.EXPECT().GetVolumeMoves(gomock.Any()).Return(nil, nil).AnyTimes()
579+
// VP sync happens asynchronously, so we allow UpdateVolumePublication to be called
580+
mockStoreClient.EXPECT().UpdateVolumePublication(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
579581
},
580582
verifyError: func(err error) {
581583
assert.NoError(t, err)
@@ -675,6 +677,8 @@ func TestBootstrapConcurrentCore(t *testing.T) {
675677
mockStoreClient.EXPECT().GetNodes(gomock.Any()).Return(nodes, nil).AnyTimes()
676678
mockStoreClient.EXPECT().IsBackendDeleting(gomock.Any(), gomock.Any()).Return(false).AnyTimes()
677679
mockStoreClient.EXPECT().GetVolumeMoves(gomock.Any()).Return(nil, nil).AnyTimes()
680+
// VP sync happens asynchronously, so we allow UpdateVolumePublication to be called
681+
mockStoreClient.EXPECT().UpdateVolumePublication(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
678682
},
679683
verifyError: func(err error) {
680684
assert.NoError(t, err)
@@ -23209,7 +23213,7 @@ func TestConcurrentTridentOrchestrator_StageVolumeMove(t *testing.T) {
2320923213
assert.Nil(t, cached.Config.MoveInfo, "backend stage failure must not set MoveInfo")
2321023214
})
2321123215

23212-
t.Run("happy path records MoveInfo", func(t *testing.T) {
23216+
t.Run("happy path sets MoveInfo on success", func(t *testing.T) {
2321323217
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
2321423218

2321523219
mockBackend.EXPECT().StageVolumeMove(gomock.Any(), gomock.Any(), gomock.Any()).
@@ -23222,7 +23226,7 @@ func TestConcurrentTridentOrchestrator_StageVolumeMove(t *testing.T) {
2322223226

2322323227
cached := getVolumeByNameFromCache(t, vol.Config.Name)
2322423228
require.NotNil(t, cached)
23225-
require.NotNil(t, cached.Config.MoveInfo)
23229+
require.NotNil(t, cached.Config.MoveInfo, "StageVolumeMove must set MoveInfo on success")
2322623230
assert.Equal(t, models.VolumeMoveStateControllerStaging, cached.Config.MoveInfo.State)
2322723231
})
2322823232
}
@@ -23265,7 +23269,7 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {
2326523269
assert.True(t, errors.IsVolumeStateError(err))
2326623270
})
2326723271

23268-
t.Run("backend transient error still updates MoveInfo", func(t *testing.T) {
23272+
t.Run("backend transient error does not project MoveInfo", func(t *testing.T) {
2326923273
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
2327023274
vol.Pool = "pool-original"
2327123275
addVolumesToCache(t, vol)
@@ -23279,12 +23283,11 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {
2327923283

2328023284
cached := getVolumeByNameFromCache(t, vol.Config.Name)
2328123285
require.NotNil(t, cached)
23282-
require.NotNil(t, cached.Config.MoveInfo, "transient error must still update MoveInfo")
23283-
assert.Equal(t, models.VolumeMoveStateMoving, cached.Config.MoveInfo.State)
23286+
assert.Nil(t, cached.Config.MoveInfo, "MoveVolume must not project MoveInfo; commitState owns projection")
2328423287
assert.Equal(t, "pool-original", cached.Pool, "pool should not change on transient move errors")
2328523288
})
2328623289

23287-
t.Run("happy path records MoveInfo", func(t *testing.T) {
23290+
t.Run("happy path persists target pool and sets MoveInfo", func(t *testing.T) {
2328823291
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
2328923292
vol.Pool = "pool-original"
2329023293
addVolumesToCache(t, vol)
@@ -23301,7 +23304,7 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {
2330123304
cached := getVolumeByNameFromCache(t, vol.Config.Name)
2330223305
require.NotNil(t, cached)
2330323306
require.NotNil(t, cached.Config.MoveInfo)
23304-
assert.Equal(t, models.VolumeMoveStateMoving, cached.Config.MoveInfo.State)
23307+
assert.NotNil(t, cached.Config.MoveInfo, "MoveVolume must set MoveInfo on success")
2330523308
assert.Equal(t, "pool-target", cached.Pool, "pool should be updated to move target on success")
2330623309
})
2330723310
}
@@ -23351,7 +23354,7 @@ func TestConcurrentTridentOrchestrator_UnstageVolumeMove(t *testing.T) {
2335123354
"backend unstage failure must leave MoveInfo untouched")
2335223355
})
2335323356

23354-
t.Run("happy path clears MoveInfo", func(t *testing.T) {
23357+
t.Run("happy path does not clear MoveInfo", func(t *testing.T) {
2335523358
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
2335623359

2335723360
vol.Config.MoveInfo = newConcMoveInfo(vol.Config.Name, models.VolumeMoveStateControllerUnstaging)
@@ -23367,8 +23370,7 @@ func TestConcurrentTridentOrchestrator_UnstageVolumeMove(t *testing.T) {
2336723370

2336823371
cached := getVolumeByNameFromCache(t, vol.Config.Name)
2336923372
require.NotNil(t, cached)
23370-
assert.Nil(t, cached.Config.MoveInfo,
23371-
"successful unstage must clear MoveInfo")
23373+
assert.NotNil(t, cached.Config.MoveInfo, "UnstageVolumeMove must not clear MoveInfo")
2337223374
})
2337323375
}
2337423376

‎core/orchestrator_core.go‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5974,18 +5974,18 @@ func (o *TridentOrchestrator) StageVolumeMove(
59745974
return errors.NotFoundError("backend %s not found for volume %s", volume.BackendUUID, volume.Config.Name)
59755975
}
59765976

5977-
initialConfig := volume.Config.ConstructClone()
5978-
err = backend.StageVolumeMove(ctx, volume.Config, volumeMoveInfo)
5977+
mutableConfig := volume.Config.ConstructClone()
5978+
err = backend.StageVolumeMove(ctx, mutableConfig, volumeMoveInfo)
59795979
if err != nil {
5980-
volume.Config = initialConfig
59815980
Logc(ctx).WithFields(LogFields{
59825981
"volume": volume.Config.Name,
59835982
"backendUUID": volume.BackendUUID,
59845983
"error": err,
59855984
}).Error("Failed to stage volume move.")
59865985
return fmt.Errorf("failed to stage volume move for %s: %w", volume.Config.Name, err)
59875986
}
5988-
volume.Config.MoveInfo = volumeMoveInfo.DeepCopy()
5987+
mutableConfig.MoveInfo = volumeMoveInfo.DeepCopy()
5988+
volume.Config = mutableConfig
59895989

59905990
Logc(ctx).WithFields(LogFields{
59915991
"volume": volume.Config.Name,
@@ -6026,6 +6026,7 @@ func (o *TridentOrchestrator) MoveVolume(ctx context.Context, volumeMoveInfo *mo
60266026
return errors.NotFoundError("backend %s not found for volume %s", volume.BackendUUID, volume.Config.Name)
60276027
}
60286028

6029+
mutableConfig := volume.Config.ConstructClone()
60296030
defer func() {
60306031
// Move is special in that it may have updated state from the backend
60316032
// that could be useful to persist on the storage.Volume reference in
@@ -6034,30 +6035,38 @@ func (o *TridentOrchestrator) MoveVolume(ctx context.Context, volumeMoveInfo *mo
60346035
// Backend moves should return transient errors until a terminal failure
60356036
// is hit, or the move succeeds which results in a nil return from the
60366037
// backend.
6037-
volume.Config.MoveInfo = volumeMoveInfo.DeepCopy()
6038-
60396038
if err != nil {
60406039
return
60416040
}
6042-
// If no error is returned, the move is complete.
6041+
if volumeMoveInfo.DryRun {
6042+
return
6043+
}
6044+
mutableConfig.MoveInfo = volumeMoveInfo.DeepCopy()
6045+
6046+
// Create a copy of the mutable state for rollback.
6047+
initialPool := volume.Pool
6048+
initialConfig := volume.Config
60436049

6044-
// Persist the moved pool only when the backend reports the move complete.
6045-
if !volumeMoveInfo.DryRun && volumeMoveInfo.TargetPool != "" {
6050+
// Commit the backend mutations to the volume.
6051+
volume.Config = mutableConfig
6052+
if volumeMoveInfo.TargetPool != "" {
60466053
volume.Pool = volumeMoveInfo.TargetPool
60476054
}
60486055

60496056
// Update the volume persistence layer once the
60506057
// move completes in the backend.
60516058
err = o.storeClient.UpdateVolume(ctx, volume)
60526059
if err != nil {
6060+
volume.Pool = initialPool
6061+
volume.Config = initialConfig
60536062
Logc(ctx).WithFields(LogFields{
60546063
"volume": volume.Config.Name,
60556064
"backendUUID": volume.BackendUUID,
6056-
}).WithError(err).Error("Failed to update volume after staging volume move.")
6065+
}).WithError(err).Error("Failed to update volume after volume move.")
60576066
}
60586067
}()
60596068

6060-
err = backend.MoveVolume(ctx, volume.Config, volumeMoveInfo)
6069+
err = backend.MoveVolume(ctx, mutableConfig, volumeMoveInfo)
60616070
if err != nil {
60626071
return err
60636072
}
@@ -6106,18 +6115,18 @@ func (o *TridentOrchestrator) UnstageVolumeMove(
61066115
return errors.NotFoundError("backend %s not found for volume %s", volume.BackendUUID, volume.Config.Name)
61076116
}
61086117

6109-
initialConfig := volume.Config.ConstructClone()
6110-
err = backend.UnstageVolumeMove(ctx, volume.Config, volumeMoveInfo)
6118+
mutableConfig := volume.Config.ConstructClone()
6119+
err = backend.UnstageVolumeMove(ctx, mutableConfig, volumeMoveInfo)
61116120
if err != nil {
6112-
volume.Config = initialConfig
61136121
Logc(ctx).WithFields(LogFields{
61146122
"volume": volume.Config.Name,
61156123
"backendUUID": volume.BackendUUID,
61166124
"error": err,
61176125
}).Error("Failed to unstage volume move.")
61186126
return fmt.Errorf("failed to unstage volume move for %s: %w", volume.Config.Name, err)
61196127
}
6120-
volume.Config.MoveInfo = nil
6128+
mutableConfig.MoveInfo = volumeMoveInfo.DeepCopy()
6129+
volume.Config = mutableConfig
61216130

61226131
Logc(ctx).WithFields(LogFields{
61236132
"volume": volume.Config.Name,

‎core/orchestrator_core_test.go‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -13890,7 +13890,7 @@ func TestTridentOrchestrator_StageVolumeMove(t *testing.T) {
1389013890
"backend stage failure must leave MoveInfo untouched")
1389113891
})
1389213892

13893-
t.Run("happy path records MoveInfo", func(t *testing.T) {
13893+
t.Run("happy path preserves MoveInfo", func(t *testing.T) {
1389413894
o, vol, mockStoreClient, mockCtrl := setupOrchestratorForMoveInfo(t, volName, backendUUID)
1389513895

1389613896
mockBackend := mockstorage.NewMockBackend(mockCtrl)
@@ -13903,8 +13903,7 @@ func TestTridentOrchestrator_StageVolumeMove(t *testing.T) {
1390313903
err := o.StageVolumeMove(ctx(),
1390413904
newMoveInfo(volName, models.VolumeMoveStateControllerStaging))
1390513905
require.NoError(t, err)
13906-
require.NotNil(t, vol.Config.MoveInfo)
13907-
assert.Equal(t, models.VolumeMoveStateControllerStaging, vol.Config.MoveInfo.State)
13906+
assert.NotNil(t, vol.Config.MoveInfo, "StageVolumeMove must not unset MoveInfo")
1390813907
})
1390913908
}
1391013909

@@ -13957,7 +13956,7 @@ func TestTridentOrchestrator_MoveVolume(t *testing.T) {
1395713956
assert.True(t, errors.IsNotFoundError(err))
1395813957
})
1395913958

13960-
t.Run("backend transient error still updates MoveInfo", func(t *testing.T) {
13959+
t.Run("backend transient error does not set MoveInfo", func(t *testing.T) {
1396113960
o, vol, mockStoreClient, mockCtrl := setupOrchestratorForMoveInfo(t, volName, backendUUID)
1396213961
vol.Pool = "pool-original"
1396313962

@@ -13970,12 +13969,11 @@ func TestTridentOrchestrator_MoveVolume(t *testing.T) {
1397013969

1397113970
err := o.MoveVolume(ctx(), newMoveInfo(volName, models.VolumeMoveStateMoving))
1397213971
require.Error(t, err)
13973-
require.NotNil(t, vol.Config.MoveInfo, "transient error must still update MoveInfo")
13974-
assert.Equal(t, models.VolumeMoveStateMoving, vol.Config.MoveInfo.State)
13972+
assert.Nil(t, vol.Config.MoveInfo, "MoveVolume must not set MoveInfo on failure")
1397513973
assert.Equal(t, "pool-original", vol.Pool, "pool should not change on transient move errors")
1397613974
})
1397713975

13978-
t.Run("happy path records MoveInfo", func(t *testing.T) {
13976+
t.Run("happy path persists target pool and sets MoveInfo", func(t *testing.T) {
1397913977
o, vol, mockStoreClient, mockCtrl := setupOrchestratorForMoveInfo(t, volName, backendUUID)
1398013978
vol.Pool = "pool-original"
1398113979
moveInfo := newMoveInfo(volName, models.VolumeMoveStateMoving)
@@ -13990,8 +13988,7 @@ func TestTridentOrchestrator_MoveVolume(t *testing.T) {
1399013988

1399113989
err := o.MoveVolume(ctx(), moveInfo)
1399213990
require.NoError(t, err)
13993-
require.NotNil(t, vol.Config.MoveInfo)
13994-
assert.Equal(t, models.VolumeMoveStateMoving, vol.Config.MoveInfo.State)
13991+
assert.NotNil(t, vol.Config.MoveInfo, "MoveVolume must not unset MoveInfo")
1399513992
assert.Equal(t, "pool-target", vol.Pool, "pool should be updated to move target on success")
1399613993
})
1399713994
}
@@ -14050,11 +14047,10 @@ func TestTridentOrchestrator_UnstageVolumeMove(t *testing.T) {
1405014047
err := o.UnstageVolumeMove(ctx(),
1405114048
newMoveInfo(volName, models.VolumeMoveStateControllerUnstaging))
1405214049
require.Error(t, err)
14053-
assert.NotNil(t, vol.Config.MoveInfo,
14054-
"backend unstage failure must leave MoveInfo untouched")
14050+
assert.NotNil(t, vol.Config.MoveInfo, "backend unstage failure must leave MoveInfo untouched")
1405514051
})
1405614052

14057-
t.Run("happy path clears MoveInfo", func(t *testing.T) {
14053+
t.Run("happy path does not clear MoveInfo", func(t *testing.T) {
1405814054
o, vol, mockStoreClient, mockCtrl := setupOrchestratorForMoveInfo(t, volName, backendUUID)
1405914055

1406014056
vol.Config.MoveInfo = newMoveInfo(volName, models.VolumeMoveStateControllerUnstaging)
@@ -14069,8 +14065,7 @@ func TestTridentOrchestrator_UnstageVolumeMove(t *testing.T) {
1406914065
err := o.UnstageVolumeMove(ctx(),
1407014066
newMoveInfo(volName, models.VolumeMoveStateControllerUnstaging))
1407114067
require.NoError(t, err)
14072-
assert.Nil(t, vol.Config.MoveInfo,
14073-
"successful unstage must clear MoveInfo")
14068+
assert.NotNil(t, vol.Config.MoveInfo, "UnstageVolumeMove must not clear MoveInfo")
1407414069
})
1407514070
}
1407614071

0 commit comments

Comments
 (0)