Snapshot notify #1554

hariha74 · 2021-10-21T13:33:35Z

Function change to enable or disable notification for snapshot notification

Changes for snapshot notification enable and disable option from slcli

hariha74 · 2021-10-21T13:34:45Z

allmightyspiff · 2021-10-21T13:38:50Z

API features in this request will be public when FBLOCK-4191 is complete.

Formatting errors mentioned in softlayer#1554 being resolved.

allmightyspiff

Thanks for the pull request, just a few changes needed.

Fix the style checks
CLI commands here should both use the Click flag option so users can just do slcli file set-notify-status --enable
Command storage API calls go in the storage manager
No need for the **kwargs in the managers.
Add unit tests for these functions

allmightyspiff · 2021-10-22T02:35:54Z

SoftLayer/CLI/block/snapshot/get_notify_status.py

+
+    if (enabled == ''):
+        click.echo(
+            'Snapshots space usage threshold warning flag setting is null. Set to default value enable. For volume %s'


Suggested change

'Snapshots space usage threshold warning flag setting is null. Set to default value enable. For volume %s'

'Snapshots space usage threshold warning flag setting is null. Use `slcli block snapshot-set-notification %s --enable` to enable'

allmightyspiff · 2021-10-22T02:37:58Z

SoftLayer/CLI/block/snapshot/set_notify_status.py

+@click.option(
+    '--notification_flag',
+    help=
+    'Enable / disable sending sending notifications for snapshots space usage threshold warning [True|False]',
+    required=True)


Click has a flag option, use that here, and set the default to Enable.

Suggested change

@click.option(

'--notification_flag',

help=

'Enable / disable sending sending notifications for snapshots space usage threshold warning [True|False]',

required=True)

@click.option(

'--enable/disable', required=True, default=True,

help='Enable / disable sending sending notifications for snapshots space usage threshold warning',

)

allmightyspiff · 2021-10-22T02:38:20Z

SoftLayer/CLI/block/snapshot/set_notify_status.py

+@environment.pass_env
+def cli(env, volume_id, notification_flag):
+    """Enables/Disables snapshot space usage threshold warning for a given volume"""
+    if (notification_flag not in ['True', 'False']):


This wont be needed when you switch to the click flag

allmightyspiff · 2021-10-22T02:39:24Z

SoftLayer/CLI/file/snapshot/set_notify_status.py

+@click.option(
+    '--notification_flag',
+    help=
+    'Enable / disable sending sending notifications for snapshots space usage threshold warning [True|False]',
+    required=True)


Click has a flag option, use that here, and set the default to Enable.

Suggested change

@click.option(

'--notification_flag',

help=

'Enable / disable sending sending notifications for snapshots space usage threshold warning [True|False]',

required=True)

@click.option(

'--enable/disable', required=True, default=True,

help='Enable / disable sending sending notifications for snapshots space usage threshold warning',

)

allmightyspiff · 2021-10-22T02:41:47Z

SoftLayer/managers/block.py

@@ -102,6 +102,25 @@ def get_block_volume_snapshot_list(self, volume_id, **kwargs):
        """
        return self.get_volume_snapshot_list(volume_id, **kwargs)

+    def set_block_volume_snapshot_notification(self, volume_id, notification_flag, **kwargs):


Add this to the storage manager, not the block/file managers since the code is exactly the same. The block/file managers inherit from the storage manager, so you shouldn't even need to edit your cli code.

Don't pass **kwargs into the API, and likely don't even need to accept them in this function unless these methods take in objectMasks or objectFilters, which I'm guessing they wont

allmightyspiff · 2021-10-22T02:42:02Z

SoftLayer/managers/file.py

@@ -129,7 +129,24 @@ def get_file_volume_snapshot_list(self, volume_id, **kwargs):
        :return: Returns a list of snapshots for the specified volume.
        """
        return self.get_volume_snapshot_list(volume_id, **kwargs)
+    def set_file_volume_snapshot_notification(self, volume_id, notification_flag, **kwargs):


Same comments as the block manager

allmightyspiff · 2021-10-22T02:42:19Z

SoftLayer/managers/storage.py

@@ -112,6 +112,15 @@ def get_volume_snapshot_list(self, volume_id, **kwargs):
            kwargs['mask'] = ','.join(items)

        return self.client.call('Network_Storage', 'getSnapshots', id=volume_id, **kwargs)
+    def set_volume_snapshot_notification(self, volume_id, notification_flag, **kwargs):


remove the **kwargs as its not really needed.

FernandoOjeda

Looks good, just a comment.

FernandoOjeda · 2021-10-26T20:41:06Z

SoftLayer/managers/block.py

+        :param volume_id: ID of volume.
+        :param kwargs:
+        :param notification-flag: Enable/Disable flag for snapshot warning notification.
+        :return:  Enables/Disables snapshot space usage threshold warning for a given volume.


It would be good if you specify what type of data this method is returning too:
e.g.
bool or an object for example SoftLayer_Network_Storage.

Did not get your recommendation. The return type is a string from an SLCLI perspective. Its a message

FernandoOjeda · 2021-10-26T20:47:01Z

SoftLayer/managers/block.py

+
+        :param volume_id: ID of volume.
+        :param kwargs:
+        :return: Enabled/Disabled snapshot space usage threshold warning for a given volume.


The same that above, about specifying what type of data this method is returning.

Did not get your recommendation. The return type is a string from an SLCLI perspective. Its a message

allmightyspiff

I'm a little concerned that it looks like this API is returning and requiring the strings "True" and "False" instead of actual boolean values, if that is the case then the API needs to be updated to fix that.

In any case, the python code should just be dealing with the boolean values, and not the strings. Hopefully that makes sense.

Also, please mark the comments as "resolved" when you resolve them, that helps. And make sure you remove the undeeded code in the file/block managers that you moved to the storage manager since those are no longer needed. Thanks.

allmightyspiff · 2021-10-27T19:00:11Z

SoftLayer/CLI/block/snapshot/set_notify_status.py

+    if enable:
+        enabled = 'True'
+    else:
+        enabled = 'False'


This wont work. Just pass the boolean value into the manager/api, otherwise the SL API will interpret it as a string, which PHP will always treat as true. Just pass enable into the manager function.

I am in agreement with you. However, this is sort of limitation of code flow between python and PHP. I tried sending the flag but its not properly getting received at the REST end. If required i can demonstrate over a webex.
The current code is test verified over an SLCLI and screenshots are uploaded.

Suggested change

if enable:

enabled = 'True'

else:

enabled = 'False'

if enable:

enabled = 'True'

else:

enabled = 'False'

allmightyspiff · 2021-10-27T19:00:18Z

SoftLayer/CLI/file/snapshot/set_notify_status.py

+    if enable:
+        enabled = 'True'
+    else:
+        enabled = 'False'
+


This wont work. Just pass the boolean value into the manager/api, otherwise the SL API will interpret it as a string, which PHP will always treat as true. Just pass enable into the manager function.

SoftLayer/CLI/file/snapshot/get_notify_status.py

SoftLayer/CLI/block/snapshot/get_notify_status.py

allmightyspiff · 2021-10-28T19:54:02Z

Testing Results:

get-status on a volume that hasn't had the status set

$ ./slcli -vvv  block  snapshot-get-notification-status 235854228

Calling: SoftLayer_Network_Storage::getSnapshotNotificationStatus(id=235854228, mask='', filter='None', args=(), limit=None, offset=None))

