Change in vdsm[master]: getDeviceList - passing includePartitioned to HSM

Daniel Erez has uploaded a new change for review.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................

getDeviceList - passing includePartitioned to HSM

Made the needed modifications for passing 'options' list
(and 'includePartitioned' parameter within it)
to hsm -> getDeviceList function.

BindingXMLRPc: added missing 'options' argument for passing to API.
API: added missing 'options' argument for passing to HSM.
vdsClient:
* Parsed args in 'getDeviceList' function for passing to HSM.
* Added 'includePartitioned' optional parameter to CLI command.

Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Signed-off-by: Daniel Erez <derez(a)redhat.com&gt;
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm_cli/vdsClient.py
3 files changed, 12 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/28/4828/1
--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;

Daniel Erez has abandoned this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 4: Abandoned

'includePartitioned' flag is not needed any more.
Instead, a status field has been added to 'getDevicetList' LUNs:
http://gerrit.ovirt.org/#/c/5515/

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 4: Looks good to me, but someone else must approve

Even though I haven't seen your handling _parseDriveSpec yet.

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Daniel Erez has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 3: (3 inline comments)

....................................................
File AUTHORS
Line 29:    Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;   
Done

....................................................
File vdsm_cli/vdsClient.py
Line 549:         options = self._parseDriveSpec(args[1]) if len(args) > 1 else {}
I'll send a fix on a separate patch.

Line 553:             options['includePartitioned'] =
options.get('includePartitioned', 
Done

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Ayal Baron has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 3: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File AUTHORS
Line 29:    Deepak C Shetty <deepakcs(a)linux.vnet.ibm.com&gt;   
please remove trailing spaces

....................................................
File vdsm_cli/vdsClient.py
Line 549:         options = self._parseDriveSpec(args[1]) if len(args) > 1 else {}
parse*Drive*Spec?
If it does what you need and is more general then name should be updated to reflect what
it really does

Line 553:             options['includePartitioned'] =
options.get('includePartitioned', 
Please removing trailing space

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 3: Looks good to me, but someone else must approve

though I still dislike the undocumented 0.

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Daniel Erez has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 2: (2 inline comments)

....................................................
File vdsm_cli/vdsClient.py
Line 548:         storageType = int(args[0]) if len(args) > 0 else 0
Currently, the signature of 'getDeviceList' method is:
getDeviceList(self, storageType=None, options={})

Isn't 'storageType=None' enough?
Or, should I simply replace:
- 'storageType = int(args[0]) if len(args) > 0 else 0' 
with 
'storageType = args[0] if len(args) > 0 else None'

Line 555:         except:
'options.get' throws an exception when 'options' is not a dictionary.
Hence, the CLI outputs an error stack - is it an acceptable behavior?

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File AUTHORS
Line 32:    Daniel Erez <derez(a)redhat.com&gt;
Please keep sorted by last name (I see that we've failed to note Asir's and
Li's misplacement. Please fix their position while at it).

....................................................
File vdsm_cli/vdsClient.py
Line 548:         storageType = int(args[0]) if len(args) > 0 else 0
0 happens to mean "all storage types" but that's undocumented. either fix

 getDeviceList(self, storageType=0, ...

or find a better solution (maybe in another patch).

Line 555:         except:
except: is frowned upon (ask Edu why). Why do you need it here?

 options['includePartitioned'] = options.get('includePartitioned'

seems enough to me.

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File vdsm_cli/vdsClient.py
Line 545:         params = {'includePartitioned': includePartitioned}
* includePartitioned *is* a special option in the api. you are now exposing this fact in
the commandline.
* I'm not sure about the direction of the default. vdsm's default is False, so the
simple choice is to follow suit. However, True is what most human users of vdsClient would
 want.
* don't aim to learn code style from vdsClient. Really. Don't.

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Daniel Erez has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 1: (1 inline comment)

....................................................
File vdsm_cli/vdsClient.py
Line 545:         params = {'includePartitioned': includePartitioned}
* It would indeed be nicer - but it means that 'includePartitioned' is regarded as
a special option.
* Do we prefer 'includePartitioned' to be set to true by default?
* I've just followed the convention of using 'params' in vdsClient - I'l
change it to 'options'....

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 1:

oh, and welcome to Vdsm ;-)

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(1 inline comment)

BTW, I personally do not see a problem in flipping the default of includePartitioned - no
vdsClient user would explode if he finds partitions reported in his output.

....................................................
File vdsm_cli/vdsClient.py
Line 545:         params = {'includePartitioned': includePartitioned}
It would be nicer if you used _parseDriveSpec and have the command line look like

  getDeviceList <storageType> includePartitioned:True,

also, the second param to getDeviceList is called "options" not
"params".

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;

Daniel Erez has posted comments on this change.

Change subject: getDeviceList - passing includePartitioned to HSM
......................................................................


Patch Set 1: Verified

--
To view, visit http://gerrit.ovirt.org/4828
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I388442f361ea4ae00e3d8878a2497c7819ab2214
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Daniel Erez <derez(a)redhat.com&gt;
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com&gt;