Skip to content

Commit 3a7cae7

Browse files
VinayKumarHavanurVinayKumarHavanur
authored
Implementation to restrict snapshot deletion if it is a source for read-only clone
Changes to restrict the deletion of a snapshot which is used as a source for a read-only clone volume.
1 parent fd4540c commit 3a7cae7

2 files changed

Lines changed: 88 additions & 31 deletions

File tree

‎core/orchestrator_core.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4255,6 +4255,14 @@ func (o *TridentOrchestrator) DeleteSnapshot(ctx context.Context, volumeName, sn
42554255
return nil
42564256
}
42574257

4258+
// Check if the snapshot is a source for a read-only volume. If so, return error.
4259+
for _, vol := range o.volumes {
4260+
if vol.Config.ReadOnlyClone && vol.Config.CloneSourceSnapshot == snapshotName {
4261+
return fmt.Errorf("unable to delete snapshot %s as it is a source for read-only clone %s", snapshotName,
4262+
vol.Config.Name)
4263+
}
4264+
}
4265+
42584266
backend, ok := o.backends[volume.BackendUUID]
42594267
if !ok {
42604268
if !snapshot.State.IsMissingBackend() {

‎core/orchestrator_core_test.go‎

Lines changed: 80 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3628,15 +3628,17 @@ func TestUpdateNode_BootstrapError(t *testing.T) {
36283628
expectedError := errors.New("bootstrap_error")
36293629
orchestrator.bootstrapError = expectedError
36303630

3631-
result := orchestrator.UpdateNode(ctx(), "testNode1", &utils.NodePublicationStateFlags{OrchestratorReady: utils.Ptr(false)})
3631+
result := orchestrator.UpdateNode(ctx(), "testNode1",
3632+
&utils.NodePublicationStateFlags{OrchestratorReady: utils.Ptr(false)})
36323633

36333634
assert.Equal(t, expectedError, result, "UpdateNode did not return bootstrap error")
36343635
}
36353636

36363637
func TestUpdateNode_NodeNotFound(t *testing.T) {
36373638
orchestrator := getOrchestrator(t, false)
36383639

3639-
result := orchestrator.UpdateNode(ctx(), "testNode1", &utils.NodePublicationStateFlags{OrchestratorReady: utils.Ptr(false)})
3640+
result := orchestrator.UpdateNode(ctx(), "testNode1",
3641+
&utils.NodePublicationStateFlags{OrchestratorReady: utils.Ptr(false)})
36403642

36413643
assert.True(t, errors.IsNotFoundError(result), "UpdateNode did not fail")
36423644
}
@@ -4929,7 +4931,8 @@ func TestUnpublishVolume(t *testing.T) {
49294931
nodes: map[string]*utils.Node{nodeName: node},
49304932
publications: map[string]map[string]*utils.VolumePublication{volumeName: {nodeName: publication}},
49314933
mocks: func(mockBackend *mockstorage.MockBackend, mockStoreClient *mockpersistentstore.MockStoreClient) {
4932-
mockBackend.EXPECT().UnpublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error"))
4934+
mockBackend.EXPECT().UnpublishVolume(coreCtx, gomock.Any(),
4935+
gomock.Any()).Return(fmt.Errorf("some error"))
49334936
},
49344937
wantErr: assert.Error,
49354938
publicationShouldExist: true,
@@ -4958,7 +4961,8 @@ func TestUnpublishVolume(t *testing.T) {
49584961
mocks: func(mockBackend *mockstorage.MockBackend, mockStoreClient *mockpersistentstore.MockStoreClient) {
49594962
mockBackend.EXPECT().UnpublishVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil)
49604963
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(nil)
4961-
mockStoreClient.EXPECT().DeleteVolumePublication(coreCtx, gomock.Any()).Return(errors.New("failed to delete"))
4964+
mockStoreClient.EXPECT().DeleteVolumePublication(coreCtx,
4965+
gomock.Any()).Return(errors.New("failed to delete"))
49624966
},
49634967
wantErr: assert.Error,
49644968
publicationShouldExist: true,
@@ -5895,7 +5899,8 @@ func TestResizeSubordinateVolume(t *testing.T) {
58955899

58965900
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
58975901
if tt.name == "PersistentStoreUpdateError" {
5898-
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(errors.New("persistent store update failed"))
5902+
mockStoreClient.EXPECT().UpdateVolume(coreCtx,
5903+
gomock.Any()).Return(errors.New("persistent store update failed"))
58995904
} else {
59005905
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(nil).AnyTimes()
59015906
}
@@ -5983,25 +5988,29 @@ func TestResizeVolume(t *testing.T) {
59835988
mockBackend.EXPECT().State().Return(storage.Online).AnyTimes()
59845989
mockBackend.EXPECT().Name().Return("mockBackend").AnyTimes()
59855990
if tt.name == "BackendResizeVolumeError" {
5986-
mockBackend.EXPECT().ResizeVolume(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("unable to resize"))
5991+
mockBackend.EXPECT().ResizeVolume(coreCtx, gomock.Any(),
5992+
gomock.Any()).Return(errors.New("unable to resize"))
59875993
} else {
59885994
mockBackend.EXPECT().ResizeVolume(coreCtx, gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
59895995
}
59905996

59915997
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
59925998
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, nil).AnyTimes()
59935999
if tt.name == "DeleteVolumeTranxError" {
5994-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("delete failed"))
6000+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6001+
gomock.Any()).Return(errors.New("delete failed"))
59956002
} else {
59966003
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(nil).AnyTimes()
59976004
}
59986005
if tt.name == "AddVolumeTransactionError" {
5999-
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to add to transaction"))
6006+
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx,
6007+
gomock.Any()).Return(errors.New("failed to add to transaction"))
60006008
} else {
60016009
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx, gomock.Any()).Return(nil).AnyTimes()
60026010
}
60036011
if tt.name == "PersistentStoreUpdateError" {
6004-
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(errors.New("persistent store update failed"))
6012+
mockStoreClient.EXPECT().UpdateVolume(coreCtx,
6013+
gomock.Any()).Return(errors.New("persistent store update failed"))
60056014
} else {
60066015
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(nil).AnyTimes()
60076016
}
@@ -6056,7 +6065,8 @@ func TestHandleFailedTranxResizeVolume(t *testing.T) {
60566065
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
60576066
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(nil).AnyTimes()
60586067
if tt.name == "VolumeNotPresent" {
6059-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("delete failed"))
6068+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6069+
gomock.Any()).Return(errors.New("delete failed"))
60606070
} else {
60616071
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(nil).AnyTimes()
60626072
}
@@ -6213,9 +6223,11 @@ func TestDeleteVolume(t *testing.T) {
62136223
mockBackend.EXPECT().Name().Return("mockBackend").AnyTimes()
62146224

62156225
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
6216-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete tranx")).AnyTimes()
6226+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6227+
gomock.Any()).Return(errors.New("failed to delete tranx")).AnyTimes()
62176228
if tt.name == "FailedToAddTranx" {
6218-
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, errors.New("falied to get tranx"))
6229+
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil,
6230+
errors.New("falied to get tranx"))
62196231
} else {
62206232
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, nil)
62216233
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx, gomock.Any()).Return(nil)
@@ -6300,7 +6312,8 @@ func TestDeleteVolume(t *testing.T) {
63006312
}
63016313

