Change in vdsm[master]: configure verb: replace libvirt_configure.sh call with pytho...
mtayer at redhat.com
mtayer at redhat.com
Wed Apr 30 08:59:58 UTC 2014
mooli tayer has posted comments on this change.
Change subject: configure verb: replace libvirt_configure.sh call with python code.
......................................................................
Patch Set 2:
(9 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 n
ok
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 ^^
ok so I will join all the verbs this patch and then I can drop the two methods and configura_libvier.sh and all its uses.
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?
that's what the script does.
do you think we should test is configured?
any way I think we should have one "replace sh with py" patch and things that change behavior in dependant commits
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
no it is the configure part,
I build the configuration values according to vdsm conf.
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
sysvToUpstart ?
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 c
Could not find any bindings. our service.py could not be used
either(it does not have the "reload-configuration verb" and
it is meant to be init system agnostic and here we want to
specifically call Upstart).
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 ^
will do now that i'm replacing the verbs in this patch
Line 323:
Line 324: def removeConf(self):
Line 325: for path in self._get_conf_files():
Line 326: if os.path.exists(path):
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 ?
This function is going to be removed (it is only needed for
old way for testing.) I will convert all other new ones to
camelCase and old ones in a different patch
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/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?
These tests now fail.
There is a problem in testing complete configure now due to
operations the we don't want to do in testing.
I don't like the skipOnTest approach. what I try to do is
crate utilities and unit test them.
Testing entire flows is also important in this case so I will
try to create a new test using mocks.
--
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 at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: mooli tayer <mtayer at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list