https://api.softlayer.com:443 "GET /rest/v3.1/SoftLayer_Network_Storage/235854228/getSnapshotNotificationStatus.json HTTP/1.1" 200 4

Returned Data:
None

Snapshots space usage threshold warning flag setting is disabled for volume 235854228

Set the status to enabled

$ ./slcli -vvv  block  snapshot-set-notification 235854228 --enable
Calling: SoftLayer_Network_Storage::setSnapshotNotification(id=235854228, mask='', filter='None', args=('True',), limit=None, offset=None))

Returned Data:
None

curl -u $SL_USER:$SL_APIKEY -X POST -H "Accept: */*" -H "Accept-Encoding: gzip, deflate, compress" -d '{"parameters": ["True"]}' 'https://api.softlayer.com/rest/v3.1/SoftLayer_Network_Storage/235854228/setSnapshotNotification.json'

Get an enabled status

$ ./slcli -vvv  block  snapshot-get-notification-status 235854228
Calling: SoftLayer_Network_Storage::getSnapshotNotificationStatus(id=235854228, mask='', filter='None', args=(), limit=None, offset=None))

Returned Data:
True

Snapshots space usage threshold warning flag setting is enabled for volume 235854228

Set status to disabled

./slcli -vvv  block  snapshot-set-notification 235854228 --disable
Calling: SoftLayer_Network_Storage::setSnapshotNotification(id=235854228, mask='', filter='None', args=('False',), limit=None, offset=None))

Returned Data:
None

curl -u $SL_USER:$SL_APIKEY -X POST -H "Accept: */*" -H "Accept-Encoding: gzip, deflate, compress" -d '{"parameters": ["False"]}' 'https://api.softlayer.com/rest/v3.1/SoftLayer_Network_Storage/235854228/setSnapshotNotification.json'

Get disabled status

$ ./slcli -vvv  block  snapshot-get-notification-status 235854228
Calling: SoftLayer_Network_Storage::getSnapshotNotificationStatus(id=235854228, mask='', filter='None', args=(), limit=None, offset=None))

Returned Data:
False

Snapshots space usage threshold warning flag setting is disabled for volume 235854228

Set the status to something that isn't the strings "True"/"False"

$ slcli  -vvv call-api  SoftLayer_Network_Storage setSnapshotNotification --id=235854228 LitterallyAnyhting
Calling: SoftLayer_Network_Storage::setSnapshotNotification(id=235854228, mask='', filter='{}', args=('LitterallyAnyhting',), limit=None, offset=None))

Returned Data:
None

curl -u $SL_USER:$SL_APIKEY -X POST -H "Accept: */*" -H "Accept-Encoding: gzip, deflate, compress" -d '{"parameters": ["LitterallyAnyhting"]}' 'https://api.softlayer.com/rest/v3.1/SoftLayer_Network_Storage/235854228/setSnapshotNotification.json'