63026314
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
6303-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete tranx")).AnyTimes()
6315+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6316+
gomock.Any()).Return(errors.New("failed to delete tranx")).AnyTimes()
63046317
if tt.name == "HasSubordinateVolumeError" || tt.name == "DeleteClone" {
63056318
mockStoreClient.EXPECT().UpdateVolume(coreCtx, gomock.Any()).Return(errors.New("error updating volume"))
63066319
} else {
@@ -6312,7 +6325,8 @@ func TestDeleteVolume(t *testing.T) {
63126325
mockStoreClient.EXPECT().DeleteVolume(coreCtx, gomock.Any()).Return(nil).AnyTimes()
63136326
}
63146327
if tt.name == "DeleteBackendError" {
6315-
mockStoreClient.EXPECT().DeleteBackend(coreCtx, gomock.Any()).Return(errors.New("delete backend failed"))
6328+
mockStoreClient.EXPECT().DeleteBackend(coreCtx,
6329+
gomock.Any()).Return(errors.New("delete backend failed"))
63166330
}
63176331

63186332
o := getOrchestrator(t, false)
@@ -6348,7 +6362,8 @@ func TestHandleFailedDeleteVolumeError(t *testing.T) {
63486362

63496363
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
63506364
mockStoreClient.EXPECT().DeleteVolume(coreCtx, gomock.Any()).Return(errors.New("unable to delete volume"))
6351-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("unable to delete transaction"))
6365+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6366+
gomock.Any()).Return(errors.New("unable to delete transaction"))
63526367

