Yaniv Bronhaim has posted comments on this change.
Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................
Patch Set 2: Code-Review-1
(11 comments)
http://gerrit.ovirt.org/#/c/27130/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:
Line 141: uncompresscmd /usr/bin/unxz
Line 142: compressext .xz
Line 143: }
Line 144: """
Line 145:
all of only libvirt related constant put under LibvirtModuleConfigure and not global
Line 146:
Line 147: class _ModuleConfigure(object):
Line 148: def __init__(self):
Line 149: pass
Line 210: raise RuntimeError("Failed to perform libvirt action.")
Line 211:
Line 212: def _get_libvirt_exec(self):
Line 213: return (os.path.join(P_VDSM_EXEC, 'libvirt_configure.sh'), )
Line 214:
you can remove _get_libvirt_exec and _exec_libvirt_configure ^^
Line 215: def configure(self):
Line 216: self.libvirtd_sysv2upstart()
Line 217: if utils.isOvirtNode():
Line 218: # TODO mtayer: Move the existance check to validate_ovirt_certs?
Line 220: raise RuntimeError(
Line 221: "vdsm: Missing certificate, vdsm not registered")
Line 222: validate_ovirt_certs.validate_ovirt_certs()
Line 223: # Remove a previous configuration (if present)
Line 224: self.removeConf()
always remove?
Line 225: lconf_maps = [LCONF_GENERAL]
Line 226: qconf_maps = [QCONF_GENERAL]
Line 227: ldconf_maps = [LDCONF_GENERAL]
Line 228: qlconf_maps = []
Line 225: lconf_maps = [LCONF_GENERAL]
Line 226: qconf_maps = [QCONF_GENERAL]
Line 227: ldconf_maps = [LDCONF_GENERAL]
Line 228: qlconf_maps = []
Line 229: # determine configuration
this is the valid part, put it on validate and call it
Line 230: config.read(VDSMM_CONF)
Line 231: if config.getboolean('vars', 'ssl'):
Line 232: qconf_maps.append(QCONF_SSL)
Line 233: if all(os.path.isfile(f) for f in
Line 265: if utils.isOvirtNode() and ovirtfunctions:
Line 266: ovirtfunctions.ovirt_store_config(fname)
Line 267: sys.stdout.write("Reconfiguration of libvirt is done.")
Line 268:
Line 269: def libvirtd_sysv2upstart(self):
i prefer - switchSysvHandlingToUpstart
Line 270: """
Line 271: On RHEL 6, libvirtd can be started by either SysV init or Upstart.
Line 272: We prefer upstart because it respawns libvirtd if when libvirtd
Line 273: crashed.
Line 297: "reload-configuration"))
we really need to call another script here? can't we avoid it by python's code?
Line 318: try:
Line 319: self._exec_libvirt_configure("check_if_configured")
Line 320: return True
Line 321: except RuntimeError:
Line 322: return False
this is easy to check with the config tool ^
Line 323:
Line 324: def removeConf(self):
Line 325: for path in self._get_conf_files():
Line 326: if os.path.exists(path):
Line 327: with ConfigFile(path, CONF_PREFIX, CONF_SUFFIX) as conff:
Line 328: conff.removeConf()
Line 329:
Line 330: def _get_conf_files(self):
Line 331: return LCONF, QCONF, QLCONF, LDCONF
lets keep standard with the function names "getConfFiles"
Line 332:
Line 333:
Line 334: class SanlockModuleConfigure(_ModuleConfigure):
Line 335:
Line 328: conff.removeConf()
Line 329:
Line 330: def _get_conf_files(self):
Line 331: return LCONF, QCONF, QLCONF, LDCONF
Line 332:
why not explicitly use the file names? why do you need the constants ?
Line 333:
Line 334: class SanlockModuleConfigure(_ModuleConfigure):
Line 335:
Line 336: SANLOCK_GROUPS = (QEMU_PROCESS_GROUP, VDSM_GROUP)
http://gerrit.ovirt.org/#/c/27130/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 1154: return (
Line 1155: os.path.exists('/etc/rhev-hypervisor-release') or
Line 1156: glob.glob('/etc/ovirt-node-*-release')
Line 1157: )
Line 1158:
no rejects for that? i recall many rejections, and don't remember why
Line 1159:
Line 1160: # Copied from
Line 1161: #
http://docs.python.org/2.6/library/itertools.html?highlight=grouper#recipes
Line 1162: def grouper(iterable, n, fillvalue=None):
http://gerrit.ovirt.org/#/c/27130/2/tests/toolTests.py
File tests/toolTests.py:
Line 185
Line 186
Line 187
Line 188
Line 189
removing this help for something? why not leaving it here?
--
To view, visit
http://gerrit.ovirt.org/27130
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie37988ef230f889e7154e504a889f28db5be7328
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: mooli tayer <mtayer(a)redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes