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)