Add API to update storage config on an instance #27

harsh-px · 2019-08-20T22:02:00Z

Currently has openstorage vendored in from libopenstorage/openstorage#1200

will update to master once merged.

Signed-off-by: Harsh Desai [email protected]

codecov-io · 2019-08-20T22:14:14Z

Codecov Report

Merging #27 into master will increase coverage by 0.48%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #27      +/-   ##
=========================================
+ Coverage    2.83%   3.31%   +0.48%     
=========================================
  Files          10      11       +1     
  Lines        2470    2534      +64     
=========================================
+ Hits           70      84      +14     
- Misses       2396    2446      +50     
  Partials        4       4

Impacted Files	Coverage Δ
vsphere/storagemanager/vsphere.go	`100% <ø> (ø)`
vsphere/vsphere.go	`0% <ø> (ø)`	⬆️
azure/storagemanager/azure.go	`100% <100%> (ø)`	⬆️
gce/gce.go	`1.23% <0%> (ø)`	⬆️
azure/azure.go	`0.72% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c61f934...e7ee52c. Read the comment docs.

adityadani · 2019-08-20T22:36:27Z

Can you separate out the vendor changes into a separate commit ?

harsh-px · 2019-08-20T22:57:09Z

Can you separate out the vendor changes into a separate commit ?

Done

harsh-px · 2019-08-22T05:11:54Z

Added support for update storage config on azure and updated the azure UTs.

Also made a change so that the expected response will match user requested IOPS more closely.

adityadani

only looked at the storage distribution changes.
Will check the vsphere changes shortly

cloud_storage_management.go

pkg/storagedistribution/storagedistribution.go

cloud_storage_management.go

adityadani · 2019-09-05T18:33:09Z

pkg/storagedistribution/storagedistribution.go

+		IOPS:      row.IOPS,
+	}
+
+	if requiredDriveSize > 0 {


nit: this is just my preference.

We should split this function into two functions.

getStoragePoolForRequestedDriveSize

getStoragePool

pkg/storagedistribution/storagedistribution.go

adityadani

vsphere changes look good.
I still have some questions on the storage distribution changes.

adityadani · 2019-09-10T17:42:12Z

cloud_storage_management.go

+	DesiredCapacity uint64 `json:"new_capacity" yaml:"new_capacity"`
+	// ResizeOperationType is the operation user wants for the storage resize on the node
+	ResizeOperationType api.SdkStoragePool_ResizeOperationType
+	// CurrentDriveCount is the current number of drives in the storage pool


nit: this is the current number of drives on the node.

may be we need two counts. One for the StoragePool we are updating and one for total no. of drives on the node.

adityadani · 2019-09-10T17:47:35Z

pkg/storagedistribution/storagedistribution.go

+		filteredRows = utils.SortByIOPS(filteredRows)
+		// Filter out rows which have lower IOPS than required
+		var index int
+		for index = 0; index < len(filteredRows); index++ {


this for is not needed as requiredIOPS is 0 in this else condition.

adityadani · 2019-09-10T17:48:18Z

pkg/storagedistribution/storagedistribution.go

+		filteredRows = utils.SortByPriority(filteredRows)
+	}
+
+	if len(requiredDriveType) > 0 {


remove this duplicate if.

adityadani · 2019-09-10T18:12:55Z

pkg/storagedistribution/storagedistribution.go

+			}
+
+			// additional check so that we are not overprovisioning
+			for instancesPerZone > 1 && instStorage.DriveCapacityGiB*instancesPerZone > minCapacityPerZone {


Do we need this?
Can we just check if the cluster capacity is less than maxCapacityPerZone otherwise the outer for loop already reduces the instancesPerZone and recalculates.

Also you should consider DriveCount as well instStorage.DriveCapacityGiB*instancesPerZone*instStorage.DriveCount

adityadani · 2019-09-10T18:43:42Z

pkg/storagedistribution/storagedistribution.go

+			}
+
+		} else {
+			logrus.Debugf("required capacity: %d GiB is lower than row's min size: %d GiB",


why not try the next candidate row / reduce the instances per zone and retry the same row?

adityadani · 2019-09-10T18:52:18Z

pkg/storagedistribution/storagedistribution.go

+			remainingCapacity := int(requiredCapacity)
+			for remainingCapacity > 0 {
+				remainingCapacity -= int(requiredDriveSize)
+				instStorage.DriveCount++


Check if instStorage.DriveCount < row.InstanceMaxDrives

adityadani · 2019-09-10T18:55:59Z

pkg/storagedistribution/storagedistribution.go

+			// check in matrix if this drive can be resized to newMinCapacityPerDrive
+			newMinCapacityPerDrive := request.CurrentDriveSize + newMinDeltaCapacityPerDrive
+			logrus.Debugf("need to resize drive to atleast: %d GiB", newMinCapacityPerDrive)
+			instStorage, err := instanceStorageForRow(row, newMinCapacityPerDrive, 0)


instanceStorageForRow also increments the DriveCount, but this switch case only wants to resize the disk.
Probably we don't need to check the matrix, but just find the row for the current instStorage and check if resizing the drives is within row.MaxSize.

adityadani · 2019-09-10T18:58:09Z

vsphere/vsphere.go

+	newSizeInKiB := int64(newSizeInGiB) * 1024 * 1024
+	if editDisk.CapacityInKB >= newSizeInKiB {
+		return uint64(editDisk.CapacityInKB / (1024 * 1024)), cloudops.NewStorageError(cloudops.ErrDiskGreaterOrEqualToExpandSize,
+			fmt.Sprintf("disk is already has a size: %d KiB greater than or equal "+


nit: fix error message.

Signed-off-by: Harsh Desai <[email protected]>

- Change update storage API to storage pool - Simplify logic to calculate update config - misc renames Signed-off-by: Harsh Desai <[email protected]>

Signed-off-by: Harsh Desai <[email protected]>

harsh-px requested review from adityadani and jvinod August 20, 2019 22:02

harsh-px self-assigned this Aug 20, 2019

harsh-px force-pushed the update branch from b34eb48 to db39737 Compare August 20, 2019 22:52

harsh-px force-pushed the update branch 2 times, most recently from 21808e8 to ab41a3c Compare August 22, 2019 05:10

adityadani reviewed Sep 5, 2019

View reviewed changes

adityadani reviewed Sep 10, 2019

View reviewed changes

Harsh Desai added 11 commits September 10, 2019 14:07

Add API to update storage config on an instance

b35d761

Signed-off-by: Harsh Desai <[email protected]>

vendoring changes

672e18d

Signed-off-by: Harsh Desai <[email protected]>

Add vsphere resize

217608a

Signed-off-by: Harsh Desai <[email protected]>

fix golink

0932952

Signed-off-by: Harsh Desai <[email protected]>

have cloud providers return current size

7fef179

Signed-off-by: Harsh Desai <[email protected]>

return error if delta capacity is 0

c7bf77e

Signed-off-by: Harsh Desai <[email protected]>

vendor update

36f8299

Signed-off-by: Harsh Desai <[email protected]>

update resize operation type enums

e9229d5

Signed-off-by: Harsh Desai <[email protected]>

Review changes

e000dbf

- Change update storage API to storage pool - Simplify logic to calculate update config - misc renames Signed-off-by: Harsh Desai <[email protected]>

reviews part 1

9180511

Signed-off-by: Harsh Desai <[email protected]>

add function comments

8ca79ca

Signed-off-by: Harsh Desai <[email protected]>

harsh-px force-pushed the update branch from 7eca292 to 8ca79ca Compare September 11, 2019 00:30

adityadani mentioned this pull request Sep 11, 2019

Add Expand API support for Vsphere #30

Merged

add vsphere yaml

e7ee52c

Signed-off-by: Harsh Desai <[email protected]>

harsh-px closed this Nov 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API to update storage config on an instance #27

Add API to update storage config on an instance #27

harsh-px commented Aug 20, 2019

codecov-io commented Aug 20, 2019 •

edited

Loading

adityadani commented Aug 20, 2019

harsh-px commented Aug 20, 2019

harsh-px commented Aug 22, 2019

adityadani left a comment

adityadani Sep 5, 2019

adityadani left a comment

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

adityadani Sep 10, 2019

Add API to update storage config on an instance #27

Add API to update storage config on an instance #27

Conversation

harsh-px commented Aug 20, 2019

codecov-io commented Aug 20, 2019 • edited Loading

Codecov Report

adityadani commented Aug 20, 2019

harsh-px commented Aug 20, 2019

harsh-px commented Aug 22, 2019

adityadani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityadani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 20, 2019 •

edited

Loading