Change in vdsm[master]: Move multipath configuration to vdsm-tool configurator
nsoffer at redhat.com
nsoffer at redhat.com
Thu Oct 2 18:57:16 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: Move multipath configuration to vdsm-tool configurator
......................................................................
Patch Set 13:
(3 comments)
I think we are going in the wrong direction. This patch should be simple move of multipath code as is to vdsm-tool.
Any change in this code will be very hard to review later, because of the code moves.
We can always improve the details later.
Regarding the backup change - it is the worst idea in this patch - mix code move an behavior change, and the behavior change is questionable.
http://gerrit.ovirt.org/#/c/30909/13/lib/vdsm/tool/configurators/multipath.py
File lib/vdsm/tool/configurators/multipath.py:
Line 101: during configuration.
Line 102: '''
Line 103: return []
Line 104:
Line 105: def _parseConf(self):
It would be nicer if the private function is at the end of the class, after the public module configure functions.
Line 106: first = second = ''
Line 107: with open(self._MPATH_CONF) as f:
Line 108: mpathconf = [x.strip("\n") for x in f.readlines()]
Line 109: try:
Line 124: first, second = self._parseConf()
Line 125: if not os.path.exists(self._CONF_BACKUP) and \
Line 126: self._MPATH_CONF_TAG not in first:
Line 127: cmd = [constants.EXT_CP, self._MPATH_CONF, self._CONF_BACKUP]
Line 128: rc, out, err = utils.execCmd(cmd)
If we cannot write multipath configuration file running as root, something is very wrong in this host, and we should fail hard.
Line 129: utils.persistFile(self._CONF_BACKUP)
Line 130:
Line 131: with tempfile.NamedTemporaryFile() as f:
Line 132: f.write(self._MPATH_CONF_TEMPLATE %
Line 138:
Line 139: if rc != 0:
Line 140: raise RuntimeError("Failed to perform Multipath config.")
Line 141: if utils.isOvirtNode():
Line 142: utils.persistFile(self._MPATH_CONF)
You don't need the ovirt node check, utils.presistFile does nothing now if we are not on ovirt node.
Line 143:
Line 144: # Flush all unused multipath device maps
Line 145: utils.execCmd([constants.EXT_MULTIPATH, "-F"])
Line 146:
--
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: 13
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