Feature/fblock 58 - Added volume-modify feature for File/Block. #893

sghatty · 2017-11-15T02:06:35Z

No description provided.

dpickle2 · 2017-11-16T16:26:23Z

SoftLayer/CLI/block/modify.py

+                   '500, 1000, 2000, 4000, 8000, 12000] '
+                   'Minimum: [the size of the origin volume] '
+                   'Maximum: [the minimum of 12000 GB or '
+                   '10*(origin volume size)]')


This 10x limit will only apply to volumes created before volume modification is released, so this comment may need to be adjusted to mention that. On the other hand, we don't want the help output to grow too large, so I wonder if it would be better to leave out the max here and rely on an error message if the size is too large.

For the 10x size limit I would recommend removing it from the help message and relying on a well worded exception. Does the SLAPI give a good exception there or do we need to make our own?

dpickle2 · 2017-11-16T19:29:31Z

SoftLayer/CLI/file/modify.py

+                   '500, 1000, 2000, 4000, 8000, 12000] '
+                   'Minimum: [the size of the origin volume] '
+                   'Maximum: [the minimum of 12000 GB or '
+                   '10*(origin volume size)]')


The "10x limit" part isn't applicable for File volumes.

coveralls · 2017-11-17T19:22:09Z

Coverage decreased (-0.8%) to 84.774% when pulling 8e84d8c on sghatty:feature/FBLOCK-58 into dc97e7e on softlayer:master.

allmightyspiff · 2017-11-17T20:56:12Z

the CI build is still complaining about the following.

SoftLayer/managers/storage_utils.py:1083:9: E265 block comment should start with '# '
SoftLayer/managers/storage_utils.py:1183:1: E302 expected 2 blank lines, found 1

Also, please try to improve the test coverage as well.

Thanks.

sghatty · 2017-11-18T04:20:06Z

