Alon Bar-Lev has posted comments on this change. Change subject: kdump: Add kdump plugin ......................................................................
Patch Set 1: (6 comments) http://gerrit.ovirt.org/#/c/28385/1/src/plugins/ovirt-host-deploy/kdump/packages.py File src/plugins/ovirt-host-deploy/kdump/packages.py: Line 51: _KEXEC_TOOLS_RPM = 'kexec-tools' Line 52: Line 53: # min version of kexec-tools per distribution Line 54: _FC20_MIN_VER = '2.0.4-27.fc20' Line 55: _EL6_MIN_VER = '2.0.0-274.el6' > Well, kdump detection feature is enabled via checkbox in Power Management s you can perform the detection before, so this process is done automatically. Line 56: Line 57: # prefixes marking changes by oVirt Line 58: _OVIRT_BACKUP_NODES = '# oVirt backup: fence_kdump_nodes ' Line 59: _OVIRT_BACKUP_ARGS = '# oVirt backup: fence_kdump_args ' Line 66: _ARG_PORT = '-p' Line 67: _ARG_INTERVAL = '-i' Line 68: _ARG_COUNT = '-c' Line 69: _ARG_FAMILY = '-f' Line 70: _ARG_VERBOSE = '-v' > So even they are use on several places, do you want me to use in place stri yes... much easier to understand what happening. Line 71: Line 72: # arguments default values Line 73: _DEFAULT_VALUES = { Line 74: _ARG_PORT: '7410', Line 122: self.logger.debug("New nodes lines: '%s'", new_nodes) Line 123: Line 124: return new_nodes Line 125: Line 126: def _parse_fence_kdump_args(self, args_line): > I don't understand here: I think that it will be better to override: 1. read original arguments comment 2. put original arguments within comment if no (1) 3. put our own no need to parse anything Line 127: parser = optparse.OptionParser() Line 128: parser.add_option( Line 129: self._ARG_PORT, Line 130: '--ipport', Line 250: args_line = line Line 251: elif line.startswith(self._OVIRT_BACKUP_ARGS): Line 252: args_backup_line = line Line 253: else: Line 254: new_content.append(line) > Do you mean: for each line to match regex instead of startswith? Or somethi yes. Line 255: Line 256: # configure fence_kdump for oVirt Line 257: new_content.extend( Line 258: self._configure_fence_kdump_nodes( Line 297: ) Line 298: def _validation(self): Line 299: result = self.packager.queryPackages( Line 300: patterns=(self._KEXEC_TOOLS_RPM,), Line 301: ) > I don't understand here. What "fake offline packager"? see: ./src/plugins/ovirt-host-deploy/core/offlinepackager.py in ovirt-node we cannot install anything, nor query yum, so we fake response of packager so we either have or do not have the package. you should also add dependency to ovirt-host-deploy-offline.spec.in to pull whatever you need. Line 302: if not result: Line 303: raise RuntimeError( Line 304: _( Line 305: 'Cannot locate kexec-tools package' Line 374: stage=plugin.Stages.STAGE_CLOSEUP, Line 375: condition=lambda self: self._enabled Line 376: ) Line 377: def _closeup(self): Line 378: self.logger.info(_('Restarting kdump')) > No, restarting kdump service is required after each configuration change, b right... but you should also mark it to be started after boot... Line 379: for state in (False, True): Line 380: self.services.state('kdump', state) Line 381: -- 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: 1 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
