Skip to content

fix: preserve backend volume associations during reconciliation #1167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
s-starostin wants to merge 1 commit into
base: master
Choose a base branch
from
+103 −24
Open

fix: preserve backend volume associations during reconciliation #1167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions core/concurrent_core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1725,6 +1725,15 @@ func (o *ConcurrentTridentOrchestrator) upsertBackend(
if err != nil {
return nil, err
}
// A replacement backend is created with an empty local volume map.
// Preserve the existing backend-to-volume associations before node
// access reconciliation and before replacing the cached backend.
volumeCount := copyBackendVolumes(originalBackend, backend)
Logc(ctx).WithFields(LogFields{
"backend": backend.Name(),
"backendUUID": backend.BackendUUID(),
"volumeCount": volumeCount,
}).Debug("Copied volume associations to replacement backend.")
}

// Node access rules may have changed in the backend config
Expand Down Expand Up @@ -9656,3 +9665,22 @@ func (o *ConcurrentTridentOrchestrator) resolveAndSetEffectiveAutogrowPolicy(

return effectiveAGPolicy, policyErr
}

func copyBackendVolumes(originalBackend, newBackend storage.Backend) int {
if originalBackend == nil || newBackend == nil {
return 0
}

if originalBackend.BackendUUID() != newBackend.BackendUUID() {
return 0
}

count := 0
originalBackend.Volumes().Range(func(key, value interface{}) bool {
newBackend.Volumes().Store(key, value)
count++
return true
})

return count
}
99 changes: 75 additions & 24 deletions core/concurrent_core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,34 @@ func getFakeVolume(name, backendUUID string) *storage.Volume {
return fakeVolume
}

func countBackendVolumes(backend storage.Backend) int {
if backend == nil {
return 0
}

count := 0
backend.Volumes().Range(func(_, _ any) bool {
count++
return true
})

return count
}

func requireBackendVolume(
t *testing.T, backend storage.Backend, volumeName, expectedBackendUUID string,
) {
t.Helper()

value, found := backend.Volumes().Load(volumeName)
require.Truef(t, found, "volume %q is absent from backend %q", volumeName, backend.Name())

volume, ok := value.(*storage.Volume)
require.Truef(t, ok, "unexpected value type for volume %q: %T", volumeName, value)
require.Equal(t, volumeName, volume.Config.Name)
require.Equal(t, expectedBackendUUID, volume.BackendUUID)
}

