Change in vdsm[master]: gluster: Temporary fix for supervdsm memory leak.

barumuga at redhat.com barumuga at redhat.com
Thu Sep 25 08:49:57 UTC 2014


Bala.FA has posted comments on this change.

Change subject: gluster: Temporary fix for supervdsm memory leak.
......................................................................


Patch Set 3: Code-Review-1

(6 comments)

http://gerrit.ovirt.org/#/c/33312/3/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:

Line 72: # Path definitions
Line 73: #
Line 74: P_LIBVIRT_VMCHANNELS = '/var/lib/libvirt/qemu/channels/'
Line 75: P_VDSM = '@VDSMDIR@/'
Line 76: P_VDSM_GLUSTER = '@VDSMDIR@/gluster/'
I feel its better to be moved to gfapi.py
Line 77: P_VDSM_RPC = '@VDSMDIR@/rpc/'
Line 78: P_VDSM_HOOKS = '@HOOKSDIR@/'
Line 79: P_VDSM_LIB = '@VDSMLIBDIR@/'
Line 80: P_VDSM_RUN = '@VDSMRUNDIR@/'


http://gerrit.ovirt.org/#/c/33312/3/vdsm/gluster/gfapi.py
File vdsm/gluster/gfapi.py:

Line 26: 
Line 27: import exception as ge
Line 28: from vdsm import constants
Line 29: from vdsm import utils
Line 30: from __init__ import makePublic
Please move extra imports before workaround function
Line 31: 
Line 32: 
Line 33: GLUSTER_VOL_PROTOCAL = 'tcp'
Line 34: GLUSTER_VOL_HOST = 'localhost'


Line 146: # avoid that, this file is executed as script.
Line 147: # This can be reverted back once the memory leak issue
Line 148: # is fixed in libgfapi.
Line 149: @makePublic
Line 150: def volumeStatvfs(volumeName):
1. This function should accept as similar as volumeStatVfsGet()

2. For simplicity you could have two function and conditionally call based on _WORKAROUND constant.  This way function prototype will not change.
Line 151:     script = constants.P_VDSM_GLUSTER + "gfapi.pyc"
Line 152:     command = [constants.EXT_PYTHON, script, "-v", volumeName]
Line 153:     rc, out, err = utils.execCmd(command)
Line 154:     if rc != 0:


Line 149: @makePublic
Line 150: def volumeStatvfs(volumeName):
Line 151:     script = constants.P_VDSM_GLUSTER + "gfapi.pyc"
Line 152:     command = [constants.EXT_PYTHON, script, "-v", volumeName]
Line 153:     rc, out, err = utils.execCmd(command)
You could use raw=True to get out/err as single string
Line 154:     if rc != 0:
Line 155:         raise ge.GlfsStatvfsException(rc, out, err)
Line 156:     json_acceptable_string = out[0].replace("'", "\"")
Line 157:     res = json.loads(json_acceptable_string)


Line 173: def parse_cmdargs():
Line 174:     parser = argparse.ArgumentParser()
Line 175:     parser.add_argument("-v", "--volume", action="store", type=str,
Line 176:                         help="volumeName")
Line 177:     parser.add_argument("-ht", "--host", action="store", type=str,
Please have -h or -H
Line 178:                         default="localhost", help="host name")
Line 179:     parser.add_argument("-p", "--port", action="store", type=int,
Line 180:                         default=24007, help="port number")
Line 181:     parser.add_argument("-t", "--protocol", action="store", type=str,


Line 189:     try:
Line 190:         res = volumeStatvfsGet(args.volume, args.host,
Line 191:                                args.port, args.protocol)
Line 192:     except ge.GlusterLibgfapiException:
Line 193:         print "Failed to retrive volume stats"
Please print to stderr
Line 194:         sys.exit(1)
Line 195:     print json.dumps({'f_blocks': res.f_blocks, 'f_bfree': res.f_bfree,
Line 196:                       'f_bsize': res.f_bsize, 'f_frsize': res.f_frsize,
Line 197:                       'f_bavail': res.f_bavail, 'f_files': res.f_files,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7079426178ce47008d9a3b83635afce98536ca34
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list