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