$ ./slcli -vvv  block  snapshot-get-notification-status 235854228
Calling: SoftLayer_Network_Storage::getSnapshotNotificationStatus(id=235854228, mask='', filter='None', args=(), limit=None, offset=None))

Returned Data:
LitterallyAnyhting
Snapshots space usage threshold warning flag setting is disabled for volume 235854228

@hariha74 For this API method, I really think you need to make SoftLayer_Network_Storage/setSnapshotNotification accept only a boolean value, and SoftLayer_Network_Storage/getSnapshotNotificationStatus return only a boolean value. Otherwise between the various ways a user can call this API, you are going to have all sorts of strings in this status table and its going to be a mess.

You should also make sure the documentation for these methods looks correct.
https://sldn.softlayer.com/reference/services/SoftLayer_Network_Storage/setSnapshotNotification/ needs to not have <<EOT as the description. and the description for https://sldn.softlayer.com/reference/services/SoftLayer_Network_Storage/getSnapshotNotificationStatus/ is incorrect, looks copied directly from another method instead.

When those API methods are updated to return only a boolean value, I think updating this CLI code to work with that should be pretty straight forward and make everything a lot easier. Let me know if you have any questions.

hariha74 · 2021-10-29T07:25:49Z

I am in agreement as i said earlier. Essentially, the way boolean is interpreted in a PHP ( as True ) is different from a boolean is interpreted in python ( true ). I would recommend to have a webex once to have a deep dive discussion. I am scheduling it for Tuesday ( Monday being a holiday in India ). Thanks again for all your help in analyzing the code and providing your valuable comments.

allmightyspiff

Need to fix the code style errors first, but otherwise looks fine. You can check the analysis with tox -e analysis in your local softlayer-python directory.

hariha74 added 3 commits October 18, 2021 09:51

Changes for snapshot notification enable and disable option from slcli

677f434

Changes for snapshot notification enable and disable option from slcli

Minor cosmetic and parameter updates

727cc90

Removing setup tagging

f266dcc

allmightyspiff requested review from allmightyspiff and FernandoOjeda October 21, 2021 13:34

allmightyspiff assigned allmightyspiff and hariha74 and unassigned allmightyspiff Oct 21, 2021

allmightyspiff added CLI New Feature labels Oct 21, 2021

hariha74 added 2 commits October 21, 2021 21:49

Formatting errors resolution

f7eea01

Formatting errors mentioned in softlayer#1554 being resolved.

updating get command

854087e

allmightyspiff requested changes Oct 22, 2021

View reviewed changes

Incorporating review comments

da4593e

FernandoOjeda reviewed Oct 26, 2021

View reviewed changes

allmightyspiff requested changes Oct 27, 2021

View reviewed changes

Incorporating review comments

4ccb9b8

allmightyspiff approved these changes Nov 3, 2021

View reviewed changes

hariha74 added 2 commits November 4, 2021 17:37

TOX formatting resolution

d2ac1a6

TOX issue resolution

e23440d

allmightyspiff merged commit 76c4203 into softlayer:master Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snapshot notify #1554

Snapshot notify #1554

hariha74 commented Oct 21, 2021

hariha74 commented Oct 21, 2021

allmightyspiff commented Oct 21, 2021

allmightyspiff left a comment •

edited

Loading

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 22, 2021

hariha74 Oct 29, 2021

FernandoOjeda left a comment

FernandoOjeda Oct 26, 2021 •

edited

Loading

hariha74 Oct 29, 2021

FernandoOjeda Oct 26, 2021

hariha74 Oct 29, 2021

allmightyspiff left a comment •

edited

Loading

allmightyspiff Oct 27, 2021

hariha74 Oct 29, 2021

allmightyspiff Oct 27, 2021

allmightyspiff commented Oct 28, 2021 •

edited

Loading

hariha74 commented Oct 29, 2021

allmightyspiff left a comment

	'Snapshots space usage threshold warning flag setting is null. Set to default value enable. For volume %s'
	'Snapshots space usage threshold warning flag setting is null. Use `slcli block snapshot-set-notification %s --enable` to enable'

Snapshot notify #1554

Snapshot notify #1554

Conversation

hariha74 commented Oct 21, 2021

hariha74 commented Oct 21, 2021

allmightyspiff commented Oct 21, 2021

allmightyspiff left a comment • edited Loading

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

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

FernandoOjeda left a comment

Choose a reason for hiding this comment

FernandoOjeda Oct 26, 2021 • edited Loading

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

allmightyspiff left a comment • edited Loading

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

allmightyspiff commented Oct 28, 2021 • edited Loading

hariha74 commented Oct 29, 2021

allmightyspiff left a comment

Choose a reason for hiding this comment

allmightyspiff left a comment •

edited

Loading

FernandoOjeda Oct 26, 2021 •

edited

Loading

allmightyspiff left a comment •

edited

Loading

allmightyspiff commented Oct 28, 2021 •

edited

Loading