Kdump ansible integration #19430

jelly · 2023-10-04T12:51:09Z

This introduces the ability to generate an Ansible playbook from the curent kdump configuration.

The new view automation script button:

The role:

Overall the functionality is quite easy to implement except for one issue. Adding compression support, as the kdump role has no special flag indication that one wants compression we have to set a kdump_corecollector. By default a corecollector is set so we can use that. If for some reason it is absent, we have to make up what it should be which I find non-ideal.

When /etc/kdump.conf does not contain core_collector when adding compression we set makedumpfile -c, however if we then disable compression we generate an invalid configuration:

Related is, that once one start's messing with core_collector settings you no longer get anything representing the default. Also note that the default core_collector according to the man page should be (RHEL-8-10):

  | The default core_collector for raw/ssh dump is:
  | "makedumpfile -F -l --message-level 7 -d 31".
  | The default core_collector for other targets is:
  | "makedumpfile -l --message-level 7 -d 31".

Which the ansible role does not adhere too. So us setting kdump_corecollector is a good thing. However when it is absent due to an Administrator the handling is suboptimal.

I see two options:

We ignore customization from an administrator and keep the current behaviour
We use the default as suggested by the kdump man page, we do have to check if this is the same for RHEL 8 - 10 and Fedora. And keep up with potential changes from upstream.

Kdump: Add Ansible/shell automation

The Kdump page can generate an Ansible role or a shell script for replicating the current kdump configuration to other machines.

pkg/kdump/kdump-view.jsx

jelly · 2023-10-04T13:55:41Z

@martinpitt @marusak please take a look, I'm leaning towards teaching pkg/kdump/config-client.js what the default is.

Drafted a PR #19434

martinpitt · 2023-10-05T03:30:13Z

keep up with potential changes from upstream.

#19434 is still unclear to me, how it validates the actual default vs. the hardcoded one (it doesn't seem to do that at all). I would have thought it would take the default kdump.conf at the beginning of the test, parse the setting, and compare it to the changed one after clicking around. That would be acceptable to me.

We ignore customization from an administrator and keep the current behaviour

In the sense of "if we don't find a core_collector setting, we disable/hide the compression check box"? This would be preferable to me actually -- if an admin "customized" it in that way, we better not touch it and make matters worse.

jelly · 2023-10-05T07:09:00Z

keep up with potential changes from upstream.

#19434 is still unclear to me, how it validates the actual default vs. the hardcoded one (it doesn't seem to do that at all). I would have thought it would take the default kdump.conf at the beginning of the test, parse the setting, and compare it to the changed one after clicking around. That would be acceptable to me.

We ignore customization from an administrator and keep the current behaviour

In the sense of "if we don't find a core_collector setting, we disable/hide the compression check box"? This would be preferable to me actually -- if an admin "customized" it in that way, we better not touch it and make matters worse.

It's not just with compression, if you removed the core_collector entry we set just core_collector makedumpfile which makes kdump.service fail. So there is more broken which can be resolved with disabling compression.

test/verify/check-kdump

pkg/kdump/kdump-view.jsx

jelly · 2023-11-10T15:34:44Z

TestKdump is flaky, and I forgot to exclude OStree so I will do that in a fixup. But Fedora-38/Fedora-39 at least passes so it's good enough to review.

martinpitt

Initial review. Note that we should test the shell script as well. Thanks!

pkg/kdump/config-client.js

pkg/kdump/kdump-view.jsx

test/verify/check-kdump

jelly · 2023-11-13T11:34:51Z

Initial review. Note that we should test the shell script as well. Thanks!

We do that in TestKdump.testBasic. Maybe that is confusing?

jelly · 2023-11-14T14:58:18Z

Leftover issues:

RHEL 8/centos-8-stream kdump doesn't support setting boot params, switch to grubby?

