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

ykaplan at redhat.com ykaplan at redhat.com
Tue Aug 5 14:07:29 UTC 2014


Yeela Kaplan has posted comments on this change.

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


Patch Set 2:

(7 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"
> excuse me to waste your time.. I think it worth a bit effort  and you'll ne
Yaniv, I am strongly against changing anything about the way the file is configured.

Even if it's just a blank line.

The goal of this patch is to move the configuration outside of vdsm and that's the only thing that should be verified now.

any other change should be done in a different patch.
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"
> move the explanation here please
Done
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"]
> correct. and btw, it won't start multipathd if it wasn't up when you call t
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")
> this will be required if we remove multipathd from getServices. just import
Done
Line 760: 
Line 761:     def isconfigured(self, *args):
Line 762:         """
Line 763:         Check the multipath daemon configuration. The configuration file


Line 766:         If the tag above is followed by tag "RHEV PRIVATE" the configuration
Line 767:         should be preserved at all cost.
Line 768:         """
Line 769:         if os.getuid() != 0:
Line 770:             raise NotRootError()
> do we really need to be root for that?
yes. reading multipath was previously done by supervdsmserver
Line 771: 
Line 772:         if os.path.exists(self._MPATH_CONF):
Line 773:             first = second = ''
Line 774:             with open(self._MPATH_CONF) as f:


Line 785:                     sys.stdout.write("This manual override for multipath.conf "
Line 786:                                      "was based on downrevved template. "
Line 787:                                      "You are strongly advised to "
Line 788:                                      "contact your support representatives\n")
Line 789:                 return CONFIGURED
> that exactly why i asked for comments near the titles! just explain the mea
Done
Line 790: 
Line 791:             if self._MPATH_CONF_TAG in first:
Line 792:                 sys.stdout.write("Current revision of multipath.conf detected,"
Line 793:                                  " preserving\n")


Line 798:                     sys.stdout.write("Downrev multipath.conf detected, "
Line 799:                                      "upgrade required\n")
Line 800:                     return NOT_CONFIGURED
Line 801: 
Line 802:         sys.stdout.write("multipath Defaulting to False\n")
> I think this will be more clear if we change the code to this:
Done
Line 803:         return NOT_CONFIGURED
Line 804: 
Line 805:     def validate(self):
Line 806:         return True


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