Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kdump: always set kdump.conf defaults #19434

Merged
merged 1 commit into from
Oct 10, 2023

Conversation

jelly
Copy link
Member

@jelly jelly commented Oct 4, 2023

When an administrator clears core_collector setting and we save a new configuration the core_collector value is set to makedumpfile without arguments. This is an invalid setting for kdump and not what the kdump.conf man page recommends as default setting.

The downside of hardcoding the default recommendation is that it may change over time, so this change includes a test to validate that when absent we re-set the default value.

@jelly jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 4, 2023
@jelly jelly mentioned this pull request Oct 4, 2023
pkg/kdump/config-client.js Outdated Show resolved Hide resolved
test/verify/check-kdump Show resolved Hide resolved
@jelly jelly force-pushed the kdump-default-makedumpfile branch from 2940fd2 to a32e96b Compare October 5, 2023 12:54
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm fine with that approach, just one detail.

test/verify/check-kdump Show resolved Hide resolved
@jelly jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Oct 10, 2023
@jelly jelly force-pushed the kdump-default-makedumpfile branch 2 times, most recently from cd5300e to 8715417 Compare October 10, 2023 11:06
When an administrator clears core_collector setting and we save a new
configuration the core_collector value is set to `makedumpfile` without
arguments. This is an invalid setting for kdump and not what the
kdump.conf man page recommends as default setting.

The downside of hardcoding the default recommendation is that it may
change over time, so this change includes a test to validate that when
absent we re-set the default value.
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jelly jelly marked this pull request as ready for review October 10, 2023 12:10
Comment on lines +271 to +272
if (target.type === "ssh") {
settings._internal.core_collector.value += " -F";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -273,7 +280,7 @@ export class ConfigFile {
if ("core_collector" in settings._internal)
settings._internal.core_collector.value = settings._internal.core_collector.value + " -c";
else
settings._internal.core_collector = { value: "makedumpfile -c" };
settings._internal.core_collector = { value: defaultCoreCollector };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

@jelly jelly merged commit 9a18e43 into cockpit-project:main Oct 10, 2023
98 checks passed
@jelly jelly deleted the kdump-default-makedumpfile branch October 10, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants