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

ybronhei at redhat.com ybronhei at redhat.com
Sun Jun 1 07:45:11 UTC 2014


Yaniv Bronhaim has posted comments on this change.

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


Patch Set 20: Code-Review-1

(15 comments)

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

Line 78: 
Line 79:     def getName(self):
Line 80:         return 'libvirt'
Line 81: 
Line 82:     def getFile(self, fname):
should be private
Line 83:         return self.FILES[fname]['path']
Line 84: 
Line 85:     def getServices(self):
Line 86:         return ["supervdsmd", "vdsmd", "libvirtd"]


Line 137:                 ret = False
Line 138:         if not ret:
Line 139:             sys.stdout.write("libvirt is not configured for vdsm yet\n")
Line 140:         else:
Line 141:             sys.stdout.write("libvirt is already configured for vdsm\n")
same as the print of "configure", the verb prints it, so here its redundant. please remove
Line 142:         return ret
Line 143: 
Line 144:     def removeConf(self):
Line 145:         for cfile, content in LibvirtModuleConfigure.FILES.items():


Line 163: 
Line 164:     def _sysvToUpstart(self):
Line 165:         """
Line 166:         On RHEL 6, libvirtd can be started by either SysV init or Upstart.
Line 167:         We prefer upstart because it respawns libvirtd if when libvirtd
remove the "if" or the "when"
Line 168:         crashed.
Line 169:         """
Line 170:         def iterateLibvirtFiles():
Line 171:             ts = rpm.TransactionSet()


Line 173:                 for matches in ts.dbMatch('name', name):
Line 174:                     for filename in matches[rpm.RPMTAG_FILENAMES]:
Line 175:                         yield filename
Line 176: 
Line 177:         INITCTL = '/sbin/initctl'
we have declaration for this in service.py (_INITCTL), make it accessible
Line 178:         LIBVIRTD_UPSTART = 'libvirtd.upstart'
Line 179:         TARGET = os.path.join(SYSCONF_PATH, "init/libvirtd.conf")
Line 180: 
Line 181:         if os.path.isfile(INITCTL) and os.access(INITCTL, os.X_OK):


Line 200:                     if (oldmod is not None and
Line 201:                             oldmod != os.stat(TARGET).st_mode):
Line 202:                         os.chmod(TARGET, oldmod)
Line 203:                     rc, out, err = utils.execCmd((INITCTL,
Line 204:                                                   "reload-configuration"))
can't we use service.py for that?
Line 205:                     if rc != 0:
Line 206:                         sys.stdout.write(out)
Line 207:                         sys.stderr.write(err)
Line 208:                         raise RuntimeError(


Line 209:                             "Failed to reload upstart configuration.")
Line 210: 
Line 211:     def _isSslConflict(self):
Line 212:         """
Line 213:         return True iff libvirt files ssl configuration match vdsm.conf
return True if libvirt configuration files match ssl configuration of vdsm.conf
Line 214:         """
Line 215:         config.read(self.getFile('VDSM_CONF'))
Line 216:         ssl = config.getboolean('vars', 'ssl')
Line 217: 


Line 257:         return ret
Line 258: 
Line 259:     def _isApplicable(self, fragment, vdsmConfiguration):
Line 260:             """
Line 261:             Return true iff this fragment should be included for current
return True if "fragment" ..
Line 262:             configuration. An applicaple fragment is a fragment who's list
Line 263:             of conditions are met according to vdsmConfiguration.
Line 264:             """
Line 265:             applyFragment = True


Line 305:         try:
Line 306:             os.remove(content['path'])
Line 307:         except OSError as e:
Line 308:             if e.errno != errno.ENOENT:
Line 309:                 raise
use utils.rmFile
Line 310: 
Line 311:     # On removeConf functions
Line 312:     def _unprefixAndRemoveSection(self, path):
Line 313:         """


Line 307:         except OSError as e:
Line 308:             if e.errno != errno.ENOENT:
Line 309:                 raise
Line 310: 
Line 311:     # On removeConf functions
I don't understand those comments (On removeConf functions, On configure functions). I think its redundant . I'll read "configure" and see what it uses
Line 312:     def _unprefixAndRemoveSection(self, path):
Line 313:         """
Line 314:         undo changes done by _prefixAndPrepend.
Line 315:         """


Line 329: 
Line 330:     # version != PACKAGE_VERSION since we do not want to update configuration
Line 331:     # on every update. see 'configuration versioning:' at ConfigFile.py for
Line 332:     # details.
Line 333:     CONF_VERSION = '4.13.0'
configfile.py ? I don't see more details there..
Line 334: 
Line 335:     PKI_DIR = os.path.join(SYSCONF_PATH, 'pki/vdsm')
Line 336:     CA_FILE = os.path.join(PKI_DIR, 'certs/cacert.pem')
Line 337:     CERT_FILE = os.path.join(PKI_DIR, 'certs/vdsmcert.pem')


Line 352:             'removeConf': lambda x, y: True,
Line 353:             'persisted': False,
Line 354:         },
Line 355: 
Line 356:         # Libvirt daemon configuration
its quite obvious what configuration the file refers to
Line 357:         'LCONF': {
Line 358:             'path': os.path.join(
Line 359:                 SYSCONF_PATH,
Line 360:                 'libvirt/libvirtd.conf'


Line 785:     failed = False
Line 786:     for c in __configurers:
Line 787:         if c.getName() in args.modules:
Line 788:             try:
Line 789:                 c.removeConf()
you can add "removed configuration of module xxx successfully" for each iteration
Line 790:             except Exception:
Line 791:                 sys.stderr.write('cannot remove %s\n' % c.getName())
Line 792:                 traceback.print_exc(file=sys.stderr)
Line 793:                 failed = True


Line 787:         if c.getName() in args.modules:
Line 788:             try:
Line 789:                 c.removeConf()
Line 790:             except Exception:
Line 791:                 sys.stderr.write('cannot remove %s\n' % c.getName())
"can't remove configuration of module xxx " would be righter
Line 792:                 traceback.print_exc(file=sys.stderr)
Line 793:                 failed = True
Line 794:     if failed:
Line 795:         raise InvalidRun("Remove configuration failed")


http://gerrit.ovirt.org/#/c/27298/20/vdsm.spec.in
File vdsm.spec.in:

Line 752: %endif
Line 753: 
Line 754: %preun
Line 755: if [ "$1" -eq 0 ]; then
Line 756:         %{_bindir}/vdsm-tool remove-config
i think remove-config deserves --force flag, to not allow remove config while service is running except on force which will might require service restart afterwards. but we can delay it for now imo, anyway we need to add remove-config for sanlock
Line 757: fi
Line 758: 
Line 759: %if ! 0%{?with_systemd}
Line 760: if [ "$1" -eq 0 ]


Line 769: 
Line 770:     %{_bindir}/vdsm-tool sebool-unconfig || :
Line 771: 
Line 772:     /usr/sbin/saslpasswd2 -p -a libvirt -d vdsm at ovirt
Line 773: 
what about using the new verb you introduce in that patch - remove-saslpasswd ?
Line 774:     if /sbin/initctl status libvirtd >/dev/null 2>/dev/null ; then
Line 775:         /sbin/initctl stop libvirtd  >/dev/null 2>/dev/null
Line 776:         rm -f /etc/init/libvirtd.conf
Line 777: 


-- 
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: 20
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: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.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