Skip to content

Commit f5729d0

Browse files
alloydsaalloydsa
authored
Return error when trying to clone from different storage class driver
1 parent b806fd6 commit f5729d0

File tree

6 files changed

+195
-9
lines changed

6 files changed

+195
-9
lines changed

‎core/concurrent_core.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2183,8 +2183,9 @@ func (o *ConcurrentTridentOrchestrator) cloneVolume(
21832183
return nil, errors.NotFoundError("backend for source volume %s not found", volConfig.CloneSourceVolume)
21842184
}
21852185

2186-
if volConfig.StorageClass != sourceVolume.Config.StorageClass {
2187-
Logc(ctx).Errorf("Clone volume %s from source volume %s with different storage classes is not recommended.",
2186+
// Check if the storage class of source and clone volume is different, only if the orchestrator is not in Docker plugin mode. In Docker plugin mode, the storage class of source and clone volume will be different at times.
2187+
if !isDockerPluginMode() && volConfig.StorageClass != sourceVolume.Config.StorageClass {
2188+
return nil, errors.MismatchedStorageClassError("clone volume %s from source volume %s with different storage classes is not allowed",
21882189
volConfig.Name, volConfig.CloneSourceVolume)
21892190
}
21902191

@@ -2372,8 +2373,9 @@ func (o *ConcurrentTridentOrchestrator) cloneVolumeRetry(
23722373

23732374
Logc(ctx).WithFields(logFields).Debug("Cloning volume.")
23742375

2375-
if cloneConfig.StorageClass != sourceVolConfig.StorageClass {
2376-
Logc(ctx).Errorf("Clone volume %s from source volume %s with different storage classes is not recommended.",
2376+
// Check if the storage class of source and clone volume is different, only if the orchestrator is not in Docker plugin mode. In Docker plugin mode, the storage class of source and clone volume will be different at times.
2377+
if !isDockerPluginMode() && cloneConfig.StorageClass != sourceVolConfig.StorageClass {
2378+
return nil, errors.MismatchedStorageClassError("clone volume %s from source volume %s with different storage classes is not allowed",
23772379
cloneConfig.Name, cloneConfig.CloneSourceVolume)
23782380
}
23792381

‎core/concurrent_core_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4287,6 +4287,50 @@ func TestCloneVolumeConcurrentCore(t *testing.T) {
42874287
assert.Nil(t, volume)
42884288
},
42894289
},
4290+
{
4291+
name: "CloneVolumeFromDifferentStorageClassFailed",
4292+
bootstrapErr: nil,
4293+
setupMocks: func(mockCtrl *gomock.Controller, mockStoreClient *mockpersistentstore.MockStoreClient, o *ConcurrentTridentOrchestrator) {
4294+
mockBackend := getMockBackend(mockCtrl, "testBackend", "backend-uuid")
4295+
4296+
sourceVolumeDifferentStorageClass := &storage.Volume{
4297+
Config: &storage.VolumeConfig{
4298+
InternalName: "sourceVolume",
4299+
Name: "sourceVolume",
4300+
Size: "1073741824",
4301+
VolumeMode: config.Filesystem,
4302+
StorageClass: "test-storage-class",
4303+
},
4304+
BackendUUID: "backend-uuid",
4305+
}
4306+
4307+
addBackendsToCache(t, mockBackend)
4308+
addVolumesToCache(t, sourceVolumeDifferentStorageClass)
4309+
addSnapshotsToCache(t, sourceSnapshot)
4310+
4311+
mockStoreClient.EXPECT().GetVolumeTransaction(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
4312+
mockStoreClient.EXPECT().AddVolumeTransaction(gomock.Any(), gomock.Any()).Return(nil).Times(1)
4313+
mockStoreClient.EXPECT().DeleteVolumeTransaction(gomock.Any(), gomock.Any()).Return(nil).Times(1)
4314+
},
4315+
volumeConfig: &storage.VolumeConfig{
4316+
InternalName: "cloneVolume",
4317+
Name: "cloneVolume",
4318+
Size: "1073741824",
4319+
VolumeMode: config.Filesystem,
4320+
CloneSourceVolume: "sourceVolume",
4321+
CloneSourceVolumeInternal: "sourceVolume",
4322+
StorageClass: "test-different-storage-class",
4323+
},
4324+
verifyError: func(err error) {
4325+
assert.True(t, errors.IsMismatchedStorageClassError(err))
4326+
},
4327+
verifyResult: func(result *storage.VolumeExternal) {
4328+
require.Nil(t, result)
4329+
// Additionally verify the volume is not added to the cache
4330+
volume := getVolumeByNameFromCache(t, "cloneVolume")
4331+
assert.Nil(t, volume)
4332+
},
4333+
},
42904334
}
42914335

42924336
for _, tt := range tests {

‎core/orchestrator_core.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2282,9 +2282,10 @@ func (o *TridentOrchestrator) cloneVolumeInitial(
22822282
return nil, errors.NotFoundError("source volume not found: %s", volumeConfig.CloneSourceVolume)
22832283
}
22842284

2285-
if volumeConfig.StorageClass != sourceVolume.Config.StorageClass {
2286-
Logc(ctx).Errorf("Clone volume %s from source volume %s with different storage classes is not recommended.",
2287-
volumeConfig.Name, volumeConfig.CloneSourceVolume)
2285+
// Check if the storage class of source and clone volume is different, only if the orchestrator is not in Docker plugin mode. In Docker plugin mode, the storage class of source and clone volume will be different.
2286+
if !isDockerPluginMode() && volumeConfig.StorageClass != sourceVolume.Config.StorageClass {
2287+
return nil, errors.MismatchedStorageClassError("clone volume %s from source volume %s with different storage classes is not allowed", volumeConfig.Name,
2288+
volumeConfig.CloneSourceVolume)
22882289
}
22892290

22902291
Logc(ctx).WithFields(LogFields{
@@ -2522,8 +2523,9 @@ func (o *TridentOrchestrator) cloneVolumeRetry(
25222523
return nil, errors.NotFoundError("source volume not found: %s", cloneConfig.CloneSourceVolume)
25232524
}
25242525

2525-
if cloneConfig.StorageClass != sourceVolume.Config.StorageClass {
2526-
Logc(ctx).Errorf("Clone volume %s from source volume %s with different storage classes is not recommended.",
2526+
// Check if the storage class of source and clone volume is different, only if the orchestrator is not in Docker plugin mode. In Docker plugin mode, the storage class of source and clone volume will be different at times.
2527+
if !isDockerPluginMode() && cloneConfig.StorageClass != sourceVolume.Config.StorageClass {
2528+
return nil, errors.MismatchedStorageClassError("clone volume %s from source volume %s with different storage classes is not allowed",
25272529
cloneConfig.Name, cloneConfig.CloneSourceVolume)
25282530
}
25292531

‎core/orchestrator_core_test.go‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1527,6 +1527,99 @@ func TestCloneVolumes(t *testing.T) {
15271527
cleanup(t, orchestrator)
15281528
}
15291529

1530+
func TestCloneVolumeWithMismatchedStorageClass(t *testing.T) {
1531+
ctx := context.Background()
1532+
mockPools := tu.GetFakePools()
1533+
orchestrator := getOrchestrator(t, false)
1534+
1535+
// Add backends
1536+
backendConfig, err := fakedriver.NewFakeStorageDriverConfigJSON(
1537+
"fast-backend",
1538+
config.File,
1539+
map[string]*fake.StoragePool{
1540+
tu.FastSmall: mockPools[tu.FastSmall],
1541+
},
1542+
[]fake.Volume{},
1543+
)
1544+
if err != nil {
1545+
t.Fatalf("Unable to generate backend config JSON: %v", err)
1546+
}
1547+
_, err = orchestrator.AddBackend(ctx, backendConfig, "")
1548+
if err != nil {
1549+
t.Fatalf("Unable to add backend: %v", err)
1550+
}
1551+
1552+
// Add storage classes
1553+
storageClasses := []storageClassTest{
1554+
{
1555+
config: &storageclass.Config{
1556+
Name: "fast",
1557+
Attributes: map[string]sa.Request{
1558+
sa.IOPS: sa.NewIntRequest(2000),
1559+
sa.Snapshots: sa.NewBoolRequest(true),
1560+
sa.ProvisioningType: sa.NewStringRequest("thin"),
1561+
},
1562+
},
1563+
expected: []*tu.PoolMatch{{Backend: "fast-backend", Pool: tu.FastSmall}},
1564+
},
1565+
{
1566+
config: &storageclass.Config{
1567+
Name: "slow",
1568+
Attributes: map[string]sa.Request{
1569+
sa.IOPS: sa.NewIntRequest(40),
1570+
sa.Snapshots: sa.NewBoolRequest(true),
1571+
sa.ProvisioningType: sa.NewStringRequest("thin"),
1572+
},
1573+
},
1574+
expected: []*tu.PoolMatch{},
1575+
},
1576+
}
1577+
for _, sc := range storageClasses {
1578+
if _, err := orchestrator.AddStorageClass(ctx, sc.config); err != nil {
1579+
t.Fatalf("Unable to add storage class %s %v", sc.config.Name, err)
1580+
}
1581+
}
1582+
1583+
// Create source volume
1584+
sourceConfig := tu.GenerateVolumeConfig("source-vol", 1, "fast", config.File)
1585+
_, err = orchestrator.AddVolume(ctx, sourceConfig)
1586+
if err != nil {
1587+
t.Fatalf("Unable to add source volume: %v", err)
1588+
}
1589+
1590+
// Attempt to clone with mismatched storage class
1591+
cloneConfig := &storage.VolumeConfig{
1592+
Name: "clone-vol",
1593+
StorageClass: "slow", // Mismatched storage class
1594+
CloneSourceVolume: "source-vol",
1595+
VolumeMode: "file",
1596+
}
1597+
_, err = orchestrator.CloneVolume(ctx, cloneConfig)
1598+
1599+
// Verify error
1600+
if err == nil {
1601+
t.Error("Expected error when cloning with mismatched storage class, but got none")
1602+
} else if !errors.IsMismatchedStorageClassError(err) {
1603+
t.Errorf("Expected CloneStorageClassNotMatchError, got: %v", err)
1604+
}
1605+
1606+
// Verify clone was not created
1607+
orchestrator.mutex.Lock()
1608+
_, found := orchestrator.volumes["clone-vol"]
1609+
if found {
1610+
t.Error("Clone volume was created despite mismatched storage class")
1611+
}
1612+
orchestrator.mutex.Unlock()
1613+
1614+
// Verify source volume still exists
1615+
_, err = orchestrator.storeClient.GetVolume(ctx, "source-vol")
1616+
if err != nil {
1617+
t.Errorf("Source volume not found after failed clone attempt: %v", err)
1618+
}
1619+
1620+
cleanup(t, orchestrator)
1621+
}
1622+
15301623
func addBackend(
15311624
t *testing.T, orchestrator *TridentOrchestrator, backendName string, backendProtocol config.Protocol,
15321625
) {

‎utils/errors/errors.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -666,6 +666,34 @@ func IsInvalidInputError(err error) bool {
666666
return ok
667667
}
668668

669+
// ///////////////////////////////////////////////////////////////////////////
670+
// mismatchedStorageClassError
671+
// ///////////////////////////////////////////////////////////////////////////
672+
673+
type mismatchedStorageClassError struct {
674+
message string
675+
}
676+
677+
func (e *mismatchedStorageClassError) Error() string {
678+
return e.message
679+
}
680+
681+
func MismatchedStorageClassError(message string, a ...any) error {
682+
if len(a) == 0 {
683+
return &mismatchedStorageClassError{message: message}
684+
}
685+
return &mismatchedStorageClassError{message: fmt.Sprintf(fmt.Sprintf("%s", message), a...)}
686+
}
687+
688+
func IsMismatchedStorageClassError(err error) bool {
689+
if err == nil {
690+
return false
691+
}
692+
693+
var errPointer *mismatchedStorageClassError
694+
return errors.As(err, &errPointer)
695+
}
696+
669697
// ///////////////////////////////////////////////////////////////////////////
670698
// unsupportedCapacityRangeError
671699
// ///////////////////////////////////////////////////////////////////////////

‎utils/errors/errors_test.go‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,3 +539,20 @@ func TestConflictError(t *testing.T) {
539539
assert.True(t, IsConflictError(err))
540540
assert.Equal(t, "outer; inner; ", err.Error())
541541
}
542+
543+
func TestMismatchedStorageClassError(t *testing.T) {
544+
err := MismatchedStorageClassError("clone volume test-clone from source volume test-source with different storage classes is not allowed")
545+
assert.True(t, strings.Contains("clone volume test-clone from source volume test-source with different storage classes is not allowed", err.Error()))
546+
547+
err = errors.New("clone volume test-clone from source volume test-source with different storage classes is not allowed")
548+
assert.False(t, IsMismatchedStorageClassError(err))
549+
550+
assert.False(t, IsMismatchedStorageClassError(nil))
551+
552+
err = MismatchedStorageClassError("")
553+
assert.True(t, IsMismatchedStorageClassError(err))
554+
555+
err = MismatchedStorageClassError("clone volume test-clone from source volume test-source with different storage classes is not allowed")
556+
assert.True(t, IsMismatchedStorageClassError(err))
557+
assert.Equal(t, "clone volume test-clone from source volume test-source with different storage classes is not allowed", err.Error())
558+
}

0 commit comments

Comments
 (0)