> warning: Resolving coreutils failed: {"detail":"cannot update repo 'RHEL-BaseOS': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried; Last error: Curl error (6): Couldn't resolve host name for http://download.devel.redhat.com/rhel-8/nightly/RHEL-8/latest-RHEL-8.10.0/compose/BaseOS/x86_64/os/repodata/repomd.xml [Could not resolve host: download.devel.redhat.com]","code":64}
> warning: loading available updates failed: {"detail":"cannot update repo 'RHEL-BaseOS': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried; Last error: Curl error (6): Couldn't resolve host name for http://download.devel.redhat.com/rhel-8/nightly/RHEL-8/latest-RHEL-8.10.0/compose/BaseOS/x86_64/os/repodata/repomd.xml [Could not resolve host: download.devel.redhat.com]","code":64}
> warning: transport closed: disconnected
> error: TextArea: TextArea requires either an id or aria-label to be specified
> error: TextArea: TextArea requires either an id or aria-label to be specified
kdump: Usage: /usr/bin/kdumpctl {estimate|start|stop|status|restart|reload|rebuild|propagate|showmem}

jelly · 2023-11-14T19:25:26Z

@martinpitt do you have an idea what this might be?

  File "/work/bots/make-checkout-workdir/test/verify/check-kdump", line 520, in testAnsible
    b2.login_and_go("/kdump")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 802, in login_and_go
    self.try_login(user, password, superuser=superuser, legacy_authorized=legacy_authorized)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 768, in try_login
    self.wait_visible("#login")
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 626, in wait_visible
    self._wait_present(selector)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 617, in _wait_present
    self.wait_js_func('ph_is_present', selector)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 611, in wait_js_func
    self.wait_js_cond("%s(%s)" % (func, ','.join(map(jsquote, args))))
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 596, in wait_js_cond
    self.raise_cdp_exception("timeout\nwait_js_cond", cond, result["exceptionDetails"], trailer)
  File "/work/bots/make-checkout-workdir/test/common/testlib.py", line 300, in raise_cdp_exception
    raise Error("%s(%s): %s" % (func, arg, msg))
testlib.Error: timeout
wait_js_cond(ph_is_present("#login")): Uncaught (in promise) Error: condition did not become true

The related code is, so start_cockpit() ran but no screenshot was created? So the browser did not start?

        m2 = self.machines["nfs"]
        m2.start_cockpit()

        b2 = self.new_browser(m2)
        b2.login_and_go("/kdump")

test/verify/check-kdump

jelly · 2023-11-16T13:51:30Z

Debugged, squashed and re-pushed. Everything should go green now, code just needs a closer look :)

martinpitt

Thanks! More in-depth review.

pkg/kdump/kdump-view.jsx

test/verify/check-kdump

martinpitt

Thanks! Note that there are still two threads from the previous review which are current.

pkg/kdump/kdump-view.jsx

martinpitt · 2023-11-21T12:57:42Z

pkg/kdump/kdump-view.jsx

