Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator

ybronhei at redhat.com ybronhei at redhat.com
Thu Mar 27 16:42:42 UTC 2014


Yaniv Bronhaim has posted comments on this change.

Change subject: Move multipath configuration to vdsm-tool configurator
......................................................................


Patch Set 2: Code-Review-1

(5 comments)

hope that during next week http://gerrit.ovirt.org/#/c/25263/ will be merged. and mooli will be able to show you how to test tools stuff. the verified ack should be only if tool tests exist as part of the additional configure class patch

http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/tool/configurator.py
File lib/vdsm/tool/configurator.py:

Line 22: import grp
Line 23: import argparse
Line 24: import tempfile
Line 25: 
Line 26: from .. import utils, constants
why? try to stick with your goal in this patch scope. you can propose this change in separate patch and ill be glad to discuss about it
Line 27: from . import service, expose
Line 28: 
Line 29: 
Line 30: class _ModuleConfigure(object):


Line 216: _scsi_id = utils.CommandPath("scsi_id",
Line 217:                          "/sbin/scsi_id",  # EL6
Line 218:                          "/usr/lib/udev/scsi_id",  # Fedora
Line 219:                          "/lib/udev/scsi_id",  # Ubuntu
Line 220:                          )
put all multipath related parameters as class globals
Line 221: 
Line 222: 
Line 223: class MultipathModuleConfigure(_ModuleConfigure):
Line 224: 


Line 306:                                      "upgrade required")
Line 307:                     return False
Line 308: 
Line 309:         sys.stdout.write("multipath Defaulting to False")
Line 310:         return False
check the rest of the interface, what about validate.. might be also useful. and keep in mind that reconfigureOnForce is very meaningful if you want to override old configuration on --force flag . if False (not), state it specifically
Line 311: 
Line 312: 
Line 313: __configurers = (
Line 314:     LibvirtModuleConfigure(),


http://gerrit.ovirt.org/#/c/26123/2/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 154:         else:
Line 155:             raise
Line 156: 
Line 157: 
Line 158: def rotateFiles(directory, prefixName, gen, cp=False, persist=False):
> this function is so old and dusty. would you break this big patch to smalle
good to have rotate to old conf files. +1 for use it in configurator (maybe even put this function there) and use it also for old libvirt related conf files (but in separate patch)
Line 159:     logging.debug("dir: %s, prefixName: %s, versions: %s" %
Line 160:                  (directory, prefixName, gen))
Line 161:     gen = int(gen)
Line 162:     files = os.listdir(directory)


Line 206:             try:
Line 207:                 execCmd([constants.EXT_PERSIST, newName], logErr=False,
Line 208:                         sudo=True)
Line 209:             except:
Line 210:                 pass
> omg
:)
Line 211: 
Line 212: 
Line 213: IPXMLRPCRequestHandler = SimpleXMLRPCRequestHandler
Line 214: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsland at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan 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