Alon Bar-Lev has posted comments on this change. Change subject: kdump: Add kdump plugin ......................................................................
Patch Set 1: (6 comments) quick initial 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' you probably need to query packages during setup phase so that engine will be notified via environment if there is a compatible version so it can choose to enable. do not use packager to detect rpm specific. query for packages and use the platform python module to determine what distribution, and check at that conditionals. turn off this feature if distribution is not supported. 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' please do not use constants for arguments. 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): this is way too complex... the place for error is huge, as we ignore whatever we do not know, so we end up with something not working. if this is the case, better to override the entire configuration. 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) please use regular expression of key, value groups. 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: ) if you query packages, you must also update the fake offline packager to return what 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')) you should set it to start at 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 <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
