Martin Peřina 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' > you probably need to query packages during setup phase so that engine will Well, kdump detection feature is enabled via checkbox in Power Management settings of hosts. Currently required kexec-tools packages exists only for F20 and EL6 (EL7 patch was merged, but probably it will be included in 7.0.1 or 7.1). So current logic is user enables feature and if there's not required version, installation failes, user can turn off the feature and start installation again. 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. So even they are use on several places, do you want me to use in place strings? 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 whatev I don't understand here: 1) I parse existing args 2) Check if some of them should be updated for kdump detection feature to work properly 3) Save original arguments in comment starting with 'oVirt backup ...' 4) Insert line with new arguments The only optimization here is not set arguments if they contain default value. I could remove it, but IMO it makes result more readable ... 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. Do you mean: for each line to match regex instead of startswith? Or something else? 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 re I don't understand here. What "fake offline packager"? 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 No, restarting kdump service is required after each configuration change, because during restart initial ramdisk is created. 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
