Change in vdsm[master]: Introducing configurator package in vdsm-tool

Alon Bar-Lev alonbl at redhat.com
Mon Oct 21 07:08:54 UTC 2013


Alon Bar-Lev has posted comments on this change.

Change subject: Introducing configurator package in vdsm-tool
......................................................................


Patch Set 14:

(10 comments)

....................................................
File lib/vdsm/tool/configurator.py
Line 32:     def __init__(self):
Line 33:         pass
Line 34: 
Line 35:     def validate(self):
Line 36:         return 0
return boolean?
Line 37: 
Line 38:     def configure(self, force, autoreset):
Line 39:         return 0
Line 40: 


Line 41:     def isconfigured(self):
Line 42:         return 0
Line 43: 
Line 44:     def getName(self):
Line 45:         return ''
return None?
Line 46: 
Line 47: 
Line 48: class LibvirtModuleConfigure(_ModuleConfigure):
Line 49:     def getName():


Line 44:     def getName(self):
Line 45:         return ''
Line 46: 
Line 47: 
Line 48: class LibvirtModuleConfigure(_ModuleConfigure):
call constructor?

 def __init__(self):
     super(LibvirtModuleConfigure, self).__init__()
Line 49:     def getName():
Line 50:         return 'libvirt'
Line 51: 
Line 52:     def _exec_libvirt_configure(self, action, *args):


Line 82:                     service_should_start.append(srv)
Line 83: 
Line 84:         args = ''
Line 85:         if force:
Line 86:             args = '--force'
consider:

 args = '--force' if force else ''
Line 87: 
Line 88:         self._exec_libvirt_configure("reconfigure", args)
Line 89: 
Line 90:         for srv in reversed(service_should_start):


Line 84:         args = ''
Line 85:         if force:
Line 86:             args = '--force'
Line 87: 
Line 88:         self._exec_libvirt_configure("reconfigure", args)
or consider:

 self._exec_libvirt_configure(
     "reconfigure",
     '--force' if force else '',
 )

but I thought the --force is to restart services...

I thought I've commented the algorithm.... in previous comment.

you check if you should configure, if not you do nothing, if yes and services is up and no force you exit with error, then if force you stop services, configure, start stopped services.

this should be applied to all modules as a *SINGLE* transaction.
Line 89: 
Line 90:         for srv in reversed(service_should_start):
Line 91:             service.service_start(srv)
Line 92: 


Line 87: 
Line 88:         self._exec_libvirt_configure("reconfigure", args)
Line 89: 
Line 90:         for srv in reversed(service_should_start):
Line 91:             service.service_start(srv)
missing return
Line 92: 
Line 93:     def validate(self):
Line 94:         """
Line 95:         Validate conflict in configured files


Line 155:         return 0
Line 156: 
Line 157: 
Line 158: # List all available configurers
Line 159: configurers = [
underscore, as it is module private?
Line 160:     LibvirtModuleConfigure(),
Line 161:     SanlockModuleConfigure()
Line 162: ]
Line 163: 


Line 157: 
Line 158: # List all available configurers
Line 159: configurers = [
Line 160:     LibvirtModuleConfigure(),
Line 161:     SanlockModuleConfigure()
comma
Line 162: ]
Line 163: 
Line 164: 
Line 165: @vdsm.tool.expose("configure")


Line 158: # List all available configurers
Line 159: configurers = [
Line 160:     LibvirtModuleConfigure(),
Line 161:     SanlockModuleConfigure()
Line 162: ]
I think this can be tupple
Line 163: 
Line 164: 
Line 165: @vdsm.tool.expose("configure")
Line 166: def configure():


Line 216:                         help='Specify the module to configure '
Line 217:                              '(e.g libvirt,sanlock ..).\n'
Line 218:                              'If non is specified, operation will run for '
Line 219:                              'all related modules')
Line 220:     parser.add_argument('--dry', dest='dry', default=False,
all these dry and test, we discussed, need to find something cleaner.
Line 221:                         action='store_true',
Line 222:                         help='Dry run, prints if configure is required')
Line 223:     parser.add_argument('--test', dest='test', default=False,
Line 224:                         action='store_true',


-- 
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: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alonbl at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list