Hi Chris, I fixed those errors, and a couple of test cases. Regards,Surya Ghatty"This too shall pass"________________________________________________________________________________________________________Surya Ghatty | Software Engineer | IBM Cloud Infrastructure Services Development | tel: (507) 316-0559 | ghatty@us.ibm.com ----- Original message -----From: Christopher Gallo <notifications@github.com>To: softlayer/softlayer-python <softlayer-python@noreply.github.com>Cc: sghatty <ghatty@us.ibm.com>, Author <author@noreply.github.com>Subject: Re: [softlayer/softlayer-python] Feature/fblock 58 - Added volume-modify feature for File/Block. (#893)Date: Fri, Nov 17, 2017 2:56 PM the CI build is still complaining about the following. SoftLayer/managers/storage_utils.py:1083:9: E265 block comment should start with '# 'SoftLayer/managers/storage_utils.py:1183:1: E302 expected 2 blank lines, found 1 Also, please try to improve the test coverage as well. Thanks. —You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or mute the thread.

dpickle2 · 2017-11-20T22:45:08Z

SoftLayer/CLI/file/modify.py

+              help='Endurance Storage Tier (IOPS per GB) [only used for '
+                   'endurance volumes] ***If no tier is specified, the original tier '
+                   'of the volume will be used.***\n'
+                   'Requirements: [IOPS/GB for the volume is 0.25, '


This is minor, but for consistency, I think it would be helpful to insert "If original" before "IOPS/GB" here.

dpickle2 · 2017-11-20T22:48:05Z

SoftLayer/CLI/routes.py

    ('block:volume-count', 'SoftLayer.CLI.block.count:cli'),
    ('block:volume-detail', 'SoftLayer.CLI.block.detail:cli'),
    ('block:volume-duplicate', 'SoftLayer.CLI.block.duplicate:cli'),
+    ('block:volume-modify', 'SoftLayer.CLI.block.modify:cli'),


This is minor, but it would be ideal if this line was moved just below the line for 'volume-list', so that the commands remain in alphabetical order.

dpickle2 · 2017-11-20T22:48:07Z

SoftLayer/CLI/routes.py

    ('file:volume-count', 'SoftLayer.CLI.file.count:cli'),
    ('file:volume-detail', 'SoftLayer.CLI.file.detail:cli'),
    ('file:volume-duplicate', 'SoftLayer.CLI.file.duplicate:cli'),
+    ('file:volume-modify', 'SoftLayer.CLI.file.modify:cli'),


This is minor, but it would be ideal if this line was moved just below the line for 'volume-list', so that the commands remain in alphabetical order.

dpickle2 · 2017-11-20T22:57:29Z

SoftLayer/managers/file.py

+
+        file_mask = 'id,billingItem[location,hourlyFlag],snapshotCapacityGb,'\
+                    'storageType[keyName],capacityGb,originalVolumeSize,'\
+                    'provisionedIops,storageTierLevel,osType[keyName],'\


osType is not relevant for File volumes, so this is not needed.

dpickle2 · 2017-11-20T23:06:12Z

SoftLayer/managers/storage_utils.py


+def prepare_modify_order_object(manager, origin_volume, new_iops, new_tier,
+                                new_size, volume_type):
+    """Prepare the duplicate order to submit to SoftLayer_Product::placeOrder()


This should be something like "modification order" instead of "duplicate order".

dpickle2 · 2017-11-20T23:35:15Z

SoftLayer/managers/storage_utils.py

+        'packageId': package['id'],
+        'prices': prices,
+        'volume': origin_volume,
+        #'useHourlyPricing' : 'true',


I don't think this line is needed anymore, since the hourly vs. monthly billing will be handled automatically by the API now.

allmightyspiff

Only passing an ID should likely throw an error and print the help message

 ./slcli block volume-modify 26140889
SoftLayerAPIError(SoftLayer_Exception_Public): Price does not have an id.

Lets figure out a way to rewrite this error to indicate a proper size wasn't selected. IDEALLY the api would tell us that, but if thats not possible maybe something in the manager would be helpful.
Is there a good way to get a list of acceptable sizes from the API so that they don't have to be hard coded?

./slcli block volume-modify 34034881 -c 200
SoftLayerAPIError(SoftLayer_Exception_Public): Price does not have an id.

This should work.

./slcli block volume-modify 34034881 -c 250
SoftLayerAPIError(SoftLayer_Exception_Public): Price does not have an id.

This should also work.... am I doing something wrong?

./slcli block volume-modify 34034881 -c 250 -t 2
(this took a long time, maybe add a message before we place an api call)
SoftLayerAPIError(SoftLayer_Exception_Public): Price does not have an id.

Changes needed

Please make sure the commands do work with the given list of arguments I tried.
Please use a line width of 120. I know most of the codebase uses 80, but I'm trying to get that updated as we add new code
the _validate functions you added seem to be duplicate of code that already exists, which I don't really like in the first place. So unless you have a good reason for adding new functions please remove them and use the ones that already exist.
if 'ENDURANCE' in volume_storage_type: is a much better way of testing storage types than the 4 explicit key names. unless there is a good reason for testing the 4 explicitly.
There is a LOT of copy/pasted code it seems coming from the order duplicate methods into this modify method. I'd really like to see these condensed as much as possible.

allmightyspiff · 2017-11-20T22:57:43Z

SoftLayer/CLI/block/modify.py

+                   '500, 1000, 2000, 4000, 8000, 12000] '
+                   'Minimum: [the size of the origin volume] '
+                   'Maximum: [the minimum of 12000 GB or '
+                   '10*(origin volume size)]')


For the 10x size limit I would recommend removing it from the help message and relying on a well worded exception. Does the SLAPI give a good exception there or do we need to make our own?

dpickle2

For the size/performance validation helper functions in storage_utils.py, sort of along the same lines with what Chris mentioned, I think we can mostly get rid of these. I'm about to make an update to the similar helper functions for volume duplication as well. For both, I think we can get rid of most of this validation logic and just rely on the API to return an error when sizes/performance values are invalid (the API is validating all of these things anyway). This will likely reduce the chance that we need to update things as often any time the something changes with the API.

dpickle2 · 2017-11-21T16:48:02Z

@allmightyspiff regarding the commands you were trying to run above, were you pointing to a current production API endpoint? The API changes are still in development/QE, so you would need to use a custom endpoint for these commands to work properly.
Also, regarding item 3 in the changes you requested, I apologize that the helper functions that these were based on were not preferable as is. Had I known that several months ago when they were written, I would have been happy to modify them back then. If I reduce or eliminate these in the way I mentioned in my review comment above, will that likely bring them into an acceptable state, or is further action also desired? Please let me know, and feel free to send me a direct message through some other medium if that is easier. Thank you!

coveralls · 2017-11-21T20:13:36Z

Coverage decreased (-0.8%) to 84.78% when pulling f38259a on sghatty:feature/FBLOCK-58 into dc97e7e on softlayer:master.

allmightyspiff · 2017-11-22T16:18:47Z

@dpickle2 yeah removing those validation functions would solve a lot of my complaints about the pull request. Once that is done I'll test again against the the testing url.

coveralls · 2017-11-23T16:54:46Z

Coverage decreased (-0.8%) to 84.772% when pulling 3002c46 on sghatty:feature/FBLOCK-58 into dc97e7e on softlayer:master.

coveralls · 2017-11-27T17:10:13Z

Coverage decreased (-1.1%) to 84.505% when pulling 0acc6e2 on sghatty:feature/FBLOCK-58 into dc97e7e on softlayer:master.

coveralls · 2017-12-04T22:02:08Z

Coverage increased (+0.2%) to 85.79% when pulling 5eaa9a2 on sghatty:feature/FBLOCK-58 into b060f91 on softlayer:master.

coveralls · 2017-12-04T22:10:11Z

Coverage increased (+0.2%) to 85.79% when pulling 20f2644 on sghatty:feature/FBLOCK-58 into b060f91 on softlayer:master.

allmightyspiff

looks good.

allmightyspiff · 2017-11-20T23:40:23Z

SoftLayer/CLI/block/modify.py

+        for item in order['placedOrder']['items']:
+            click.echo(" > %s" % item['description'])
+    else:
+        click.echo("Order could not be placed! Please verify your options " +


The line width for this project is 120, feel free to use it so you don't have to split strings needlessly.

allmightyspiff · 2017-11-20T23:42:43Z

SoftLayer/managers/block.py

+        :return: Returns a SoftLayer_Container_Product_Order_Receipt
+        """
+
+        block_mask = 'id,billingItem[location,hourlyFlag],snapshotCapacityGb,'\


I prefer masks to be broken out a bit so they are more readable at a glance. Like in block/get_block_volume_details

allmightyspiff · 2017-11-20T23:42:58Z

SoftLayer/managers/file.py

+        :return: Returns a SoftLayer_Container_Product_Order_Receipt
+        """
+
+        file_mask = 'id,billingItem[location,hourlyFlag],snapshotCapacityGb,'\


break out this object mask too

allmightyspiff · 2017-11-20T23:46:01Z

SoftLayer/managers/storage_utils.py

+        'packageId': package['id'],
+        'prices': prices,
+        'volume': origin_volume,
+        #'useHourlyPricing' : 'true',


why is this commented out?

allmightyspiff · 2017-11-20T23:53:45Z

SoftLayer/managers/storage_utils.py

                "IOPS/GB. %s IOPS/GB requested." % duplicate_tier)
    return duplicate_tier

+def _validate_new_size(origin_volume, new_volume_size,


does the API not give a decent error if you give it a bad size? Same with the other 2 _validate fuctions.

I'd prefer this sort of validation be done by the API

allmightyspiff · 2017-11-21T00:01:18Z

SoftLayer/managers/storage_utils.py

                "IOPS/GB. %s IOPS/GB requested." % duplicate_tier)
    return duplicate_tier

+def _validate_new_size(origin_volume, new_volume_size,


How are these 3 functions different than the _validate_dupl functions?

coveralls · 2017-12-05T19:50:41Z

Coverage increased (+0.2%) to 85.793% when pulling a10f5a8 on sghatty:feature/FBLOCK-58 into b060f91 on softlayer:master.

Some certain types of transactions which are currently returned in a volume's activeTransactions contain transactionStatus sub-properties of inconsistent types. There is a plan to fix the core issue in the backend, but until that fix is added, the changes in this commit should prevent the volume-detail commands from resulting in an error if any invalid transactionStatus objects are returned.

coveralls · 2017-12-05T23:10:35Z

Coverage increased (+0.2%) to 85.793% when pulling 74830b5 on sghatty:feature/FBLOCK-58 into b060f91 on softlayer:master.

allmightyspiff · 2017-12-07T18:38:30Z

$ ./slcli block volume-modify 34034881 -c 250
Order #21207661 placed successfully!
 > Storage as a Service
 > 250 GBs
 > 2 IOPS per GB

dpickle2 self-requested a review November 15, 2017 16:49

dpickle2 reviewed Nov 16, 2017

View reviewed changes

dpickle2 reviewed Nov 20, 2017

View reviewed changes

allmightyspiff requested changes Nov 20, 2017

View reviewed changes

dpickle2 reviewed Nov 21, 2017

View reviewed changes

sghatty added 3 commits December 1, 2017 16:01

Added volume-modify feature to file/block.

d2eb29a

Updated volume details remote for modification

26977f2

Address pull request feedback; improve code style/formatting

ae30c66

dpickle2 force-pushed the feature/FBLOCK-58 branch from 0acc6e2 to 5eaa9a2 Compare December 4, 2017 21:59

David Pickle added 2 commits December 4, 2017 16:03

Address PR feedback; remove validation which duplicates API

745867a

Add unit tests for volume modification commands/functions

20f2644

dpickle2 force-pushed the feature/FBLOCK-58 branch from 5eaa9a2 to 20f2644 Compare December 4, 2017 22:05

allmightyspiff approved these changes Dec 5, 2017

View reviewed changes

Update formatting of object masks in volume modification functions

a10f5a8

allmightyspiff merged commit 363d6ad into softlayer:master Dec 7, 2017

Feature/fblock 58 - Added volume-modify feature for File/Block. #893

Feature/fblock 58 - Added volume-modify feature for File/Block. #893

Uh oh!

Conversation

sghatty commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpickle2 Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 17, 2017

Uh oh!

allmightyspiff commented Nov 17, 2017

Uh oh!

sghatty commented Nov 18, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allmightyspiff left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Changes needed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpickle2 left a comment

Choose a reason for hiding this comment

Uh oh!

dpickle2 commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allmightyspiff commented Nov 22, 2017

Uh oh!

coveralls commented Nov 23, 2017

Uh oh!

coveralls commented Nov 27, 2017

Uh oh!

coveralls commented Dec 4, 2017

Uh oh!

coveralls commented Dec 4, 2017

Uh oh!

allmightyspiff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 5, 2017

Uh oh!

coveralls commented Dec 5, 2017

Uh oh!

allmightyspiff commented Dec 7, 2017

Uh oh!

Uh oh!

dpickle2 Nov 16, 2017 •

edited

Loading

allmightyspiff left a comment •

edited

Loading

dpickle2 commented Nov 21, 2017 •

edited

Loading

coveralls commented Nov 21, 2017 •

edited

Loading