7:58 a.m.
Hello Timothy Asir, Saggi Mizrahi, Dan Kenigsberg,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/2808
to review the following change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
[WIP: REVIEW ONLY] Added Gluster file system management support.
Verbs added into vdsm are
glusterVolumesList,
glusterVolumeCreate,
glusterVolumeStart,
glusterVolumeStop,
glusterVolumeDelete,
glusterVolumeSet,
glusterVolumeBrickAdd,
glusterVolumeRebalanceStart,
glusterVolumeRebalanceStop,
glusterVolumeRebalanceStatus,
glusterVolumeReplaceBrickStart,
glusterVolumeReplaceBrickAbort,
glusterVolumeReplaceBrickPause,
glusterVolumeReplaceBrickStatus,
glusterVolumeReplaceBrickCommit,
glusterVolumeRemoveBrickStart,
glusterVolumeRemoveBrickPause,
glusterVolumeRemoveBrickAbort,
glusterVolumeRemoveBrickStatus,
glusterVolumeRemoveBrickCommit,
glusterVolumeRemoveBrickForce,
glusterHostAdd,
glusterHostRemove,
glusterHostsList,
glusterDisksList
Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Signed-off-by: Timothy Asir <tjeyasin(a)redhat.com>
Signed-off-by: Bala.FA <barumuga(a)redhat.com>
---
M configure.ac
M vdsm.spec.in
M vdsm/BindingXMLRPC.py
M vdsm/Makefile.am
M vdsm/clientIF.py
A vdsm/gluster/Makefile.am
A vdsm/gluster/__init__.py.in
A vdsm/gluster/gluster_api.py
A vdsm/gluster/gluster_binding.py
A vdsm/gluster/gluster_cli.py
A vdsm/gluster/gluster_exception.py
A vdsm/gluster/gluster_utils.py
M vdsm_cli/Makefile.am
A vdsm_cli/gluster_extension.py
M vdsm_cli/vdsClient.py
15 files changed, 1,824 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/2808/1
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
10:17 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File vdsm/gluster/__init__.py.in
Line 1: #
Don't forget to add vdsm/gluster/__init__.py to .gitignore
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:39 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 16: I would prefer that you didn't submit this
I do not see that you've followed Federico's advice. Was this intentional?
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
4:36 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 15: (1 inline comment)
....................................................
File vdsm_cli/vdsm_cli_gluster_extension.py
Line 1: # Copyright 2012 Red Hat, Inc.
Done
How about pep-8 for module?
http://www.python.org/dev/peps/pep-0008/#package-and-module-names
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
3:01 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Federico Simoncelli has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 15: (1 inline comment)
....................................................
File vdsm_cli/vdsm_cli_gluster_extension.py
Line 1: # Copyright 2012 Red Hat, Inc.
I just don't like the file name, can we stick to camel-case as all the other files?
Eg: vdsClientGluster.py
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:37 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 14: (1 inline comment)
....................................................
File vdsm/BindingXMLRPC.py
Line 180: if self.cif.gluster and _glusterEnabled:
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:52 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(1 inline comment)
for visibility
....................................................
File vdsm/BindingXMLRPC.py
Line 180: if self.cif.gluster and _glusterEnabled:
I think Ewoud's comment makes sense. Please relate to it.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:42 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 14: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
8:58 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 13: Looks good to me, but someone else must approve
(1 inline comment)
Minor comment inline.
....................................................
File vdsm/BindingXMLRPC.py
Line 180: if self.cif.gluster and _glusterEnabled:
I'm not that familiar with how cif properties are defined, but isn't this safer:
if _glusterEnabled and self.cif.gluster:
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
7:32 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 13: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:08 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 13: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
10:02 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 12: (1 inline comment)
....................................................
File vdsm/gluster/api.py
Line 234: if hasattr(func, 'exportAsVerb') and \
The statement can be rewritten as
if getattr(func, 'exportAsVerb', False) == True:
But True check is required because of getattr() on svdsmProxy returns some value always.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
8:09 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 12: Looks good to me, but someone else must approve
(1 inline comment)
but a minor comment within.
....................................................
File vdsm/gluster/api.py
Line 234: if hasattr(func, 'exportAsVerb') and \
isn't this just like
getattr(func, 'exportAsVerb', False)
but more complex?
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
7:26 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 12: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
12:29 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 11: (1 inline comment)
....................................................
File vdsm/gluster/api.py
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:38 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Added Gluster file system management support.
......................................................................
Added Gluster file system management support.
Verbs added into vdsm are
glusterVolumesList,
glusterVolumeCreate,
glusterVolumeStart,
glusterVolumeStop,
glusterVolumeDelete,
glusterVolumeSet,
glusterVolumeBrickAdd,
glusterVolumeRebalanceStart,
glusterVolumeRebalanceStop,
glusterVolumeRebalanceStatus,
glusterVolumeReplaceBrickStart,
glusterVolumeReplaceBrickAbort,
glusterVolumeReplaceBrickPause,
glusterVolumeReplaceBrickStatus,
glusterVolumeReplaceBrickCommit,
glusterVolumeRemoveBrickStart,
glusterVolumeRemoveBrickStop,
glusterVolumeRemoveBrickStatus,
glusterVolumeRemoveBrickCommit,
glusterVolumeRemoveBrickForce,
glusterHostAdd,
glusterHostRemove,
glusterHostsList
Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Signed-off-by: Timothy Asir <tjeyasin(a)redhat.com>
Signed-off-by: Bala.FA <barumuga(a)redhat.com>
---
M Makefile.am
M vdsm.spec.in
M vdsm/BindingXMLRPC.py
M vdsm/clientIF.py
M vdsm/gluster/Makefile.am
A vdsm/gluster/api.py
M vdsm_cli/Makefile.am
M vdsm_cli/vdsClient.py
A vdsm_cli/vdsClientGluster.py
9 files changed, 626 insertions(+), 2 deletions(-)
Approvals:
Timothy Asir: Verified
Dan Kenigsberg: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:38 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 22: Looks good to me, approved
Pushing, even though Haim has reported a possible problem with migration. I think it has
low probability to be related to this patch, and could be solved in another patch anyway.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
11:05 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Timothy Asir has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 22: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
3:05 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 18: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
6:11 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 17: (1 inline comment)
....................................................
File vdsm_cli/vdsClientGluster.py
Line 374: ('<volume_name> [replica=<count>]
bricks=brick[,brick] ... \n\t' \
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
9:33 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 17: Looks good to me, but someone else must approve
(1 inline comment)
....................................................
File vdsm_cli/vdsClientGluster.py
Line 374: ('<volume_name> [replica=<count>]
bricks=brick[,brick] ... \n\t' \
minor: there's no need for all this trailing backslashes - they are inside
parenthesis.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
8:16 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 16:
My bad. I did all other changes except 'git mv'. Fix is on the way.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
12:30 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 10: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
7:45 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 9: Verified
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
9:42 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 7: (2 inline comments)
....................................................
File vdsm/gluster/gluster_api.py
Line 27: def verbMethod(func):
Done
Line 31: wrapper.verbMethod = 'yes'
Problem here is, getattr(self.svdsmProxy , ANY, None) returns non-None value always.
However getattr(func , ANY, None) == True will work. I feel defined string value is much
more reliable. Need to know your thoughts here.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
8:51 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 7: I would prefer that you didn't submit this
(2 inline comments)
....................................................
File vdsm/gluster/gluster_api.py
Line 27: def verbMethod(func):
a better name, maybe: exported? public?
Line 31: wrapper.verbMethod = 'yes'
I very much prefer when boolean logic is done in booleans, not strings.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:22 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 6:
so please, put these patch on the same git branch and push together to gerrit, so the
dependency shows and they the patches can be pushed consequently.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:31 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 6: (2 inline comments)
Yes. This depends on http://gerrit.ovirt.org/2797
....................................................
File vdsm/BindingXMLRPC.py
Line 891: ## Gluster verbs throw errors as exception, which is retrieved
Done
....................................................
File vdsm/gluster/gluster_api.py
Line 221: if types.MethodType == type(func) and \
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:22 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 6: (2 inline comments)
seems fine, though I suppose this should depend on http://gerrit.ovirt.org/2797 ?
....................................................
File vdsm/BindingXMLRPC.py
Line 891: ## Gluster verbs throw errors as exception, which is retrieved
you've defined VdsmException - I wouldn't mind if EVERY VdsmException has a
response() method. That would simplify this except clause - I think you could drop all
gluster references from here...
....................................................
File vdsm/gluster/gluster_api.py
Line 221: if types.MethodType == type(func) and \
I have a deja vu ;-)
again, I prefer decorators
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
12:25 a.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 11: (2 inline comments)
....................................................
File vdsm/gluster/api.py
Line 83: 'INFINIBAND': 'rdma'}
the other way around: the api should be list of "enums". translation to/fro
comma separated string should be hidden deep in gluster_cli.
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
I'm suggesting that volumeSetOptionsList() returns {'volumeSetOptions':
self.svdsmProxy.glusterVolumeSetHelpXml()}
@exportAsVerb can add _SUCCESS to that.
But my main issue is that you should not call "if" on a function that never
return False. It is misleading.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
10:42 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 11: (5 inline comments)
....................................................
File vdsm/gluster/api.py
Line 34: wrapper.exportAsVerb = 'yes'
Done
Line 71: info["volumeStatus"] = "ONLINE"
Done
Line 83: 'INFINIBAND': 'rdma'}
Are you saying transportList suppose to be a string?
I suppose to pass comma separated string of transportList without any translation. The
above transportMap is invalid. I will fix this.
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
Done
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
Most of gluster verbs here return success/failure, but few of them return success+more.
Do you suggest to have another decorator for such verbs?
In case of error, appropriate exception are raised by superVdsm. So there is no None
returned.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:53 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 11: (1 inline comment)
....................................................
File vdsm/gluster/api.py
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
if glusterVolumeStop() always return True, there is no point in testing its value. Blindly
returning _SUCCESS is better than seeming to return None.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:51 p.m.
New subject: Change in vdsm[master]: Added Gluster file system management support.
Dan Kenigsberg has posted comments on this change.
Change subject: Added Gluster file system management support.
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/gluster/api.py
Line 34: wrapper.exportAsVerb = 'yes'
I do not agree that the string 'yes' has any advantage over True.
btw, you can do
hasattr(func, 'exportAsVerb')
and ignore the value of the attr outside the wrapper.
Line 71: info["volumeStatus"] = "ONLINE"
"ONLINE" is part of the API so better be defined as a "class enum".
Line 83: 'INFINIBAND': 'rdma'}
I don't quite follow why it's 'tcp' here and 'TCP' a funciton
above.
I'd prefer if the API was all enum (which happen to be strings), and that users are
not to be allowed to play with the letter-case.
In my opinion, the list->string conversion should be hidden as deep as possible - when
passed to the command line.
I should have noticed this long ago, but I would like to hear if you can fix this now or
in a following patch.
Line 99: if self.svdsmProxy.glusterVolumeStop(volumeName, force):
not a blocker for this patch, but a point for thought: it may have been clearer and
shorter if exportAsVerb had added this _SUCCESS to each verb return value.
hmmm. is there anything that catches None in case of an error? We should never return None
over xmlrpc.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
5:09 a.m.
New subject: Change in vdsm[master]: [REVIEW ONLY] Added Gluster file system management support.
Bala.FA has posted comments on this change.
Change subject: [REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 2: (6 inline comments)
....................................................
File vdsm/BindingXMLRPC.py
Line 898: except ge.GlusterException, e:
I agree with you. currently only gluster verbs error handling is done through exception.
I will send separate new patch for basic vdsm exception which will be inherited by gluster
exception.
....................................................
File vdsm/gluster/gluster_cli.py
Line 48: if line.upper() == "NO VOLUMES PRESENT":
Localization is not yet supported by gluster. However gluster cli will give info in xml
soon ie these parsing will be thrown out.
Line 82: volumeOptions[tokens[0].strip()] = tokens[1].strip()
gluster cli will give output in xml soon. So these parsing will go away. Till then I
don't sure about this change requirement.
Line 93: volumeInfo["replicaCount"] = m.groups()[1]
Done
Line 100: volumeInfo["stripeCount"] = m.groups()[1]
Done
....................................................
File vdsm/gluster/gluster_exception.py
Line 42: self.err = err if err else []
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
4:15 p.m.
New subject: Change in vdsm[master]: [REVIEW ONLY] Added Gluster file system management support.
Ayal Baron has posted comments on this change.
Change subject: [REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 2: I would prefer that you didn't submit this
I actually have multiple issues with the way gLuster is plugged into vdsm here. I believe
there should be a more generic way to add a plugin. I believe Saggi will start a
discussion about this upstream.
My problem is with the import if exists approach as well as the way exceptions are handled
in wrapApiMethod in the xmlrpc bindings etc.
I believe some refactoring is due here (the existing code is just not built for such a
thing)
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
3:56 p.m.
New subject: Change in vdsm[master]: [REVIEW ONLY] Added Gluster file system management support.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: [REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 2: (5 inline comments)
....................................................
File vdsm/BindingXMLRPC.py
Line 898: except ge.GlusterException, e:
I'm still not happy about this because it will break if gluster is not available. I
know it's an annoying problem and still unsure what the best solution is. Something
like a base exception class that can be extended might be the best choice.
....................................................
File vdsm/gluster/gluster_cli.py
Line 82: volumeOptions[tokens[0].strip()] = tokens[1].strip()
I think creating a variable for tokens[0].strip().upper() might be useful here.
Line 93: volumeInfo["replicaCount"] = m.groups()[1]
Equal to m.group(2)
Line 100: volumeInfo["stripeCount"] = m.groups()[1]
Equal to m.group(2)
....................................................
File vdsm/gluster/gluster_exception.py
Line 42: self.err = err if err else []
How about:
self.x = x or []
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
3:34 p.m.
New subject: Change in vdsm[master]: [REVIEW ONLY] Added Gluster file system management support.
Ayal Baron has posted comments on this change.
Change subject: [REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 2: (2 inline comments)
....................................................
File vdsm/gluster/gluster_binding.py
Line 23: return ((gluster.volumeCreate, "glusterVolumeCreate"),
I'm not sure why we need this file.
First of all, it is not binding to any type of transport.
Second if all functions under gluster module should be exposed and there is a clear naming
convention then why manually iterate them here and not do it automatically?
If not all functions should be exposed then you can still decorate those that should in
the API file itself where it is clear it's exposed.
....................................................
File vdsm/gluster/gluster_cli.py
Line 48: if line.upper() == "NO VOLUMES PRESENT":
Does gLuster support localization? (i.e. could the output be in other languages?)
Is there any plan to add different return codes per message in gLuster?
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
2:48 a.m.
Bala.FA has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1:
Thanks. I will pull your changes in vdsm/gluster/gluster_utils.py and push it here.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
12:49 p.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1: (7 inline comments)
Some more suggestions. I have some code in progress that fixes it. To not invade your
patch again I pushed it to github:
https://github.com/ekohl/vdsm/commit/fbaab7a87fc097a8b01e688beab5e0ba5c4a...
....................................................
File vdsm/gluster/gluster_utils.py
Line 60: for line in f:
From here I would split it in a parseProcMdStat(lines). That would
make it easier to test.
Line 69: tokens[4:])}
For readability I would make a members variable prior to using it.
Line 77: {})
This seems somewhat complex and hard to wrap my head around. I'd use a generator:
for line int out:
yield dict(pair.split('=') for pair in line.strip().split(':'))
Then apply the reduce in the actual parsing.
Line 92: return _reduceToDict(_makeLvDict, out)
Again, I'd prefer to split it into getLvs and _parseLvs.
Line 106: if x.has_key(y['LVM2_VG_NAME']):
has_key is deprecated in favor of:
y['LVM2_VG_NAME'] in x
Line 111: return _reduceToDict(_makeVgDict, out)
Same deal here about splitting.
Line 126: return _reduceToDict(_makePvDict, out)
And here.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
4:15 a.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1: (1 inline comment)
....................................................
File vdsm/BindingXMLRPC.py
Line 898: except ge.GlusterException, e:
You could provide a VDSMModuleException or something along that line and let
GlusterException extend it. If you ship the former with vdsm and import it here you keep
gluster support optional.
An alternative would be to provide some mock exception in try except ImportError, but that
won't scale well I think.
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
3 a.m.
Bala.FA has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File configure.ac
Line 181: vdsm/gluster/Makefile
Done
....................................................
File vdsm/BindingXMLRPC.py
Line 898: except ge.GlusterException, e:
If no ge, then there won't be no ge.GlusterException. However there is no straight
way to do this
....................................................
File vdsm/gluster/gluster_api.py
Line 77: transportList))
Cool. Thanks. Will replace ugly reduce/map
....................................................
File vdsm/gluster/gluster_exception.py
Line 299: raise NameError, "Collision with RHEVM found: %s code %s:
" \
Done
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
1:08 p.m.
Ewoud Kohl van Wijngaarden has posted comments on this change.
Change subject: [WIP: REVIEW ONLY] Added Gluster file system management support.
......................................................................
Patch Set 1: (6 inline comments)
Partial review
....................................................
File configure.ac
Line 181: vdsm/gluster/Makefile
I think this is supposed to be sorted.
....................................................
File vdsm/BindingXMLRPC.py
Line 38: from gluster.gluster_binding import getGlusterMethods
I think you want absolute imports here, so vdsm.gluster.
Line 43: import gluster.gluster_exception as ge
Same here about absolute imports
Line 898: except ge.GlusterException, e:
This will throw a NameError if ge was not imported.
....................................................
File vdsm/gluster/gluster_api.py
Line 77: transportList))
How about:
transport = ','.join(filter(None, (transportMap.get(x.upper()) for x in
transportList)))
....................................................
File vdsm/gluster/gluster_exception.py
Line 299: raise NameError, "Collision with RHEVM found: %s code %s:
" \
How about oVirt here?
--
To view, visit http://gerrit.ovirt.org/2808
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd51bec292991f7abdcacf3ba747c9662f660f51
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud(a)kohlvanwijngaarden.nl>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
4353
days inactive
4425
days old
vdsm-patches@lists.fedorahosted.org
41 comments
6 participants
participants (6)
-
abaron＠redhat.com -
barumuga＠redhat.com -
Dan Kenigsberg -
ewoud＠kohlvanwijngaarden.nl -
Federico Simoncelli -
tjeyasin＠redhat.com