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

ybronhei at redhat.com ybronhei at redhat.com
Tue Aug 5 05:33:13 UTC 2014


Yaniv Bronhaim has posted comments on this change.

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


Patch Set 2:

(4 comments)

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

Line 670: 
Line 671:     _MPATH_CONF = "/etc/multipath.conf"
Line 672: 
Line 673:     _STRG_MPATH_CONF = (
Line 674:         "\n\n"
> Changing now these lines is a waste of time and make the review harder.  We
excuse me to waste your time.. I think it worth a bit effort  and you'll never touch it later (as you didn't touch the old way till now)
Line 675:         "defaults {\n"
Line 676:         "    polling_interval        5\n"
Line 677:         "    getuid_callout          \"%(scsi_id_path)s --whitelisted "
Line 678:         "--replace-whitespace --device=/dev/%%n\"\n"


Line 705:                  "# RHEV REVISION 0.4", "# RHEV REVISION 0.5",
Line 706:                  "# RHEV REVISION 0.6", "# RHEV REVISION 0.7",
Line 707:                  "# RHEV REVISION 0.8", "# RHEV REVISION 0.9"]
Line 708: 
Line 709:     _MPATH_CONF_TAG = "# RHEV REVISION 1.0"
> It does not matter now, we don't want to change the logic of the multipath 
again, matters to me. I didn't ask for any logic here, only a comment that explain your investigation about why we use this tag in the configuration. please try understand what I ask first.. I think its reasonable to ask for explanation and that it won't take you so long if you already understand this part (as I wish you do)
Line 710:     _MPATH_CONF_PRIVATE_TAG = "# RHEV PRIVATE"
Line 711: 
Line 712:     _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF
Line 713: 


Line 720:     def getName(self):
Line 721:         return 'multipath'
Line 722: 
Line 723:     def getServices(self):
Line 724:         return ["multipathd"]
> I think that stopping multipath while we configuring should be the safest w
correct. and btw, it won't start multipathd if it wasn't up when you call to configure. but are you sure vdsmd shouldn't restart? if not, this part is well done.
Line 725: 
Line 726:     def configure(self):
Line 727:         """
Line 728:         Set up the multipath daemon configuration to the known and


Line 755: 
Line 756:         cmd = [constants.EXT_VDSM_TOOL, "service-reload", "multipathd"]
Line 757:         rc, out, err = utils.execCmd(cmd)
Line 758:         if rc != 0:
Line 759:             sys.stdout.write("Failed to reload Multipath.\n")
> Yaniv:
anyway line 724 will cause multipathd restart (it will stop the service if up, configure it, and will start it afterwards. if the service was down, only configure it.

this part is not needed, and we do anything as this will be called when multipathd is down
Line 760: 
Line 761:     def isconfigured(self, *args):
Line 762:         """
Line 763:         Check the multipath daemon configuration. The configuration file


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife045908dc6e2aea9829b51482b909af1faf79da
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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: Yeela Kaplan <ykaplan 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