63536368
o := getOrchestrator(t, false)
63546369
o.storeClient = mockStoreClient
@@ -6435,30 +6450,36 @@ func TestCreateSnapshotError(t *testing.T) {
64356450
mockBackend.EXPECT().GetDriverName().Return("driver").AnyTimes()
64366451
mockBackend.EXPECT().State().Return(storage.Online).AnyTimes()
64376452
if tt.name == "SnapshotNotPossible" {
6438-
mockBackend.EXPECT().CanSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("cannot take snapshot"))
6453+
mockBackend.EXPECT().CanSnapshot(coreCtx, gomock.Any(),
6454+
gomock.Any()).Return(errors.New("cannot take snapshot"))
64396455
} else {
64406456
mockBackend.EXPECT().CanSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
64416457
}
64426458

64436459
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
64446460
if tt.name == "AddVolumeTranxError" {
6445-
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, errors.New("error getting transaction"))
6461+
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil,
6462+
errors.New("error getting transaction"))
64466463
} else {
64476464
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, nil).AnyTimes()
64486465
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx, gomock.Any()).Return(nil).AnyTimes()
64496466
}
64506467
if tt.name == "MaxLimitError" {
6451-
mockBackend.EXPECT().CreateSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil, errors.MaxLimitReachedError("error"))
6452-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete transaction"))
6468+
mockBackend.EXPECT().CreateSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil,
6469+
errors.MaxLimitReachedError("error"))
6470+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
6471+
gomock.Any()).Return(errors.New("failed to delete transaction"))
64536472
}
64546473
if tt.name == "CreateSnapshotError" {
6455-
mockBackend.EXPECT().CreateSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil, errors.New("failed to create snapshot"))
6474+
mockBackend.EXPECT().CreateSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil,
6475+
errors.New("failed to create snapshot"))
64566476
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(nil)
64576477
}
64586478

64596479
if tt.name == "AddSnapshotError" {
64606480
mockBackend.EXPECT().CreateSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(snap3, nil)
6461-
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("cleanup error"))
6481+
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(),
6482+
gomock.Any()).Return(errors.New("cleanup error"))
64626483
mockStoreClient.EXPECT().AddSnapshot(coreCtx, gomock.Any()).Return(errors.New("failed to add snapshot"))
64636484
}
64646485

@@ -7128,18 +7149,21 @@ func TestDeleteSnapshotError(t *testing.T) {
71287149
mockBackend.EXPECT().GetDriverName().Return("driver").AnyTimes()
71297150
mockBackend.EXPECT().State().Return(storage.Online).AnyTimes()
71307151
if tt.name == "DeleteSnapshotFail" {
7131-
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("delete snapshot failed"))
7152+
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(),
7153+
gomock.Any()).Return(errors.New("delete snapshot failed"))
71327154
} else {
71337155
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
71347156
}
71357157

71367158
mockStoreClient := mockpersistentstore.NewMockStoreClient(mockCtrl)
7137-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete transaction")).AnyTimes()
7159+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
7160+
gomock.Any()).Return(errors.New("failed to delete transaction")).AnyTimes()
71387161
if tt.name == "NilVolumeFound" || tt.name == "NilBackendFound" {
71397162
mockStoreClient.EXPECT().DeleteSnapshot(coreCtx, gomock.Any()).Return(errors.New("delete failed"))
71407163
}
71417164
if tt.name == "AddVolumeTranxFail" {
7142-
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, errors.New("failed to get transaction"))
7165+
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil,
7166+
errors.New("failed to get transaction"))
71437167
} else {
71447168
mockStoreClient.EXPECT().GetVolumeTransaction(coreCtx, gomock.Any()).Return(nil, nil).AnyTimes()
71457169
mockStoreClient.EXPECT().AddVolumeTransaction(coreCtx, gomock.Any()).Return(nil).AnyTimes()
@@ -7250,7 +7274,8 @@ func TestHandleFailedSnapshot(t *testing.T) {
72507274
// storage.AddSnapshot switch case tests
72517275
vt.Op = storage.AddSnapshot
72527276

7253-
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("failed to delete snapshot"))
7277+
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(),
7278+
gomock.Any()).Return(errors.New("failed to delete snapshot"))
72547279
mockBackend.EXPECT().Name().Return("abc")
72557280
err := o.handleFailedTransaction(ctx(), vt)
72567281
assert.Error(t, err, "Delete volume error")
@@ -7260,28 +7285,50 @@ func TestHandleFailedSnapshot(t *testing.T) {
72607285
mockBackend2.EXPECT().State().Return(storage.Unknown).AnyTimes()
72617286
mockBackend.EXPECT().State().Return(storage.Online)
72627287
mockBackend.EXPECT().Name().Return("abc")
7263-
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("failed to delete snapshot"))
7288+
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(),
7289+
gomock.Any()).Return(errors.New("failed to delete snapshot"))
72647290
err = o.handleFailedTransaction(ctx(), vt)
72657291
assert.Error(t, err, "Delete snapshot error")
72667292

