Nir Soffer has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator ......................................................................
Patch Set 3: (10 comments) To make it easier to review, please separate patches where you remove code without any change (except changes required by the movement), and patches were you modify the code. http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/tool/configurator.py File lib/vdsm/tool/configurator.py: Line 23 Line 24 Line 25 Line 26 Line 27 This change is good, but not related to this patch. Please separate to another patch before or after this patch. Line 219: " product \"Compellent Vol\"\n" Line 220: " no_path_retry fail\n" Line 221: "}\n" Line 222: "}" Line 223: ) Please use multi-line string literals for stuff like this: _STRG_MPATH_CONF = """ defaults { blah yes } ... """ Line 224: Line 225: _OLD_TAGS = ["# RHAT REVISION 0.2", "# RHEV REVISION 0.3", Line 226: "# RHEV REVISION 0.4", "# RHEV REVISION 0.5", Line 227: "# RHEV REVISION 0.6", "# RHEV REVISION 0.7", Line 224: Line 225: _OLD_TAGS = ["# RHAT REVISION 0.2", "# RHEV REVISION 0.3", Line 226: "# RHEV REVISION 0.4", "# RHEV REVISION 0.5", Line 227: "# RHEV REVISION 0.6", "# RHEV REVISION 0.7", Line 228: "# RHEV REVISION 0.8", "# RHEV REVISION 0.9"] This is too much duplication here. Please replace with something like: _OLD_TAGS = tuple("# RHAT REVISION 0.%d" % n for n in range(2, 9)) This will also fix the pep8 issues in the current code. Line 229: _MPATH_CONF_TAG = "# RHEV REVISION 1.0" Line 230: _MPATH_CONF_PRIVATE_TAG = "# RHEV PRIVATE" Line 231: Line 232: _MPATH_CONF_TEMPLATE = _MPATH_CONF_TAG + _STRG_MPATH_CONF Line 240: ) Line 241: Line 242: Line 243: def __init__(self): Line 244: super(MultipathModuleConfigure, self).__init__() This does nothing, please remove. Not implementing this will call the super implementation. I know this error is common in this file but this is not a reason to repeat it. Line 245: Line 246: def getName(self): Line 247: return 'multipath' Line 248: Line 257: if os.getuid() != 0: Line 258: raise UserWarning("Must run as root") Line 259: if self.isconfigured(): Line 260: return Line 261: if os.path.exists(MultipathModuleConfigure._MPATH_CONF): Use self. Line 262: utils.rotateFiles( Line 263: os.path.dirname(MultipathModuleConfigure._MPATH_CONF), Line 264: os.path.basename(MultipathModuleConfigure._MPATH_CONF), Line 265: MultipathModuleConfigure._MAX_CONF_COPIES, Line 276: raise RuntimeError("Failed to perform Multipath config.") Line 277: utils.persistFile(MultipathModuleConfigure._MPATH_CONF) Line 278: Line 279: # Flush all unused multipath device maps Line 280: utils.execCmd([constants.EXT_MULTIPATH, "-F"]) Not sure this is needed, since multipath should do this when reloading configuration. For another patch. Line 281: Line 282: cmd = [constants.EXT_VDSM_TOOL, "service-reload", "multipathd"] Line 283: rc, out, err = utils.execCmd(cmd) Line 284: if rc != 0: Line 281: Line 282: cmd = [constants.EXT_VDSM_TOOL, "service-reload", "multipathd"] Line 283: rc, out, err = utils.execCmd(cmd) Line 284: if rc != 0: Line 285: raise RuntimeError("Failed to reload Multipath.") This assumes that multipathd is running, but this is not the case when using vdsm-tool configure before vdsm is started in the first time. If is not running there is no need to reload, because it will pick the new configuration when it is started by vdsm later. So we should reload only if multipathd is running. Line 286: Line 287: def isconfigured(self, *args): Line 288: """ Line 289: Check the multipath daemon configuration. The configuration file Line 289: Check the multipath daemon configuration. The configuration file Line 290: /etc/multipath.conf should contain private tag in form Line 291: "RHEV REVISION X.Y" for this check to succeed. Line 292: If the tag above is followed by tag "RHEV PRIVATE" the configuration Line 293: should be preserved at all cost. This information must be in a header in the configuration file. The user should not read vdsm code to understand this mechanism. Line 294: Line 295: """ Line 296: if os.getuid() != 0: Line 297: raise UserWarning("Must run as root") Line 295: """ Line 296: if os.getuid() != 0: Line 297: raise UserWarning("Must run as root") Line 298: Line 299: if os.path.exists(MultipathModuleConfigure.MPATH_CONF): Use self instead of the class name. Otherwise you will have to modify this code when you change the class name. Line 300: first = second = '' Line 301: with open(MultipathModuleConfigure.MPATH_CONF) as f: Line 302: mpathconf = [x.strip("\n") for x in f.readlines()] Line 303: try: Line 311: if MultipathModuleConfigure.MPATH_CONF_TAG not in first: Line 312: sys.stdout.write("This manual override for multipath.conf " Line 313: "was based on downrevved template. " Line 314: "You are strongly advised to " Line 315: "contact your support representatives") Requiring that the first two lines are tags is too fragile, but improving it is for another patch. Line 316: return True Line 317: Line 318: if MultipathModuleConfigure.MPATH_CONF_TAG in first: Line 319: sys.stdout.write("Current revision of multipath.conf detected," -- To view, visit http://gerrit.ovirt.org/26123 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40f802477e39000c5cae01a496ac2d9f879ebfa8 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: mooli tayer <mta...@redhat.com> 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
