mooli tayer has posted comments on this change. Change subject: replace configure_libvirt.py with python code. ......................................................................
Patch Set 5: (13 comments) The way I see things now, I think 'type' belongs on file and not on a config section. http://gerrit.ovirt.org/#/c/27298/5/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 37: SANLOCK_ENABLED, LIBVIRT_SELINUX Line 38: from vdsm.config import config Line 39: Line 40: Line 41: VDSM_CONF_SECTION = { > when do you decide what to put at class level and what as global? I have another patch that moves sanlock file stuff to SanlockModuleConfigure and then I will need this for both. for now I will move it to LibvirtModuleConfigure. Line 42: 'sectionStart': '## beginning of configuration section by vdsm', Line 43: 'sectionEnd': '## end of configuration section by vdsm', Line 44: 'version': '4.13.0' Line 45: } Line 41: VDSM_CONF_SECTION = { Line 42: 'sectionStart': '## beginning of configuration section by vdsm', Line 43: 'sectionEnd': '## end of configuration section by vdsm', Line 44: 'version': '4.13.0' Line 45: } > you removed the long and important comment: I've edit it a little bit to be more informative. see next version. (there is also info about it in configfile.py) Line 46: Line 47: Line 48: class _ModuleConfigure(object): Line 49: def __init__(self): Line 137: Line 138: # qemu configuration Line 139: { Line 140: 'condition': lambda context: Line 141: True, > use variable same as above Line 142: Line 143: 'conf': "QCONF", Line 144: 'content': { Line 145: 'dynamic_ownership': 0, Line 198: Line 199: # libvirt sysconfig file Line 200: { Line 201: 'condition': lambda x: Line 202: True, > why x ? put more meaning to the variables names I will rename all to context Line 203: Line 204: 'conf': "LDCONF", Line 205: 'content': { Line 206: 'LIBVIRTD_ARGS': '--listen', Line 210: Line 211: # sanlock configuration file Line 212: { Line 213: 'condition': lambda x: Line 214: SANLOCK_ENABLED, > I suggest that for consistency you move all into context or as I see now th Done Line 215: Line 216: 'conf': "QLCONF", Line 217: 'content': { Line 218: 'auto_disk_leases': 0, Line 251: 'QLCONF': 'libvirt/qemu-sanlock.conf', Line 252: 'LRCONF': 'logrotate.d/libvirtd', Line 253: 'QNETWORK': 'libvirt/qemu/networks/autostart/default.xml' Line 254: }[fname] Line 255: ) > what happened to the indentation ? Done Line 256: Line 257: def getServices(self): Line 258: return ["supervdsmd", "vdsmd", "libvirtd"] Line 259: Line 292: for item in self.CONF: Line 293: condition, conf, content = ( Line 294: item['condition'], item['conf'], item['content']) Line 295: if condition(c): Line 296: config_map[conf].update(content) > any reason to use extensive temp vars? OK Line 297: Line 298: for name, configuration in config_map.items(): Line 299: with ConfigFile(self.envGet(name), **VDSM_CONF_SECTION) as conff: Line 300: for key, val in configuration.items(): Line 301: conff.addEntry(key, val) Line 302: Line 303: qnetwork = self.envGet('QNETWORK') Line 304: if os.path.exists(qnetwork): Line 305: os.remove(qnetwork) > why is this an exception and cannot be done for all? Done Line 306: Line 307: # libvirt log rotate configuration Line 308: # Todo: make idempotent Line 309: with ConfigFile( Line 308: # Todo: make idempotent Line 309: with ConfigFile( Line 310: self.envGet('LRCONF'), **VDSM_CONF_SECTION) as conf: Line 311: conf.prefixLines('# VDSM backup') Line 312: conf.prependSection(self.LLOGR_CONF) > why is this exception? can we have dictionary entry of 'type' to prepend or Done Line 313: Line 314: if utils.isOvirtNode(): Line 315: from ovirtnode import ovirtfunctions Line 316: Line 320: self.envGet('LDCONF'), Line 321: self.envGet('QLCONF'), Line 322: self.envGet('LRCONF') Line 323: ): Line 324: ovirtfunctions.ovirt_store_config(fname) > please have a list/set of all files that were updated and iterate through t Ok Line 325: Line 326: sys.stdout.write("Reconfiguration of libvirt is done.") Line 327: Line 328: def _sysvToUpstart(self): Line 353: if not os.path.isfile(TARGET): Line 354: service.service_stop('libvirtd') Line 355: if not os.path.isfile(TARGET) or \ Line 356: not filecmp.cmp(packeged, TARGET): Line 357: shutil.copyfile(packeged, TARGET) > when you copy a file you need chmod as well. Do I need the mode of the packaged file or that of the old TARGET? Line 358: rc, out, err = utils.execCmd((INITCTL, Line 359: "reload-configuration")) Line 360: if rc != 0: Line 361: sys.stdout.write(out) Line 391: "libvirtd.conf: listen_tcp=0, auth_tcp=\"sasl\", \n" Line 392: "qemu.conf: spice_tls=1.\n" Line 393: ) Line 394: return False Line 395: else: > if you return at middle of function (look above) why do you need the else? Done Line 396: if listen_tcp == 1 and auth_tcp == 'none' and spice_tls == 0: Line 397: sys.stdout.write( Line 398: "SUCCESS: ssl configured to false. No conflicts.\n") Line 399: return True Line 433: self.envGet('LCONF'), Line 434: self.envGet('QCONF'), Line 435: self.envGet('LDCONF'), Line 436: self.envGet('QLCONF') Line 437: ): > if you already have this envGet, why don't you mark what is for configure a Done Line 438: if os.path.exists(path): Line 439: with ConfigFile(path, **VDSM_CONF_SECTION) as conff: Line 440: conff.removeConf() Line 441: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.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