func getFakeNode(name string) *models.Node {
fakeNode := &models.Node{
Name: name,
Expand Down Expand Up @@ -581,9 +609,7 @@ func TestBootstrapConcurrentCore(t *testing.T) {
mockStoreClient.EXPECT().GetVolumePublications(gomock.Any()).Return(pubs, nil).AnyTimes()
mockStoreClient.EXPECT().GetNodes(gomock.Any()).Return(nodes, nil).AnyTimes()
mockStoreClient.EXPECT().IsBackendDeleting(gomock.Any(), gomock.Any()).Return(false).AnyTimes()
// Bootstrap backfills the node-name label on existing publications,
// which triggers an asynchronous UpdateVolumePublication; allow it.
mockStoreClient.EXPECT().UpdateVolumePublication(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockStoreClient.EXPECT().GetVolumeMoves(gomock.Any()).Return(nil, nil).AnyTimes()
},
verifyError: func(err error) {
assert.NoError(t, err)
Expand Down Expand Up @@ -682,9 +708,7 @@ func TestBootstrapConcurrentCore(t *testing.T) {
mockStoreClient.EXPECT().GetVolumePublications(gomock.Any()).Return(pubs, nil).AnyTimes()
mockStoreClient.EXPECT().GetNodes(gomock.Any()).Return(nodes, nil).AnyTimes()
mockStoreClient.EXPECT().IsBackendDeleting(gomock.Any(), gomock.Any()).Return(false).AnyTimes()
// Bootstrap backfills the node-name label on existing publications,
// which triggers an asynchronous UpdateVolumePublication; allow it.
mockStoreClient.EXPECT().UpdateVolumePublication(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
mockStoreClient.EXPECT().GetVolumeMoves(gomock.Any()).Return(nil, nil).AnyTimes()
},
verifyError: func(err error) {
assert.NoError(t, err)
Expand Down Expand Up @@ -13708,12 +13732,12 @@ func TestPeriodicallyReconcileNodeAccessOnBackendsConcurrentCore(t *testing.T) {

// Start loop
go o.PeriodicallyReconcileNodeAccessOnBackends()
for o.getStopNodeAccessLoop() == nil {
for o.stopNodeAccessLoop == nil {
// Wait for loop to initialize
time.Sleep(10 * time.Millisecond)
}

assert.NotNil(t, o.getStopNodeAccessLoop(), "loop channel should be initialized")
assert.NotNil(t, o.stopNodeAccessLoop, "loop channel should be initialized")

time.Sleep(500 * time.Millisecond) // Wait for loop to do some work

Expand All @@ -13737,7 +13761,7 @@ func TestPeriodicallyReconcileBackendStateConcurrentCore(t *testing.T) {
// No setup needed for zero interval test
},
verifyBehavior: func(t *testing.T, o *ConcurrentTridentOrchestrator) {
assert.Nil(t, o.getStopReconcileBackendLoop(), "reconcile backend loop should not have started")
assert.Nil(t, o.stopReconcileBackendLoop, "reconcile backend loop should not have started")
},
},
{
Expand All @@ -13758,7 +13782,7 @@ func TestPeriodicallyReconcileBackendStateConcurrentCore(t *testing.T) {
},
verifyBehavior: func(t *testing.T, o *ConcurrentTridentOrchestrator) {
// The loop should be running (stopReconcileBackendLoop should not be nil)
assert.NotNil(t, o.getStopReconcileBackendLoop(), "reconcile backend loop should have started")
assert.NotNil(t, o.stopReconcileBackendLoop, "reconcile backend loop should have started")
},
},
{
Expand All @@ -13769,7 +13793,7 @@ func TestPeriodicallyReconcileBackendStateConcurrentCore(t *testing.T) {
},
verifyBehavior: func(t *testing.T, o *ConcurrentTridentOrchestrator) {
// The loop should still be running even with no backends
assert.NotNil(t, o.getStopReconcileBackendLoop(), "reconcile backend loop should have started")
assert.NotNil(t, o.stopReconcileBackendLoop, "reconcile backend loop should have started")
},
},
{
Expand All @@ -13790,7 +13814,7 @@ func TestPeriodicallyReconcileBackendStateConcurrentCore(t *testing.T) {
},
verifyBehavior: func(t *testing.T, o *ConcurrentTridentOrchestrator) {
// The loop should be running
assert.NotNil(t, o.getStopReconcileBackendLoop(), "reconcile backend loop should have started")
assert.NotNil(t, o.stopReconcileBackendLoop, "reconcile backend loop should have started")
},
},
}
Expand All @@ -13813,7 +13837,7 @@ func TestPeriodicallyReconcileBackendStateConcurrentCore(t *testing.T) {

go o.PeriodicallyReconcileBackendState(tt.pollInterval)
if tt.pollInterval > 0 {
for o.getStopReconcileBackendLoop() == nil {
for o.stopReconcileBackendLoop == nil {
// Wait for loop to initialize
time.Sleep(10 * time.Millisecond)
}
Expand All @@ -13840,6 +13864,7 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) {
backendUUID string
setupMocks func(mockCtrl *gomock.Controller, mockStoreClient *mockpersistentstore.MockStoreClient, o *ConcurrentTridentOrchestrator) storage.Backend
verifyError func(t *testing.T, err error)
verifyState func(t *testing.T)
}{
{
name: "BackendNotFound",
Expand Down Expand Up @@ -14006,7 +14031,7 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) {
},
},
{
name: "ReconcileBackendPoolsChange",
name: "ReconcileBackendPoolsChangePreservesVolumes",
backendUUID: "uuid1",
setupMocks: func(mockCtrl *gomock.Controller, mockStoreClient *mockpersistentstore.MockStoreClient, o *ConcurrentTridentOrchestrator) storage.Backend {
mockBackend := getMockBackendWithMap(mockCtrl, []storage.Pool{fakePool1}, map[string]string{
Expand All @@ -14021,7 +14046,14 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) {
fakeBackend := getFakeBackend("backend1", "uuid1", nil)
fakeConfigBytes, _ := fakeBackend.MarshalDriverConfig()

// Setup backend expectations for offline backend
volume1 := getFakeVolume("volume1", "uuid1")
volume2 := getFakeVolume("volume2", "uuid1")
backendVolumes := makeSyncMapFromMap(map[string]*storage.Volume{
volume1.Config.Name: volume1,
volume2.Config.Name: volume2,
})

// Setup backend expectations.
mockBackend.EXPECT().CanGetState().Return(true).Times(1)
mockBackend.EXPECT().GetBackendState(gomock.Any()).Return("", bitset).Times(2)
mockBackend.EXPECT().UpdateBackendState(gomock.Any(), "").Times(1)
Expand All @@ -14034,19 +14066,33 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) {
mockBackend.EXPECT().State().Return(storage.Online).AnyTimes()
mockBackend.EXPECT().Online().Return(true).AnyTimes()
mockBackend.EXPECT().HasVolumes().Return(true).AnyTimes()
mockBackend.EXPECT().Volumes().Return(backendVolumes).AnyTimes()
mockBackend.EXPECT().SmartCopy().Return(mockBackend).AnyTimes()
mockBackend.EXPECT().ConstructPersistent(gomock.Any()).Return(&storage.BackendPersistent{Name: "backend1", BackendUUID: "uuid1"}).AnyTimes()
mockBackend.EXPECT().Terminate(gomock.Any()).Times(1)
mockStoreClient.EXPECT().UpdateBackend(gomock.Any(), gomock.Any()).Return(nil).Times(1)

// Add backend to cache
// Model the state created by bootstrap: both the global volume cache
// and the original backend-local volume map contain the volumes.
addBackendsToCache(t, mockBackend)
addVolumesToCache(t, volume1, volume2)

return mockBackend
},
verifyError: func(t *testing.T, err error) {
assert.NoError(t, err)
},
verifyState: func(t *testing.T) {
replacement := getBackendByUuidFromCache(t, "uuid1")
require.NotNil(t, replacement)
require.Equal(t, 2, countBackendVolumes(replacement))
requireBackendVolume(t, replacement, "volume1", "uuid1")
requireBackendVolume(t, replacement, "volume2", "uuid1")

// The independent global volume cache must remain intact as well.
require.Equal(t, "uuid1", getVolumeByNameFromCache(t, "volume1").BackendUUID)
require.Equal(t, "uuid1", getVolumeByNameFromCache(t, "volume2").BackendUUID)
},
},
{
name: "ReconcileBackendVersionChangeStoreError",
Expand Down Expand Up @@ -14110,6 +14156,9 @@ func TestReconcileBackendStateConcurrentCore(t *testing.T) {
if tt.verifyError != nil {
tt.verifyError(t, err)
}
if tt.verifyState != nil {
tt.verifyState(t)
}

persistenceCleanup(t, o)
})
Expand Down Expand Up @@ -23219,7 +23268,7 @@ func TestConcurrentTridentOrchestrator_StageVolumeMove(t *testing.T) {
assert.Nil(t, cached.Config.MoveInfo, "backend stage failure must not set MoveInfo")
})

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

mockBackend.EXPECT().StageVolumeMove(gomock.Any(), gomock.Any(), gomock.Any()).
Expand All @@ -23232,7 +23281,7 @@ func TestConcurrentTridentOrchestrator_StageVolumeMove(t *testing.T) {

cached := getVolumeByNameFromCache(t, vol.Config.Name)
require.NotNil(t, cached)
require.NotNil(t, cached.Config.MoveInfo, "StageVolumeMove must set MoveInfo on success")
require.NotNil(t, cached.Config.MoveInfo)
assert.Equal(t, models.VolumeMoveStateControllerStaging, cached.Config.MoveInfo.State)
})
}
Expand Down Expand Up @@ -23275,7 +23324,7 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {
assert.True(t, errors.IsVolumeStateError(err))
})

t.Run("backend transient error does not project MoveInfo", func(t *testing.T) {
t.Run("backend transient error still updates MoveInfo", func(t *testing.T) {
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
vol.Pool = "pool-original"
addVolumesToCache(t, vol)
Expand All @@ -23289,11 +23338,12 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {

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

t.Run("happy path persists target pool and sets MoveInfo", func(t *testing.T) {
t.Run("happy path records MoveInfo", func(t *testing.T) {
o, vol, mockBackend, mockStoreClient, _ := setupConcurrentMoveInfoFixture(t)
vol.Pool = "pool-original"
addVolumesToCache(t, vol)
Expand All @@ -23310,7 +23360,7 @@ func TestConcurrentTridentOrchestrator_MoveVolume(t *testing.T) {
cached := getVolumeByNameFromCache(t, vol.Config.Name)
require.NotNil(t, cached)
require.NotNil(t, cached.Config.MoveInfo)
assert.NotNil(t, cached.Config.MoveInfo, "MoveVolume must set MoveInfo on success")
assert.Equal(t, models.VolumeMoveStateMoving, cached.Config.MoveInfo.State)
assert.Equal(t, "pool-target", cached.Pool, "pool should be updated to move target on success")
})
}
Expand Down Expand Up @@ -23360,7 +23410,7 @@ func TestConcurrentTridentOrchestrator_UnstageVolumeMove(t *testing.T) {
"backend unstage failure must leave MoveInfo untouched")
})

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

vol.Config.MoveInfo = newConcMoveInfo(vol.Config.Name, models.VolumeMoveStateControllerUnstaging)
Expand All @@ -23376,7 +23426,8 @@ func TestConcurrentTridentOrchestrator_UnstageVolumeMove(t *testing.T) {

cached := getVolumeByNameFromCache(t, vol.Config.Name)
require.NotNil(t, cached)
assert.NotNil(t, cached.Config.MoveInfo, "UnstageVolumeMove must not clear MoveInfo")
assert.Nil(t, cached.Config.MoveInfo,
"successful unstage must clear MoveInfo")
})
}

Expand Down
Loading