Skip to content

Conversation

aparnapatil
Copy link
Contributor

Handling issue #282 . Provided ISCSI CLI support and Added testcases for the same
Operations supported are as follows:
CLI modules are divided into iSCSI targets section and snapshots section.
iSCSI commands

usage: sl iscsi [<command>] [<args>...] [options]

Manage, order, delete iSCSI targets

The available commands are:
  cancel    Cancel an existing iSCSI target
  create    Order and create an iSCSI target
  detail    Output details about an iSCSI
  list      List iSCSI targets on the account

Snapshot Commands

usage: sl snapshot [<command>] [<args>...] [options]

Manage, order, delete iSCSI snapshots

The available commands are:
  cancel                Cancel an iSCSI snapshot
  create                Create a snapshot of given iSCSI volume
  create-space          Orders space for storing snapshots
  list                  List snpshots of given iSCSI
  restore-volume        Restores volume from existing snapshot

@aparnapatil aparnapatil reopened this Mar 28, 2014
options = ['confirm']

def execute(self, args):
iscsi = ISCSIManager(self.client)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of iscsi iscsi_mgr would be more appropriate here.

@amol
Copy link
Contributor

amol commented Mar 28, 2014

Strange. Build seems to be failing because of files which are not part of this change set.

@swapnil90
Copy link
Contributor

@amol: the problem seems to be due to pep8 version change

@sudorandom
Copy link
Contributor

@amol, @swapnil90: I merged in a fix for the new pep8 errors so a rebase should fix the CI.

New iSCSI manager has been introduced to conveniently work with iSCSI
service provided by SofyLayer. Respective CLI module has also been
introduced for CLI interaction.
@aparnapatil
Copy link
Contributor Author

@sudorandom : Rebased the repo with master. Can you force the build to run again?

@sudorandom
Copy link
Contributor

@aparnapatil I'm not really able to... I have no idea why travis-ci isn't running. This passes everything when I run tox locally. It looks to work nicely. I have a couple UI-related notes that I'll make in-line

list List iSCSI targets
"""
# :license: MIT, see LICENSE for more details.
list List iSCSI targets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command listing should follow the style guide that we came up with: http://softlayer-python.readthedocs.org/en/latest/dev/cli.html#defining-a-module

E.G.: The second column should start 2 spaces after the longest command name. The list should be alphabetized. Capitalize the first letter of the description text.

For groupings of snapshot commands vs normal iscsi commands, you could use snapshot as a prefix for the command. Alternatively, you could divide up the command listing into a iSCSI target section and snapshot section similar to how the root help block (sl --help) divides modules into categories.

corrected command listing style and divided iSCSI section and snapshot section.
@aparnapatil
Copy link
Contributor Author

@sudorandom: Considered all review comments. I have added short description about 'zero_recurring' param in create_iscsi() as well.
_find_item_prices() function returns a list of ItemPriceIDs, some of which (low cost ones), aren't usable. For example, for 1GB volume, It will return [22501,22441,2152] with recurring fees 0,0.35,15 respectively. If we try to order iSCSI with ItemPriceID having recurring fee as 0/0.35, it fails saying -
SoftLayerAPIError(SoftLayer_Exception_Order_Item_Invalid): The following price(s) are not valid for package (0):22501: 1 GB iSCSI SAN Storage
May be these itemPriceIDS with 0/0.35 recurring fee are available for Softlayer people. So –zero-recurring flag added here. By default this flag is set to False. So, it will use ItemPriceID with recurringFee > 1 (i.e. here for 1 GB recurringFee $15)for ordering iSCSI. hope this helps.

@amol
Copy link
Contributor

amol commented Apr 7, 2014

@sudorandom, this is strange, and we haven't found any reason for this anywhere:

>>> _filter
{'itemPrices': {'item': {'capacity': {'operation': 1},
   'description': {'operation': '~ GB iSCSI SAN Storage'}}}}
>>> mask
'id,recurringFee'
>>> _1gb = client['Product_Package'].\
        getItemPrices(id=0,
                      filter=_filter.to_dict(),
                      mask=mask)
>>> _1gb
[{'id': 2152, 'recurringFee': '.35'},
 {'id': 22501, 'recurringFee': '0'},
 {'id': 22441, 'recurringFee': '15'}]

>>> 
def can_order(item, client):
    order = {'complexType': 'SoftLayer_Container_Product_Order_Network_Storage_Iscsi',
        'location': 138124,
        'packageId': 0,
        'prices': [{}],
        'quantity': 1}
    order['prices'][0]['id'] = item['id']
    try:
        client['Product_Order'].verifyOrder(order)
        print 'ordering %s succeeded' % item
    except SoftLayerAPIError as e:
        print 'ordering %s failed' % item
        print e.message

>>> for item in _1gb:
        can_order(item, client)
        print ''
>>>
ordering {'recurringFee': '.35', 'id': 2152} failed
The following price(s) are not valid for package (0):
2152: 1 GB iSCSI SAN Storage

ordering {'recurringFee': '0', 'id': 22501} failed
The following price(s) are not valid for package (0):
22501: 1 GB iSCSI SAN Storage

ordering {'recurringFee': '15', 'id': 22441} succeeded

The error given by the exception seems to be strange. The service seems to contradicting itself, as the item prices we are providing while ordering is given by the service itself. Not sure what might be the reason behind this. If anyone can order those with <$1 iSCSI, the zero_recurring would makes sense. Your thoughts?

@sudorandom
Copy link
Contributor

@amol @aparnapatil: Okay, I was able to get this figured out.

Here's the call I'm now using to get appropriate price ids:

client['Product_Package'].getItems(id=0, mask='id, capacity, prices', filter={
    'items': {
        'capacity': {'operation': '1'},
        'categories': {'categoryCode': {'operation': 'iscsi'}}}})

The structure is different (it returns items instead of price ids) but it does the proper filtering so that unusable price ids don't come back. http://sldn.softlayer.com/reference/services/SoftLayer_Product_Package/getItems

@aparnapatil
Copy link
Contributor Author

@sudorandom: Thanks for figuring out this. Removed zero-recurring param and unnecessary filters.

Fixure Product_Package has too many things which are not required by the iscsi tests. Using return_value from `mock` to test iscsi manager.
@aparnapatil
Copy link
Contributor Author

Do we need any modifications to this?
if not, can this be merged?

@sudorandom
Copy link
Contributor

@aparnapatil I apologize, I didn't notice that you'd pushed the changes that you did 2 days ago. I gave it a once over and it looks good to me to merge.

sudorandom added a commit that referenced this pull request Apr 11, 2014
@sudorandom sudorandom merged commit 002daa4 into softlayer:master Apr 11, 2014
@aparnapatil aparnapatil deleted the iscsi-manager branch April 28, 2014 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants