Skip to content

Commit

Permalink
fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@umagnus
umagnus committed Jun 27, 2024
1 parent 9d14c43 commit bd40aff
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 22 deletions.
23 changes: 12 additions & 11 deletions pkg/azuredisk/azure_managedDiskController.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,6 @@ func (c *ManagedDiskController) ResizeDisk(ctx context.Context, diskURI string,

// ModifyDisk: modify disk
func (c *ManagedDiskController) ModifyDisk(ctx context.Context, options *ManagedDiskOptions) error {
var err error
klog.V(4).Infof("azureDisk - modifying managed Name:%s, StorageAccountType:%s, DiskIOPSReadWrite:%s, DiskMBpsReadWrite:%s", options.DiskName, options.StorageAccountType, options.DiskIOPSReadWrite, options.DiskMBpsReadWrite)

rg, subsID, err := getInfoFromDiskURI(options.SourceResourceID)
Expand All @@ -429,6 +428,7 @@ func (c *ManagedDiskController) ModifyDisk(ctx context.Context, options *Managed
return err
}

model := armcompute.DiskUpdate{}
result, err := diskClient.Get(ctx, rg, options.DiskName)
if err != nil {
return err
Expand All @@ -439,8 +439,14 @@ func (c *ManagedDiskController) ModifyDisk(ctx context.Context, options *Managed
}

diskSku := *result.SKU.Name
if options.StorageAccountType != "" {
if options.StorageAccountType != "" && options.StorageAccountType != diskSku {
if diskSku == armcompute.DiskStorageAccountTypesUltraSSDLRS || diskSku == armcompute.DiskStorageAccountTypesPremiumV2LRS || options.StorageAccountType == armcompute.DiskStorageAccountTypesUltraSSDLRS || options.StorageAccountType == armcompute.DiskStorageAccountTypesPremiumV2LRS {
return fmt.Errorf("AzureDisk - StorageAccountType is not allowed to change from or to UltraSSD_LRS or PremiumV2_LRS disk type")
}
diskSku = options.StorageAccountType
model.SKU = &armcompute.DiskSKU{
Name: to.Ptr(diskSku),
}
}

diskProperties := armcompute.DiskUpdateProperties{}
Expand All @@ -463,22 +469,17 @@ func (c *ManagedDiskController) ModifyDisk(ctx context.Context, options *Managed
diskMBpsReadWrite := int64(v)
diskProperties.DiskMBpsReadWrite = pointer.Int64(diskMBpsReadWrite)
}

model.Properties = &diskProperties
} else {
if options.DiskIOPSReadWrite != "" {
return fmt.Errorf("AzureDisk - DiskIOPSReadWrite parameter is only applicable in UltraSSD_LRS disk type")
return fmt.Errorf("AzureDisk - DiskIOPSReadWrite parameter is only applicable in UltraSSD_LRS or PremiumV2_LRS disk type")
}
if options.DiskMBpsReadWrite != "" {
return fmt.Errorf("AzureDisk - DiskMBpsReadWrite parameter is only applicable in UltraSSD_LRS disk type")
return fmt.Errorf("AzureDisk - DiskMBpsReadWrite parameter is only applicable in UltraSSD_LRS or PremiumV2_LRS disk type")
}
}

model := armcompute.DiskUpdate{
SKU: &armcompute.DiskSKU{
Name: to.Ptr(diskSku),
},
Properties: &diskProperties,
}

if _, err := diskClient.Patch(ctx, rg, options.DiskName, model); err != nil {
return err
}
Expand Down
21 changes: 19 additions & 2 deletions pkg/azuredisk/azure_managedDiskController_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,13 @@ func TestModifyDisk(t *testing.T) {
expectedErr bool
expectedErrMsg error
}{
{
desc: "new sku and no error shall be returned if everything is good",
diskName: diskName,
storageAccountType: armcompute.DiskStorageAccountTypesStandardLRS,
existedDisk: &armcompute.Disk{Name: pointer.String(disk1Name), SKU: &armcompute.DiskSKU{Name: &storageAccountTypePremiumLRS}, Properties: &armcompute.DiskProperties{DiskIOPSReadWrite: pointer.Int64(100)}},
expectedErr: false,
},
{
desc: "new diskIOPSReadWrite and no error shall be returned if everything is good",
diskName: diskName,
Expand All @@ -656,6 +663,16 @@ func TestModifyDisk(t *testing.T) {
existedDisk: &armcompute.Disk{Name: pointer.String(disk1Name), SKU: &armcompute.DiskSKU{Name: &storageAccountTypeUltraSSDLRS}, Properties: &armcompute.DiskProperties{DiskIOPSReadWrite: pointer.Int64(100), DiskMBpsReadWrite: pointer.Int64(100)}},
expectedErr: false,
},
{
desc: "change from or to UltraSSD_LRS or PremiumV2_LRS disk type shall fail",
diskName: diskName,
diskIOPSReadWrite: "200",
diskMBpsReadWrite: "200",
storageAccountType: armcompute.DiskStorageAccountTypesUltraSSDLRS,
existedDisk: &armcompute.Disk{Name: pointer.String(disk1Name), SKU: &armcompute.DiskSKU{Name: &storageAccountTypePremiumLRS}, Properties: &armcompute.DiskProperties{DiskIOPSReadWrite: pointer.Int64(100)}},
expectedErr: true,
expectedErrMsg: fmt.Errorf("AzureDisk - StorageAccountType is not allowed to change from or to UltraSSD_LRS or PremiumV2_LRS disk type"),
},
{
desc: "an error shall be returned when disk SKU is nil",
diskName: diskName,
Expand All @@ -672,15 +689,15 @@ func TestModifyDisk(t *testing.T) {
diskIOPSReadWrite: "200",
existedDisk: &armcompute.Disk{Name: pointer.String(disk1Name), SKU: &armcompute.DiskSKU{Name: &storageAccountTypePremiumLRS}, Properties: &armcompute.DiskProperties{DiskIOPSReadWrite: pointer.Int64(100)}},
expectedErr: true,
expectedErrMsg: fmt.Errorf("AzureDisk - DiskIOPSReadWrite parameter is only applicable in UltraSSD_LRS disk type"),
expectedErrMsg: fmt.Errorf("AzureDisk - DiskIOPSReadWrite parameter is only applicable in UltraSSD_LRS or PremiumV2_LRS disk type"),
},
{
desc: "new diskMBpsReadWrite but wrong disk type error shall be returned",
diskName: diskName,
diskMBpsReadWrite: "200",
existedDisk: &armcompute.Disk{Name: pointer.String(disk1Name), SKU: &armcompute.DiskSKU{Name: &storageAccountTypePremiumLRS}, Properties: &armcompute.DiskProperties{DiskMBpsReadWrite: pointer.Int64(100)}},
expectedErr: true,
expectedErrMsg: fmt.Errorf("AzureDisk - DiskMBpsReadWrite parameter is only applicable in UltraSSD_LRS disk type"),
expectedErrMsg: fmt.Errorf("AzureDisk - DiskMBpsReadWrite parameter is only applicable in UltraSSD_LRS or PremiumV2_LRS disk type"),
},
{
desc: "new diskIOPSReadWrite but failed to parse",
Expand Down
8 changes: 2 additions & 6 deletions pkg/azuredisk/controllerserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,11 +412,8 @@ func (d *Driver) ControllerModifyVolume(ctx context.Context, req *csi.Controller
return nil, status.Errorf(codes.InvalidArgument, "Failed parsing disk parameters: %v", err)
}

localCloud := d.cloud
localDiskController := d.diskController

// normalize values
skuName, err := azureutils.NormalizeStorageAccountType(diskParams.AccountType, localCloud.Config.Cloud, localCloud.Config.DisableAzureStackCloud)
skuName, err := azureutils.NormalizeStorageAccountType(diskParams.AccountType, d.cloud.Config.Cloud, d.cloud.Config.DisableAzureStackCloud)
if err != nil {
return nil, status.Error(codes.InvalidArgument, err.Error())
}
Expand Down Expand Up @@ -454,8 +451,7 @@ func (d *Driver) ControllerModifyVolume(ctx context.Context, req *csi.Controller
mc.ObserveOperationWithResult(isOperationSucceeded, consts.VolumeID, diskURI)
}()

err = localDiskController.ModifyDisk(ctx, volumeOptions)
if err != nil {
if err = d.diskController.ModifyDisk(ctx, volumeOptions); err != nil {
if strings.Contains(err.Error(), consts.NotFound) {
return nil, status.Error(codes.NotFound, err.Error())
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/azuredisk/controllerserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ func TestControllerModifyVolume(t *testing.T) {
expectedResp: &csi.ControllerModifyVolumeResponse{},
},
{
desc: "success standard with PremiumV2_LRS",
desc: "fail with storageAccountType is not allowed to change from or to UltraSSD_LRS or PremiumV2_LRS disk type",
req: &csi.ControllerModifyVolumeRequest{
VolumeId: testVolumeID,
MutableParameters: map[string]string{
Expand All @@ -661,8 +661,9 @@ func TestControllerModifyVolume(t *testing.T) {
consts.DiskMBPSReadWriteField: "100",
},
},
oldSKU: &storageAccountTypeUltraSSDLRS,
expectedResp: &csi.ControllerModifyVolumeResponse{},
oldSKU: &storageAccountTypeUltraSSDLRS,
expectedResp: nil,
expectedErrCode: codes.Internal,
},
{
desc: "fail with no volume id",
Expand Down

0 comments on commit bd40aff

Please sign in to comment.