+    } else if (target === "nfs") {
+        ansible += `


yeah, weird -- why wouldn't this be covered by TestKdumpNFSAnsible?

pkg/kdump/kdump-view.jsx

test/verify/check-kdump

martinpitt · 2023-11-23T09:39:05Z

pkg/kdump/kdump-view.jsx

+    if (os_release.NAME === "RHEL" || os_release.ID_LIKE?.includes('rhel')) {
+        role_name = "rhel-system-roles";


that's legit, we only do coverage on Fedora.

martinpitt · 2023-11-23T09:39:41Z

pkg/kdump/kdump-view.jsx

+    if (target === "ssh") {
+        let ssh_user;
+        if (targetSettings.server.includes('@')) {
+            ssh_user = targetSettings.server.split('@')[0];


This is weird -- we currently have a test case which exercises "admin@ip"..

martinpitt · 2023-11-23T09:39:52Z

pkg/kdump/kdump-view.jsx

+        if (targetSettings.server.includes('@')) {
+            ssh_user = targetSettings.server.split('@')[0];
+        } else {
+            ssh_user = "root";


that's the missing test case (see the other thread)

martinpitt · 2023-11-23T09:40:39Z

pkg/kdump/kdump-view.jsx

+`;
+    } else if (target !== "local") {
+        // target is unsupported
+        throw new Error("Unsupported kdump target");


please add a // not-covered: assertion, it's not meant to be reached.

Extend the kdump page to allow administrators to configure kdump via Cockpit and export an Ansible playbook using the linux-system-roles kdump role.

pkg/kdump/kdump-view.jsx

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+  include_role:
+    name: ${role_name}.kdump
+  vars:
+    kdump_path: ${targetSettings.path || DEFAULT_KDUMP_PATH}


This added line is not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+    kdump_path: ${targetSettings.path || DEFAULT_KDUMP_PATH}
+    kdump_core_collector: ${kdump_core_collector}`;
+
+    if (target === "ssh") {


This added line is not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+        let ssh_user;
+        let ssh_server;
+        const parts = targetSettings.server.split('@');
+        if (parts.length === 1) {
+            ssh_user = "root";
+            ssh_server = parts[0];
+        } else if (parts.length === 2) {
+            ssh_user = parts[0];
+            ssh_server = parts[1];


These 9 added lines are not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+            ssh_user = parts[0];
+            ssh_server = parts[1];
+        } else {
+            throw new Error("ssh server contains two @ symbols");


This added line is not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+        } else {
+            throw new Error("ssh server contains two @ symbols");
+        }
+        ansible += `


This added line is not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+    kdump_sshkey: ${targetSettings.sshkey}
+    kdump_ssh_server: ${ssh_server}
+    kdump_ssh_user: ${ssh_user}`;
+    } else if (target === "nfs") {
+        ansible += `


These 5 added lines are not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+        ansible += `
+    kdump_target:
+      type: nfs
+      location: ${targetSettings.server}:${targetSettings.export}


This added line is not executed by any test. Details

cockpituous · 2023-11-24T11:45:55Z

pkg/kdump/kdump-view.jsx

+      type: nfs
+      location: ${targetSettings.server}:${targetSettings.export}
+`;
+    } else if (target !== "local") {


This added line is not executed by any test. Details

martinpitt

OK, enough bickering! There's that outdated comment, but it doesn't hurt much. If you want to update again because of that, I'm happy to re-approve.

I added a release-note label. Can you please massage the description and screenshot accordingly? Thanks!

martinpitt · 2023-11-24T14:29:29Z

pkg/kdump/kdump-view.jsx

+        // HACK: we should not have to specify kdump_ssh_user and kdump_ssh_user as it is in kdump_target.location
+        // https://github.com/linux-system-roles/kdump/issues/184


This comment is outdated now, as we don't actually specify .location any more.

I need to do a follow up anyway for self.createPackage, so I'll add it there.

jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 4, 2023

github-advanced-security bot found potential problems Oct 4, 2023

View reviewed changes

pkg/kdump/kdump-view.jsx Fixed Show fixed Hide fixed

jelly force-pushed the kdump-ansible-integration branch from fcc5dc3 to ec3ece5 Compare October 31, 2023 15:16

jelly force-pushed the kdump-ansible-integration branch from ec3ece5 to 657da11 Compare November 8, 2023 16:32

jelly commented Nov 9, 2023

View reviewed changes

test/verify/check-kdump Outdated Show resolved Hide resolved

jelly commented Nov 9, 2023

View reviewed changes

test/verify/check-kdump Outdated Show resolved Hide resolved

jelly commented Nov 9, 2023

View reviewed changes

pkg/kdump/kdump-view.jsx Show resolved Hide resolved

jelly force-pushed the kdump-ansible-integration branch 2 times, most recently from 1e940ef to 94c6deb Compare November 9, 2023 17:31

jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Nov 10, 2023

jelly force-pushed the kdump-ansible-integration branch 4 times, most recently from 636acb6 to 0da6d5d Compare November 10, 2023 14:47

jelly marked this pull request as ready for review November 10, 2023 15:33

jelly force-pushed the kdump-ansible-integration branch from 0da6d5d to bb8994f Compare November 10, 2023 15:44

martinpitt requested changes Nov 13, 2023

View reviewed changes

jelly added no-test For doc/workflow changes, or experiments which don't need a full CI run, and removed no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Nov 15, 2023

jelly force-pushed the kdump-ansible-integration branch from a83d5dc to f713bf1 Compare November 16, 2023 13:29

jelly commented Nov 16, 2023

View reviewed changes

test/verify/check-kdump Show resolved Hide resolved

jelly requested a review from martinpitt November 16, 2023 13:50

martinpitt requested changes Nov 20, 2023

View reviewed changes

jelly force-pushed the kdump-ansible-integration branch 2 times, most recently from 169a6c2 to 5182da9 Compare November 20, 2023 16:53

jelly requested a review from martinpitt November 21, 2023 07:46

jelly force-pushed the kdump-ansible-integration branch from 5182da9 to dee7084 Compare November 21, 2023 08:56

martinpitt requested changes Nov 21, 2023

View reviewed changes

jelly force-pushed the kdump-ansible-integration branch from dee7084 to d704728 Compare November 21, 2023 16:46

martinpitt reviewed Nov 23, 2023

View reviewed changes

jelly force-pushed the kdump-ansible-integration branch from d704728 to 3d0f3d5 Compare November 23, 2023 15:01

cockpit-project deleted a comment from jelly Nov 23, 2023

jelly added 2 commits November 24, 2023 11:30

lib: allow Modifications component to be re-used

698bb37

kdump: add automation script support

864d376

Extend the kdump page to allow administrators to configure kdump via Cockpit and export an Ansible playbook using the linux-system-roles kdump role.

jelly force-pushed the kdump-ansible-integration branch from 3d0f3d5 to 864d376 Compare November 24, 2023 10:30

cockpituous reviewed Nov 24, 2023

View reviewed changes

jelly requested a review from martinpitt November 24, 2023 12:35

martinpitt added the release-note label Nov 24, 2023

martinpitt approved these changes Nov 24, 2023

View reviewed changes

jelly merged commit 8f7b958 into cockpit-project:main Nov 24, 2023
93 checks passed

jelly deleted the kdump-ansible-integration branch November 24, 2023 15:02

jelly removed the release-note label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kdump ansible integration #19430

Kdump ansible integration #19430

jelly commented Oct 4, 2023 •

edited by garrett

Loading

jelly commented Oct 4, 2023 •

edited

Loading

martinpitt commented Oct 5, 2023

jelly commented Oct 5, 2023

jelly commented Nov 10, 2023

martinpitt left a comment

jelly commented Nov 13, 2023 •

edited

Loading

jelly commented Nov 14, 2023

jelly commented Nov 14, 2023

jelly commented Nov 16, 2023

martinpitt left a comment

martinpitt left a comment

martinpitt Nov 21, 2023

martinpitt Nov 23, 2023

martinpitt Nov 23, 2023

martinpitt Nov 23, 2023

martinpitt Nov 23, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

cockpituous Nov 24, 2023

martinpitt left a comment

martinpitt Nov 24, 2023

jelly Nov 24, 2023

		if (os_release.NAME === "RHEL" \|\| os_release.ID_LIKE?.includes('rhel')) {
		role_name = "rhel-system-roles";

		// HACK: we should not have to specify kdump_ssh_user and kdump_ssh_user as it is in kdump_target.location
		// https://github.com/linux-system-roles/kdump/issues/184

Kdump ansible integration #19430

Kdump ansible integration #19430

Conversation

jelly commented Oct 4, 2023 • edited by garrett Loading

Kdump: Add Ansible/shell automation

jelly commented Oct 4, 2023 • edited Loading

martinpitt commented Oct 5, 2023

jelly commented Oct 5, 2023

jelly commented Nov 10, 2023

martinpitt left a comment

Choose a reason for hiding this comment

jelly commented Nov 13, 2023 • edited Loading

jelly commented Nov 14, 2023

jelly commented Nov 14, 2023

jelly commented Nov 16, 2023

martinpitt left a comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelly commented Oct 4, 2023 •

edited by garrett

Loading

jelly commented Oct 4, 2023 •

edited

Loading

jelly commented Nov 13, 2023 •

edited

Loading