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 <ykap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
