Alon Bar-Lev has posted comments on this change.
Change subject: Introducing configurator package in vdsm-tool
......................................................................
Patch Set 15:
(4 comments)
....................................................
File lib/vdsm/tool/configurator.py
Line 41: sys.stdout.write("%s is already configured\n" %
self.getName())
Line 42: elif service.service_status(self.getName()) and not force:
Line 43: raise RuntimeError("Module %s is up. To force
reconfiguration use "
Line 44: "--force flag")
Line 45: else:
I suggest to use boolean and not numbers
I suggest the following structure:
if not self.isconfigured():
if service_status and not fource:
raise ...
return 0
notes:
1. if already configured, I do not expect failure.
2. please do not create dependency between module name and service name.
Line 46: ret = 0
Line 47: return ret
Line 48:
Line 49: def isconfigured(self):
Line 46: ret = 0
Line 47: return ret
Line 48:
Line 49: def isconfigured(self):
Line 50: return 0
boolean please
Line 51:
Line 52: def getName(self):
Line 53: return None
Line 54:
Line 57: def __init__(self):
Line 58: super(LibvirtModuleConfigure, self).__init__()
Line 59:
Line 60: def getName():
Line 61: return 'libvirtd'
usage friendly not service name?
Line 62:
Line 63: def _exec_libvirt_configure(self, action, *args):
Line 64: """
Line 65: Invoke libvirt_configure.sh script
Line 83: """
Line 84: libvirt configuration (--force to force reconfigure also when
Line 85: configured)
Line 86: """
Line 87: if super(LibvirtModuleConfigure, self).configure(force):
again... this should be for the whole transaction.
so if we have sanlock and libvirt to be configured we first as for all then
perform all.
moving this deeper into the code just make it more complex.
Line 88: libvirt_related_services = ["supervdsmd", "libvirtd"]
Line 89: service_should_start = []
Line 90: for srv in libvirt_related_services:
Line 91: if service.service_status(srv):
--
To view, visit http://gerrit.ovirt.org/20100
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I16bf5894e7e55a84b4c2a0caacde383ae7c19242
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshz...@linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
