Hello Ayal Baron, Timothy Asir, Saggi Mizrahi, Federico Simoncelli, Dan Kenigsberg,
I'd like you to do a code review. Please visit
to review the following change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
[WIP] Added getStorageDevices verb.
This verb is used to get list of storage devices in the node. Return value is a dictionary, its keys are device names and values are properties of those devices as dictionary.
{DEVNAME: {'container': CONTAINER_DEVNAME, 'contentType': CONTENT_TYPE_STRING, 'endSize': SIZE_MB, 'flagList': STRING_LIST, 'fsSize': SIZE_MB, 'fsSizeFree': SIZE_MB, 'fsType': STRING, 'fsUuid': UUID, 'members': DEVNAME_LIST, 'model': STRING, 'mountPoint': STRING, 'name': DEVNAME, 'origin': LV_DEVNAME, 'parent': PARENT_DEVNAME, 'partitions': DEVNAME_LIST, 'size': SIZE_MB, 'sizeFree': SIZE_MB, 'startSize': SIZE_MB, 'status': STATUS_STRING, 'tableType': DISK_TABLE_TYPE, 'type': TYPE_STRING, 'uuid': UUID, 'vendor': STRING},...}
here, property dictionary contains
container: If DEVNAME is used by another device eg. LVM PV, MD etc, name of that device is set contentType: This is determined by various other properties. Possible value is one of 'SWAP', 'OS', 'DATA' and 'NA' endSize: If DEVNAME is a partition, its end boundry is set. The value is in MB flagList: If DEVNAME is a partition, its flags(eg. boot,bios_grub etc) are set as a list of strings fsSize: If DEVNAME has a file system, its size is set. The value is in MB fsSizeFree: If DEVNAME is mounted, its free size is set. The value is in MB fsType: If DEVNAME is formatted, its file system type is set fsUuid': If DEVNAME has file system, its UUID is set members: If DEVNAME is a MD, LVM VG etc, its member device names are set as list of strings model: Model of DEVNAME is set as string mountPoint: If DEVNAME is mounted, its mount point is set name: DEVNAME is set here for completion origin: If DEVNAME is a snapshot of LV, that LV name is set parent: If DEVNAME is a partition, its disk name is set partitions: If DEVNAME is a disk, its partition names are set as list of strings size: Size of DEVNAME is set. The value is in MB sizeFree: If DEVNAME is VG, its size of free extents is set. If its a partitioned disk, its unallocated size is set. The value is in MB startSize: If DEVNAME is a partition, its start boundry is set. The value is in MB status: This is determined by various other properites of the DEVNAME. Possible value is one of 'UNINITIALIZED', 'NA', 'UNUSABLE', 'FORMAT_UNSUPPORTED', 'MOUNTED', 'FORMATTED' and 'PARTED' tableType': If DEVNAME is a disk, its table(label) type is set as a string type: This is determined value. Possible value is one of 'MD', 'LVM_VG', 'LVM_LV_SNAPSHOT', 'LVM_LV' and 'BLOCK' uuid: UUID of DEVNAME vendor: Vendor of DEVNAME is set as string
Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Signed-off-by: Bala.FA barumuga@redhat.com --- M vdsm.spec.in M vdsm/API.py M vdsm/BindingXMLRPC.py M vdsm/Makefile.am A vdsm/storage_device_utils.py M vdsm_cli/vdsClient.py 6 files changed, 505 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/25/3725/1 -- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
could you compare this new verb to getDeviceList/getDeviceInfo ?
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Bala.FA has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
The new verb getStorageDevices lists disk, partitions, md, lvm vg/lv with properties of their relations.
I guess getDeviceList supposes to list lvm pvs and iscsi devices and getDeviceInfo lists information of a pv or iscsi device. However both are not working for me. 'vdsClient localhost getDeviceList' gives empty list.
The requirement is that to get possible block devices including their properties in one go. I am not sure getDeviceList/getDeviceInfo solves this.
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Eduardo has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(2 inline comments)
We have a lot of duplicate functionality here, Let's reuse and unify the functions of this patch. A lot of similar functions are in the vdsm.storage.devicemapper and vdsm.storage.lvm modules, etc. May be we can modify the output of getDeviceList adding the extra keys? Actual managers will ignore this new info.
Mr Bala, can we help you with the getDeviceList you are experiencing?
.................................................... File vdsm/storage_device_utils.py Line 38: return os.path.normpath(name) Please use devicemapper and multipath module functions
Line 92: Please use the lvm module.
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Bala.FA has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1: (2 inline comments)
It would be good to have getDeviceList (or something else) to populate these values with ignoring unwanted keys by respective managers.
Regarding non-working getDeviceList, could you tell me how I can run this verb? I ran 'vdsClient localhost getDeviceList' returns empty list for me.
.................................................... File vdsm/storage_device_utils.py Line 38: return os.path.normpath(name) I will check those modules
Line 92: I was trying to use the module, but relationship of pv/vg/lv are not stored in. I will check it once again and get back to you exactly
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Deepak C Shetty has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
Hello Bala,
1) Can you provide a usecase/scenario where this verb will be used in order to better understand the patch ?
I am assuming this will be used to list down the potential storage devices which can act as a brick for creating gluster volume from oVirt, as well as use these storage devices for basing VDSM storage domain upon.
2) Going ahead, its possible for oVirt user to use a gluster volume or a LUN provisioned from external array (both available/seen on the host) also as potential storage devices, in which case I am not sure if the proposed dict structure would suffice ? Should we be looking at a structure which is extensible to accomodate gluster and/or LUN capabilities ?
3) Given that storage in oVirt is either file/block based, and it will remain that ways going ahead, isn't it better to have a dict where primary key is either File or Block and values are dict as proposed by you in this patch ? That will help oVirt make calls like "Give me all File based storage devices" if user is creating a file based domain and similarly for block based. Having them as primary keys, will help oVirt GUI to populate availabel storage devices based on File or Block type, selected by user.
4) #3 above can be extended in future by adding Gluster as the primary key, in case oVirt user wishes to use gluster volume as the backend for storage domain
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Bala.FA has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
Hi Deepak,
We have internal discussion of using existing getDeviceList/getDeviceInfo verbs with an optional new flag which helps for gluster needs. This approach won't break existing functionalities.
I will update the status here.
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
change the name of the module: - To conform to our naming convention - Have it not end in utils. We have too much of those already and I'm always trying to get rid of them.
Have a class to contain all this information. We need this information and having a canonical device representation in vDSM is long over due.
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Bala.FA has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
Thanks Saggi. I will do these changes and update new patch.
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Bala.FA has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
Next patchset is in progress. I will update soon here
-- To view, visit http://gerrit.ovirt.org/3725 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I2cb217321a7a8dfcd1b507c7cba2888f08612207 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Bala.FA barumuga@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Deepak C Shetty deepakcs@linux.vnet.ibm.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Timothy Asir tjeyasin@redhat.com
Itamar Heim has posted comments on this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Patch Set 1:
ping?
Itamar Heim has abandoned this change.
Change subject: [WIP] Added getStorageDevices verb. ......................................................................
Abandoned
abandoning as no activity, please restore if relevant
vdsm-patches@lists.fedorahosted.org