Alon Bar-Lev has posted comments on this change. Change subject: kdump: Add kdump plugin ......................................................................
Patch Set 2: (13 comments) http://gerrit.ovirt.org/#/c/28385/2/src/plugins/ovirt-host-deploy/kdump/packages.py File src/plugins/ovirt-host-deploy/kdump/packages.py: Line 49: KdumpEnv.MESSAGE_INTERVAL -- interval between fence_kdump messages Line 50: Line 51: """ Line 52: Line 53: _KEXEC_TOOLS_RPM = 'kexec-tools' s/RPM/PACKAGE/ as we do like to support other distributions, Line 54: Line 55: # pattern to match lines with fence_kdump configuration Line 56: _FK_OPTS_REGEX = re.compile( Line 57: flags=re.VERBOSE, Line 56: _FK_OPTS_REGEX = re.compile( Line 57: flags=re.VERBOSE, Line 58: pattern=r""" Line 59: ^ Line 60: (?P<key>(\#\ oVirt\ backup\:\ )?fence_kdump_(nodes|args)) I do not understand why you parse the comment... Line 61: [ ] Line 62: (?P<value>.*) Line 63: $ Line 64: """ Line 57: flags=re.VERBOSE, Line 58: pattern=r""" Line 59: ^ Line 60: (?P<key>(\#\ oVirt\ backup\:\ )?fence_kdump_(nodes|args)) Line 61: [ ] \s ? Line 62: (?P<value>.*) Line 63: $ Line 64: """ Line 65: ) Line 85: full_distribution_name=0 Line 86: ) Line 87: Line 88: if name in ('redhat', 'centos') and version.startswith('6.'): Line 89: return '2.0.0-274.el6' check only minimum version, drop the el6, what you care is that you have your minimum version, not more than that. Line 90: Line 91: elif name == 'fedora' and version == '20': Line 92: return '2.0.4-27.fc20' Line 93: Line 88: if name in ('redhat', 'centos') and version.startswith('6.'): Line 89: return '2.0.0-274.el6' Line 90: Line 91: elif name == 'fedora' and version == '20': Line 92: return '2.0.4-27.fc20' same here... no sense checking fedora-20, only if version is >= what we need. Line 93: Line 94: raise RuntimeError( Line 95: _( Line 96: 'Kdump detection is not currently supported in: ' Line 90: Line 91: elif name == 'fedora' and version == '20': Line 92: return '2.0.4-27.fc20' Line 93: Line 94: raise RuntimeError( detection phase should not abort setup. Line 95: _( Line 96: 'Kdump detection is not currently supported in: ' Line 97: '{name} {version}' Line 98: ).format( Line 156: ) Line 157: Line 158: self.logger.debug("New nodes lines: '%s'", new_nodes) Line 159: Line 160: return new_nodes I do not understand this function and why it is that complex. please send me example of a file before and after configuration. it should be simple algorithm of inject comment and replace a line. Line 161: Line 162: def _configure_fence_kdump_args( Line 163: self, Line 164: port, Line 200: ) Line 201: Line 202: self.logger.debug("New args lines: '%s'", new_args) Line 203: Line 204: return new_args same here, please send me examples. Line 205: Line 206: def _update_kdump_conf(self, content): Line 207: new_content = [] Line 208: fk_options = {} Line 242: ) Line 243: ) Line 244: Line 245: self.logger.debug("Updated kdump.conf: '%s'", new_content) Line 246: return new_content well... even more complex.... need to understand why update of single file is that complex. Line 247: Line 248: @plugin.event( Line 249: stage=plugin.Stages.STAGE_INIT, Line 250: ) Line 248: @plugin.event( Line 249: stage=plugin.Stages.STAGE_INIT, Line 250: ) Line 251: def _setup(self): Line 252: self.environment[odeploycons.KdumpEnv.ENABLE] = False again... please add: plugin.Stages.STAGE_CUSTOMIZATION priority HIGH perform detection of kdump version, set in environment that it is supported for engine to know and enable if required. Line 253: Line 254: @plugin.event( Line 255: stage=plugin.Stages.STAGE_VALIDATION, Line 256: condition=lambda self: ( Line 259: ) Line 260: def _validation(self): Line 261: result = self.packager.queryPackages( Line 262: patterns=(self._KEXEC_TOOLS_RPM,), Line 263: ) this should be done before customization stage Line 264: if not result: Line 265: raise RuntimeError( Line 266: _( Line 267: 'Cannot locate kexec-tools package' Line 285: ).format( Line 286: minimum=min_version, Line 287: version=cur_version, Line 288: ) Line 289: ) there will be no error as will be not supported. Line 290: self._enabled = True Line 291: Line 292: @plugin.event( Line 293: stage=plugin.Stages.STAGE_PACKAGES, Line 306: with open(odeploycons.FileLocations.KDUMP_CONFIG_FILE, 'r') as f: Line 307: content = f.read().splitlines() Line 308: new_content = self._update_kdump_conf(content) Line 309: if content != new_content: Line 310: self.environment[ actually, there is no problem to perform transaction if content was not modified, it is detected automatically... Line 311: otopicons.CoreEnv.MAIN_TRANSACTION Line 312: ].append( Line 313: filetransaction.FileTransaction( Line 314: name=odeploycons.FileLocations.KDUMP_CONFIG_FILE, -- To view, visit http://gerrit.ovirt.org/28385 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idda48cb053c7e8747de5434c3681403f739c06b1 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-host-deploy Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
