Deepak C Shetty has posted comments on this change.
Change subject: gluster: Get size information of a gluster volume
......................................................................
Patch Set 5: Code-Review-1
(9 comments)
....................................................
File client/vdsClientGluster.py
Line 715: 'Returns status of all gluster services if serviceName
is '
Line 716: 'not set'
Line 717: '(swift, glusterd, smb, memcached)'
Line 718: )),
Line 719: 'glusterVolumeSizeInfoGet': (
SInce this ends up doing glfs_statvfs.. why do we name it as volumeinfoget ?
Why not glusterVolumeStatsGet ? volumeinfoget is confusing as one things it is
doing `gluster volume info`
Line 720: serv.do_glusterVolumeSizeInfoGet,
Line 721: ('volumeName=<volume name> [humanReadable=<yes|no>]',
Line 722: 'Returns total, free and used space(bytes) of gluster
volume'
Line 723: )),
Line 719: 'glusterVolumeSizeInfoGet': (
Line 720: serv.do_glusterVolumeSizeInfoGet,
Line 721: ('volumeName=<volume name> [humanReadable=<yes|no>]',
Line 722: 'Returns total, free and used space(bytes) of gluster
volume'
Line 723: )),
How abt adding addnl parameter for volFileServer. Without that you are always
going to assume glusterd /server is running on local machine. In VDSM and in
general mgmt. usecase.. the hyp host may or maynot be the same as gluster
host.. so volFileServer should help provide the capability to query remote
gluster host from hyp host when both are not co-located
....................................................
File vdsm.spec.in
Line 1271: %dir %{_datadir}/%{vdsm_name}/gluster
Line 1272: %doc COPYING
Line 1273: %{_datadir}/%{vdsm_name}/gluster/api.py*
Line 1274: %{_datadir}/%{vdsm_name}/gluster/vdsmapi-gluster-schema.json
Line 1275: %{_datadir}/%{vdsm_name}/gluster/gfapi.py*
IIUC this provides the pythin binding for libgfapi. This should ideally be part
of glusterfs-api RPM package and when -api gets installed.. the python binding
should go to the /usr/lib64/python-x.x/site-packages/... dir such that it can
be used not just by vdsm-gluster but even in vdsm in general or any appln on
the host, agree ?
By doing this, u r limiting the usage of gfapi.py to vdsm-gluster only. Is that
the plan ?
Having said that I see that gfapi.py only has support for statvfs.. what abt
other gfapi methods ? Is there a plan to make gfapi.py provide full support for
all libgfapi methods ?
Line 1276: %{_datadir}/%{vdsm_name}/gluster/hooks.py*
Line 1277: %{_datadir}/%{vdsm_name}/gluster/services.py*
Line 1278: %endif
Line 1279:
....................................................
File vdsm/gluster/api.py
Line 289: return {'services': status}
Line 290:
Line 291: @exportAsVerb
Line 292: def volumeSizeInfoGet(self, volumeName, human_readable=False,
Line 293: options=None):
Similar point.. why name it as sizeinfo why not volumeStatsInfoGet .. so that
caller know he/she is asking for volume Stats. Also add volFileServer for
ability to query a remote gluster host
Line 294: """
Line 295: f_blocks = Total number of blocks
Line 296: f_bfree = Total number of blocks free
Line 297: f_bavail = Total number of blocks available for non root user
Line 293: options=None):
Line 294: """
Line 295: f_blocks = Total number of blocks
Line 296: f_bfree = Total number of blocks free
Line 297: f_bavail = Total number of blocks available for non root user
What is the significance of non root user here ? Can you put better comment..
Does that mean for root use this field will be intepreted diferently, if yes
how ?
Line 298: total blocks available = f_blocks - (f_bfree - f_bavail)
Line 299:
Line 300: @returns {
Line 301: 'total': TOTALBYTES,
Line 312: total = formatVolumeSizeHumanReadable(total)
Line 313: free = formatVolumeSizeHumanReadable(free)
Line 314: used = formatVolumeSizeHumanReadable(used)
Line 315:
Line 316: return {'total': str(total), 'free': str(free), 'used':
str(used)}
Per @returns i thot you are returning number, here its string... pls indicate
in @return that is a str.
Line 317:
Line 318:
Line 319: def getGlusterMethods(gluster):
Line 320: l = []
....................................................
File vdsm/gluster/gfapi.py
Line 24: import exception as ge
Line 25: from . import makePublic
Line 26:
Line 27:
Line 28: GLUSTER_VOL_PROTOCAL = 'tcp'
s/PROTOCAL/PROTOCOL
What abt rdma and unix protocol ? If we don't support them, and volume was
created with rdma transport type.. we should return appr. error somewhere from
this .py ?
Line 29: GLUSTER_VOL_HOST = 'localhost'
Line 30: GLUSTER_VOL_PORT = 24007
Line 31: GLUSTER_VOL_PATH = "/"
Line 32: _api = None
Line 25: from . import makePublic
Line 26:
Line 27:
Line 28: GLUSTER_VOL_PROTOCAL = 'tcp'
Line 29: GLUSTER_VOL_HOST = 'localhost'
This is a wrong assumption. This only works for the case where the vdsm-gluster
is on the same host as the gluster peer. As mentioned prev. its possible that
in VDSM case gluster peer is a diff host than hyp. host. Always aim for remote
host usecase to be safer from mgmt. usecase perspective.
Line 30: GLUSTER_VOL_PORT = 24007
Line 31: GLUSTER_VOL_PATH = "/"
Line 32: _api = None
Line 33:
Line 61: return
Line 62:
Line 63:
Line 64: def _volumeMount(volumeId):
Line 65: fs = _api.glfs_new(volumeId)
IIUC from the gfapi.py perspective, we are passing volumeName, so why confuse
by putting the param as volumeID ? Even in glfs.h i see glfs_new taking arg as
volumeName.
Line 66: _api.glfs_set_volfile_server(fs,
Line 67: GLUSTER_VOL_PROTOCAL,
Line 68: GLUSTER_VOL_HOST,
Line 69: GLUSTER_VOL_PORT)
--
To view, visit http://gerrit.ovirt.org/17822
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I358d4f3bf793ecc1a01e0592d68919d1405f6e19
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avish...@redhat.com>
Gerrit-Reviewer: Aravinda VK <avish...@redhat.com>
Gerrit-Reviewer: Bala.FA <barum...@redhat.com>
Gerrit-Reviewer: Deepak C Shetty <deepa...@linux.vnet.ibm.com>
Gerrit-Reviewer: Timothy Asir <tjeya...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
