Change in vdsm[master]: replace configure_libvirt.py with python code.

ybronhei at redhat.com ybronhei at redhat.com
Mon May 5 13:05:07 UTC 2014


Yaniv Bronhaim has posted comments on this change.

Change subject: replace configure_libvirt.py with python code.
......................................................................


Patch Set 3:

(8 comments)

love it ! thanks!! just few comments. please have more reviews now. i think its almost fully ready

http://gerrit.ovirt.org/#/c/27298/3/lib/vdsm/tool/Makefile.am
File lib/vdsm/tool/Makefile.am:

Line 31
Line 32
Line 33
Line 34
Line 35
remove all section


http://gerrit.ovirt.org/#/c/27298/3/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 176: VMCONF
leave it VDSMCONF


Line 195:                 raise RuntimeError(
Line 196:                     "vdsm: Missing certificate, vdsm not registered")
Line 197:             validate_ovirt_certs.validate_ovirt_certs()
Line 198:         # Remove a previous configuration (if present)
Line 199:         self.removeConf()
for backward compatibility .. but im not sure we want to remove the conf if exist... maybe on force
Line 200:         lconf_maps = [self.LCONF_GENERAL]
Line 201:         qconf_maps = [self.QCONF_GENERAL]
Line 202:         ldconf_maps = [self.LDCONF_GENERAL]
Line 203:         qlconf_maps = []


Line 209:                    [self.CA_FILE, self.CERT_FILE, self.KEY_FILE]):
Line 210:                 lconf_maps.append(self.LCONF_SSL)
Line 211:                 qconf_maps.append(self.QCONF_SSL_CERTS)
Line 212:             else:
Line 213:                 lconf_maps.append(self.LCONF_NO_SSL)
this will introduce conflict ^, you must have the certs if ssl=True, no ?
Line 214:         else:
Line 215:             qconf_maps.append(self.QCONF_NO_SSL)
Line 216:             lconf_maps.append(self.LCONF_NO_SSL)
Line 217:         if SANLOCK_ENABLED:


Line 237: 
Line 238:         with ConfigFile(
Line 239:                 self.envGet('LRCONF'), **VDSM_CONF_SECTION) as conf:
Line 240:             conf.prefixLines('# VDSM backup')
Line 241:             conf.prependSection(self.LLOGR_CONF)
maybe pass boolean if backup is required or not. it can lead to big conf files and might cause a regression
Line 242: 
Line 243:         for fname in (
Line 244:             self.envGet('LCONF'),
Line 245:             self.envGet('QCONF'),


Line 247:             self.envGet('QLCONF'),
Line 248:             self.envGet('LRCONF')
Line 249:         ):
Line 250:             if utils.isOvirtNode() and ovirtfunctions:
Line 251:                 ovirtfunctions.ovirt_store_config(fname)
put the condition before the for
Line 252:         sys.stdout.write("Reconfiguration of libvirt is done.")
Line 253: 
Line 254:     def sysvToUpstart(self):
Line 255:         """


Line 258:         crashed.
Line 259:         """
Line 260:         INITCTL = '/sbin/initctl'
Line 261:         LIBVIRTD_UPSTART = 'libvirtd.upstart'
Line 262:         TARGET = "/etc/init/libvirtd.conf"
private. add underscore ^
Line 263:         if os.path.isfile(INITCTL) and os.access(INITCTL, os.X_OK):
Line 264:             ts = rpm.TransactionSet()
Line 265:             mi = itertools.chain(*[ts.dbMatch('name', name)
Line 266:                                    for name in ['libvirt', 'libvirt-daemon']])


Line 296:             'listen_tcp': '0',
Line 297:             'auth_tcp': 'none'
Line 298:         })
Line 299:         lconf_p.read(self.envGet('LCONF'))
Line 300:         listen_tcp = lconf_p.getboolean('listen_tcp')
is getboolean really needed? you check if its 1 or 0
Line 301:         auth_tcp = lconf_p.get('auth_tcp')
Line 302:         qconf_p = ParserWrapper({'spice_tls': '0'})
Line 303:         qconf_p.read(self.envGet('QCONF'))
Line 304:         spice_tls = qconf_p.getboolean('spice_tls')


-- 
To view, visit http://gerrit.ovirt.org/27298
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I74bfe05bb4b5f5d09021f21b324f9b7d5d0fdaab
Gerrit-PatchSet: 3
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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list