Skip to content

Commit 215469d

Browse files
acortelyouacortelyou
committed
fix: create blob container with correct configuration before copy
1 parent a409eae commit 215469d

File tree

2 files changed

+34
-26
lines changed

2 files changed

+34
-26
lines changed

‎pkg/blob/controllerserver.go

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -353,8 +353,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
353353

354354
var volumeID string
355355
requestName := "controller_create_volume"
356-
if req.GetVolumeContentSource() != nil {
357-
switch req.VolumeContentSource.Type.(type) {
356+
if vs := req.GetVolumeContentSource(); vs != nil {
357+
switch vs.Type.(type) {
358358
case *csi.VolumeContentSource_Snapshot:
359359
requestName = "controller_create_volume_from_snapshot"
360360
case *csi.VolumeContentSource_Volume:
@@ -367,6 +367,13 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
367367
mc.ObserveOperationWithResult(isOperationSucceeded, VolumeID, volumeID)
368368
}()
369369

370+
if vs := req.GetVolumeContentSource(); vs != nil {
371+
switch vs.Type.(type) {
372+
case *csi.VolumeContentSource_Snapshot:
373+
return nil, status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
374+
}
375+
}
376+
370377
var accountKey string
371378
accountName := account
372379
secrets := req.GetSecrets()
@@ -435,6 +442,11 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
435442
setKeyValueInMap(parameters, containerNameField, validContainerName)
436443
}
437444

445+
klog.V(2).Infof("begin to create container(%s) on account(%s) type(%s) subsID(%s) rg(%s) location(%s) size(%d)", validContainerName, accountName, storageAccountType, subsID, resourceGroup, location, requestGiB)
446+
if err := d.CreateBlobContainer(ctx, subsID, resourceGroup, accountName, validContainerName, secrets); err != nil {
447+
return nil, status.Errorf(codes.Internal, "failed to create container(%s) on account(%s) type(%s) rg(%s) location(%s) size(%d), error: %v", validContainerName, accountName, storageAccountType, resourceGroup, location, requestGiB, err)
448+
}
449+
438450
if req.GetVolumeContentSource() != nil {
439451
accountSASToken, authAzcopyEnv, err := d.getAzcopyAuth(ctx, accountName, accountKey, storageEndpointSuffix, accountOptions, secrets, secretName, secretNamespace)
440452
if err != nil {
@@ -443,11 +455,6 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
443455
if err := d.copyVolume(req, accountSASToken, authAzcopyEnv, accountName, validContainerName, storageEndpointSuffix); err != nil {
444456
return nil, err
445457
}
446-
} else {
447-
klog.V(2).Infof("begin to create container(%s) on account(%s) type(%s) subsID(%s) rg(%s) location(%s) size(%d)", validContainerName, accountName, storageAccountType, subsID, resourceGroup, location, requestGiB)
448-
if err := d.CreateBlobContainer(ctx, subsID, resourceGroup, accountName, validContainerName, secrets); err != nil {
449-
return nil, status.Errorf(codes.Internal, "failed to create container(%s) on account(%s) type(%s) rg(%s) location(%s) size(%d), error: %v", validContainerName, accountName, storageAccountType, resourceGroup, location, requestGiB, err)
450-
}
451458
}
452459

453460
if storeAccountKey && len(req.GetSecrets()) == 0 {
@@ -756,18 +763,9 @@ func (d *Driver) DeleteBlobContainer(ctx context.Context, subsID, resourceGroupN
756763
}
757764

758765
// CopyBlobContainer copies a blob container
759-
func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken string, authAzcopyEnv []string, dstAccountName, dstContainerName, storageEndpointSuffix string) error {
760-
var sourceVolumeID string
761-
if req.GetVolumeContentSource() != nil && req.GetVolumeContentSource().GetVolume() != nil {
762-
sourceVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId()
763-
764-
}
765-
_, srcAccountName, srcContainerName, _, _, err := GetContainerInfo(sourceVolumeID) //nolint:dogsled
766-
if err != nil {
767-
return status.Error(codes.NotFound, err.Error())
768-
}
766+
func (d *Driver) copyBlobContainer(accountSasToken string, authAzcopyEnv []string, srcAccountName, srcContainerName, dstAccountName, dstContainerName, storageEndpointSuffix string) error {
769767
if srcAccountName == "" || srcContainerName == "" || dstAccountName == "" || dstContainerName == "" {
770-
return fmt.Errorf("One or more of srcAccountName(%s), srcContainerName(%s), dstAccountName(%s), dstContainerName(%s) are empty", srcAccountName, srcContainerName, dstAccountName, dstContainerName)
768+
return fmt.Errorf("some of srcAccountName(%s), srcContainerName(%s), dstAccountName(%s), dstContainerName(%s) are empty", srcAccountName, srcContainerName, dstAccountName, dstContainerName)
771769
}
772770

773771
srcPath := fmt.Sprintf("https://%s.blob.%s/%s", srcAccountName, storageEndpointSuffix, srcContainerName)
@@ -807,14 +805,22 @@ func (d *Driver) copyBlobContainer(req *csi.CreateVolumeRequest, accountSasToken
807805
return err
808806
}
809807

810-
// copyVolume copies a volume form volume or snapshot, snapshot is not supported now
808+
// copyVolume copies a volume from supported sources
811809
func (d *Driver) copyVolume(req *csi.CreateVolumeRequest, accountSASToken string, authAzcopyEnv []string, dstAccountName, dstContainerName, storageEndpointSuffix string) error {
812810
vs := req.VolumeContentSource
813811
switch vs.Type.(type) {
814812
case *csi.VolumeContentSource_Snapshot:
815813
return status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
816814
case *csi.VolumeContentSource_Volume:
817-
return d.copyBlobContainer(req, accountSASToken, authAzcopyEnv, dstAccountName, dstContainerName, storageEndpointSuffix)
815+
var srcVolumeID string
816+
if vs.GetVolume() != nil {
817+
srcVolumeID = req.GetVolumeContentSource().GetVolume().GetVolumeId()
818+
}
819+
_, srcAccountName, srcContainerName, _, _, err := GetContainerInfo(srcVolumeID) //nolint:dogsled
820+
if err != nil {
821+
return status.Error(codes.NotFound, err.Error())
822+
}
823+
return d.copyBlobContainer(accountSASToken, authAzcopyEnv, srcAccountName, srcContainerName, dstAccountName, dstContainerName, storageEndpointSuffix)
818824
default:
819825
return status.Errorf(codes.InvalidArgument, "%v is not a proper volume source", vs)
820826
}

‎pkg/blob/controllerserver_test.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -793,9 +793,6 @@ func TestCreateVolume(t *testing.T) {
793793
defer ctrl.Finish()
794794

795795
m := util.NewMockEXEC(ctrl)
796-
listStr := "no error"
797-
m.EXPECT().RunCommand(gomock.Any(), gomock.Any()).Return(listStr, nil)
798-
799796
d.azcopy.ExecCmd = m
800797

801798
expectedErr := status.Errorf(codes.InvalidArgument, "copy volume from volumeSnapshot is not supported")
@@ -855,6 +852,11 @@ func TestCreateVolume(t *testing.T) {
855852
}
856853

857854
ctrl := gomock.NewController(t)
855+
clientFactoryMock := mock_azclient.NewMockClientFactory(ctrl)
856+
blobClientMock := mock_blobcontainerclient.NewMockInterface(ctrl)
857+
clientFactoryMock.EXPECT().GetBlobContainerClientForSub(gomock.Any()).Return(blobClientMock, nil)
858+
blobClientMock.EXPECT().CreateContainer(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
859+
d.clientFactory = clientFactoryMock
858860
defer ctrl.Finish()
859861

860862
m := util.NewMockEXEC(ctrl)
@@ -1625,7 +1627,7 @@ func TestCopyVolume(t *testing.T) {
16251627
VolumeContentSource: &volumecontensource,
16261628
}
16271629

1628-
expectedErr := fmt.Errorf("One or more of srcAccountName(srcAccount), srcContainerName(), dstAccountName(dstAccount), dstContainerName(dstContainer) are empty")
1630+
expectedErr := fmt.Errorf("some of srcAccountName(srcAccount), srcContainerName(), dstAccountName(dstAccount), dstContainerName(dstContainer) are empty")
16291631
err := d.copyVolume(req, "", nil, "dstAccount", "dstContainer", "core.windows.net")
16301632
if !reflect.DeepEqual(err, expectedErr) {
16311633
t.Errorf("Unexpected error: %v", err)
@@ -1655,7 +1657,7 @@ func TestCopyVolume(t *testing.T) {
16551657
VolumeContentSource: &volumecontensource,
16561658
}
16571659

1658-
expectedErr := fmt.Errorf("One or more of srcAccountName(unit-test), srcContainerName(), dstAccountName(dstAccount), dstContainerName(dstContainer) are empty")
1660+
expectedErr := fmt.Errorf("some of srcAccountName(unit-test), srcContainerName(), dstAccountName(dstAccount), dstContainerName(dstContainer) are empty")
16591661
err := d.copyVolume(req, "", nil, "dstAccount", "dstContainer", "core.windows.net")
16601662
if !reflect.DeepEqual(err, expectedErr) {
16611663
t.Errorf("Unexpected error: %v", err)
@@ -1685,7 +1687,7 @@ func TestCopyVolume(t *testing.T) {
16851687
VolumeContentSource: &volumecontensource,
16861688
}
16871689

1688-
expectedErr := fmt.Errorf("One or more of srcAccountName(f5713de20cde511e8ba4900), srcContainerName(fileshare), dstAccountName(), dstContainerName() are empty")
1690+
expectedErr := fmt.Errorf("some of srcAccountName(f5713de20cde511e8ba4900), srcContainerName(fileshare), dstAccountName(), dstContainerName() are empty")
16891691
err := d.copyVolume(req, "", nil, "", "", "core.windows.net")
16901692
if !reflect.DeepEqual(err, expectedErr) {
16911693
t.Errorf("Unexpected error: %v", err)

0 commit comments

Comments
 (0)