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

ybronhei at redhat.com ybronhei at redhat.com
Tue Aug 5 11:40:45 UTC 2014


Yaniv Bronhaim 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"
> These newlines adds an empty line between the tags and the contents of the 
thanks! :) why couldn't you say it in my first comment
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"
> The usage of this tag is explained in the docstring of isconfigured().
move the explanation here please
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"]
> If we return ["multipathd"], then with systemd, vdsm will also stop, becaus
whatever you know is better than I know. just saying what this method means because I wasn't sure you understand.

if you test that returning empty list works as we expected, please do that.
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")
> anyway line 724 will cause multipathd restart (it will stop the service if 
this will be required if we remove multipathd from getServices. just import service and use the function directly instead of execCmd
Line 760: 
Line 761:     def isconfigured(self, *args):
Line 762:         """
Line 763:         Check the multipath daemon configuration. The configuration file


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
> If the administrator added "# RHEV PRIVATE" tag to multipath.conf, we are n
that exactly why i asked for comments near the titles! just explain the meaning of them..
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 802:         sys.stdout.write("multipath Defaulting to False\n")
Line 803:         return NOT_CONFIGURED
Line 804: 
Line 805:     def validate(self):
Line 806:         return True
> We don't have code for this. Implementing this requires parsing multipath c
np. just asking
Line 807: 
Line 808: 
Line 809: __configurers = (
Line 810:     LibvirtModuleConfigure(),


Line 837: 
Line 838:     services = []
Line 839:     for c in configurer_to_trigger:
Line 840:         for s in c.getServices():
Line 841:             if service.service_status(s, False) == 0:
> Stopping multipath is not required to configure it, and we are not doing it
right. so change it
Line 842:                 if not args.force:
Line 843:                     raise InvalidRun(
Line 844:                         "\n\nCannot configure while service '%s' is "
Line 845:                         "running.\n Stop the service manually or use the "


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