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

nsoffer at redhat.com nsoffer at redhat.com
Mon Aug 4 10:50:17 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 2: Code-Review-1

(7 comments)

There are some issues in the old code from multipath that must be changed in this patch, otherwise this patch is not correct.

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"
> do we need those 2 blank lines?
Yaniv, this is code moved from storage/multipath.py - we don't want to change it in this patch.

There is lot of cleanup that we can do, but in a separate future patch. We want to get this patch in 3.5 as soon as possible.
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"
> any reason for the version number? maybe add comment that explains it as in
Again, code moved from multipath, don't touch it in this patch.
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"]
> this means what services need to restart after configuring multipath. not s
In systemd, vdsm depends on multipath, so it will be restarted when you restart multipath. Do we have this logic in the vdsm-tool or we rstart services manually?

But we don't need to restart multipath since we reload it in configure()
Line 725: 
Line 726:     def configure(self):
Line 727:         """
Line 728:         Set up the multipath daemon configuration to the known and


Line 750:         if utils.isOvirtNode():
Line 751:             NodeCfg().persist(self._MPATH_CONF)
Line 752: 
Line 753:         # Flush all unused multipath device maps
Line 754:         utils.execCmd([constants.EXT_MULTIPATH, "-F"])
In a later patch, we should check if this is really needed, and fix error handling. As Dan say, if you don't care that it failed, why do you run it?

When return value is not checked, the code must explain why.
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 752: 
Line 753:         # Flush all unused multipath device maps
Line 754:         utils.execCmd([constants.EXT_MULTIPATH, "-F"])
Line 755: 
Line 756:         cmd = [constants.EXT_VDSM_TOOL, "service-reload", "multipathd"]
We are running in vdsm tool :-)
Line 757:         rc, out, err = utils.execCmd(cmd)
Line 758:         if rc != 0:
Line 759:             sys.stdout.write("Failed to reload Multipath.\n")
Line 760: 


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:
we don't need to restart multipathd after configuration, reload is enough for multipathd.

Yeela:
This will fail on the first vdsm install, because multipathd is not running, logging bogus error to stdout, while nothing was failed.

If this fail later, when multipathd *is* running, we are hidding a fatal error that must cause the tool to fail loudly.

The correct logic for reloading multipath is:

    reload multipathd
    if failed, check if multipathd is running
    if running, fail loudly

I think I already commented about this in an older patch.

This must be fixed in *this* patch, as the code moved form multipath.py is not correct, assuming that multipath is always running.
Line 760: 
Line 761:     def isconfigured(self, *args):
Line 762:         """
Line 763:         Check the multipath daemon configuration. The configuration file


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:
Yaniv: does this mean that we cannot configure multiapthd while it is running?

There is no problem in doing that from the multipathd point of view. Not sure if we like to change multipathd configuration while vdsm is running.
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