72677293
delete(o.backends, "xyz")
72687294
mockBackend.EXPECT().State().Return(storage.Online)
72697295
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(nil)
7270-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete transaction"))
7296+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
7297+
gomock.Any()).Return(errors.New("failed to delete transaction"))
72717298
err = o.handleFailedTransaction(ctx(), vt)
72727299
assert.Error(t, err, "Delete volume transaction error")
72737300

72747301
// storage.DeleteSnapshot switch case tests
72757302
vt.Op = storage.DeleteSnapshot
72767303

72777304
o.snapshots[snapID] = &storage.Snapshot{Config: snapConfig}
7278-
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(), gomock.Any()).Return(errors.New("failed to delete snapshot"))
7305+
mockBackend.EXPECT().DeleteSnapshot(coreCtx, gomock.Any(),
7306+
gomock.Any()).Return(errors.New("failed to delete snapshot"))
72797307
mockBackend.EXPECT().Name().Return("abc")
7280-
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx, gomock.Any()).Return(errors.New("failed to delete transaction"))
7308+
mockStoreClient.EXPECT().DeleteVolumeTransaction(coreCtx,
7309+
gomock.Any()).Return(errors.New("failed to delete transaction"))
72817310
err = o.handleFailedTransaction(ctx(), vt)
72827311
assert.Error(t, err, "Delete volume transaction error")
72837312
}
72847313

7314+
func TestDeleteMountedSnapshot(t *testing.T) {
7315+
volName := "vol"
7316+
snapName := "snap"
7317+
snapshot := &storage.Snapshot{Config: &storage.SnapshotConfig{Name: snapName, VolumeName: volName}}
7318+
snapID := storage.MakeSnapshotID(volName, snapName)
7319+
volume := &storage.Volume{
7320+
Config: &storage.VolumeConfig{Name: volName, ReadOnlyClone: true, CloneSourceSnapshot: snapName},
7321+
}
7322+
7323+
o := getOrchestrator(t, false)
7324+
o.volumes[volName] = volume
7325+
o.snapshots[snapID] = snapshot
7326+
7327+
err := o.DeleteSnapshot(ctx(), volName, snapName)
7328+
assert.Error(t, err, "An error is expected")
7329+
assert.Equal(t, err.Error(), "unable to delete snapshot snap as it is a source for read-only clone vol")
7330+
}
7331+
72857332
func TestListLogWorkflows(t *testing.T) {
72867333
o := getOrchestrator(t, false)
72877334

@@ -7495,7 +7542,8 @@ func TestUpdateBackendByBackendUUID(t *testing.T) {
74957542
newBackendConfig: bConfig,
74967543
mocks: func(mockStoreClient *mockpersistentstore.MockStoreClient) {
74977544
mockStoreClient.EXPECT().UpdateBackend(gomock.Any(), gomock.Any()).Return(nil)
7498-
mockStoreClient.EXPECT().UpdateVolume(gomock.Any(), gomock.Any()).Return(errors.New("error updating non-orphan volume"))
7545+
mockStoreClient.EXPECT().UpdateVolume(gomock.Any(),
7546+
gomock.Any()).Return(errors.New("error updating non-orphan volume"))
74997547
},
75007548
wantErr: assert.Error,
75017549
},
@@ -7816,7 +7864,8 @@ func TestReconcileVolumePublications_FailsToAddMissingVolumePublications(t *test
78167864
assert.NoError(t, err)
78177865

78187866
// Set up expected mock calls.
7819-
mockStoreClient.EXPECT().AddVolumePublication(ctx, gomock.Any()).Return(fmt.Errorf("store error")).Times(len(attachedLegacyVolumes) - 1)
7867+
mockStoreClient.EXPECT().AddVolumePublication(ctx,
7868+
gomock.Any()).Return(fmt.Errorf("store error")).Times(len(attachedLegacyVolumes) - 1)
78207869
err = o.ReconcileVolumePublications(ctx, attachedLegacyVolumes)
78217870
assert.Error(t, err)
78227871

0 commit comments

Comments
 (0)