Yeela Kaplan has posted comments on this change. Change subject: Move multipath configuration to vdsm-tool configurator ......................................................................
Patch Set 3: (10 comments) 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 anot It's related because I'm using constants. Done. 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: This is an unrelated change to this patch. It should be done in a different patch if needed. 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: This is an unrelated change to this patch. It should be done in a different patch if needed. 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 Done 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. Done 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 conf This is an unrelated change to this patch. It should be done in a different patch if needed. 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 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 sh I don't see why the user needs to read vdsm code 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 Done 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: http://gerrit.ovirt.org/#/c/26123/3/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 1038: else: Line 1039: return OSName.UNKNOWN Line 1040: Line 1041: Line 1042: def isOvirtNode(): > please rebase on top on current master (which already has this function) Done Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): Line 1042: def isOvirtNode(): Line 1043: return getos() in (OSName.RHEVH, OSName.OVIRT) Line 1044: Line 1045: Line 1046: def persistFile(name): > please introduce this function in its own patch. it should not ignore error Done Line 1047: if isOvirtNode(): Line 1048: execCmd([constants.EXT_PERSIST, name], sudo=True) Line 1049: Line 1050: -- 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
