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

mtayer at redhat.com mtayer at redhat.com
Thu May 8 11:25:32 UTC 2014


mooli tayer has posted comments on this change.

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


Patch Set 4:

(6 comments)

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

Line 149:         'require_lease_for_disks': 0,
Line 150:     }
Line 151: 
Line 152:     # libvirt log rotate configuration
Line 153:     LLOGR_CONF = (
> why don't you embed this as file at /usr/share instead of hardcode?
A hint on how to do this?
Line 154:         '/var/log/libvirt/libvirtd.log {'
Line 155:         '    rotate 100'
Line 156:         '    missingok'
Line 157:         '    copytruncate'


Line 158:         '    size 15M'
Line 159:         '    compress'
Line 160:         '    compresscmd /usr/bin/xz'
Line 161:         '    uncompresscmd /usr/bin/unxz'
Line 162:         '    compressext .xz'
> I hope we have xz dependency at spec...
Yes we do.
Line 163:         '}'
Line 164:     )
Line 165: 
Line 166:     def __init__(self):


Line 218:         if SANLOCK_ENABLED:
Line 219:             qconf_map.update(self.QCONF_SANLOCK)
Line 220:             qlconf_map.update(self.QLCONF_SANLOCK)
Line 221:         if not LIBVIRT_SELINUX:
Line 222:             qconf_map.update(self.QCONF_NO_SELINUX)
> I do not see any reason to put one time used constants...
Ok I'm giving this a try.
Line 223: 
Line 224:         # write configuration
Line 225:         for file_name, configuration in [
Line 226:             (self.envGet('LCONF'), lconf_map),


Line 240:                 self.envGet('LRCONF'), **VDSM_CONF_SECTION) as conf:
Line 241:             conf.prefixLines('# VDSM backup')
Line 242:             conf.prependSection(self.LLOGR_CONF)
Line 243: 
Line 244:         if utils.isOvirtNode() and ovirtfunctions:
> it cannot be that it is node and functions are missing.
Done
Line 245:             for fname in (
Line 246:                 self.envGet('LCONF'),
Line 247:                 self.envGet('QCONF'),
Line 248:                 self.envGet('LDCONF'),


Line 260:         crashed.
Line 261:         """
Line 262:         INITCTL = '/sbin/initctl'
Line 263:         LIBVIRTD_UPSTART = 'libvirtd.upstart'
Line 264:         TARGET = "/etc/init/libvirtd.conf"
> etc should be sysconfdir from autoconf
Done
Line 265:         if os.path.isfile(INITCTL) and os.access(INITCTL, os.X_OK):
Line 266:             ts = rpm.TransactionSet()
Line 267:             mi = itertools.chain(*[ts.dbMatch('name', name)
Line 268:                                    for name in ['libvirt', 'libvirt-daemon']])


http://gerrit.ovirt.org/#/c/27298/4/tests/toolTests.py
File tests/toolTests.py:

Line 56:             "keepalive_interval=-1\n"
Line 57:             "log_outputs=\"1:file:/var/log/libvirt/libvirtd.log\"\n"
Line 58:             "ca_file=\"/etc/pki/vdsm/certs/cacert.pem\"\n"
Line 59:             "cert_file=\"/etc/pki/vdsm/certs/vdsmcert.pem\"\n"
Line 60:             "## end of configuration section by vdsm-4.13.0\n"
> I would have put all these in separate files out of the script
Done
Line 61:         ),
Line 62: 
Line 63:         "qconf_ssl": (
Line 64:             "## beginning of configuration section by vdsm-4.13.0\n"


-- 
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: 4
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