Print CORE_COLLECTOR as debug info

[sourabhjains]

Printing CORE_COLLECTOR helps developers identify which core collector, along with its respective options, was used to capture the dump.

It is also helpful when writing kdump and kdump-utils tests using external test frameworks like op-tests.

[gemini-code-assist]

Summary of Changes

Hello @sourabhjains, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a minor but impactful change to the kdump script by adding debug output for the CORE_COLLECTOR variable. The primary goal is to provide better visibility into the core collection mechanism during system dumps, which is crucial for diagnosing issues and validating test scenarios. By logging this information, developers and testers can quickly ascertain the configuration used for capturing vmcores, streamlining the debugging and testing workflows.

Highlights

• Enhanced Debugging: Added debug logging to print the CORE_COLLECTOR variable before initiating the core dump process in various kdump functions.
• Improved Troubleshooting: This change helps developers identify which core collector and its options were used during a dump capture, aiding in troubleshooting.
• Testing Support: The added debug information will be beneficial for writing and debugging kdump and kdump-utils tests, especially with external test frameworks.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces debug logging for the CORE_COLLECTOR variable in the kdump.sh script. The added ddebug statements will help developers identify the core collector used during dump capture, which can be useful for debugging and testing. I have identified a potential issue where the debug log might expose sensitive information if CORE_COLLECTOR contains secrets.

[gemini-code-assist]

The CORE_COLLECTOR variable might contain sensitive information (e.g., passwords, API keys) if custom configurations are used. Printing it as debug information could expose these secrets. Consider sanitizing the output or using a more secure way to log this information.

For example, you could log a hash of the sensitive parts, or redact them entirely.

Suggested change

	ddebug "Core Collector: $CORE_COLLECTOR"
	ddebug "Core Collector: $(echo "$CORE_COLLECTOR" | sed 's/password=\S*//g')" # Redact sensitive information

[prudo1]

Hi Sourabh,

I don't understand the problem you are trying to solve with this PR. Can you please explain in what situation printing of the CORE_COLLECTOR helps and why it needs special handling compared to the other config options.

Thanks
Philipp

[sourabhjains]

Hi Sourabh,

I don't understand the problem you are trying to solve with this PR. Can you please explain in what situation printing of the CORE_COLLECTOR helps and

First, while debugging kdump, one can see which CORE_COLLECTOR along with its options was used to capture the dump.

Second, to automate the kdump and kdump-utils tests, if CORE_COLLECTOR is printed on the console, one can parse it to verify that the configured CORE_COLLECTOR option was actually used during dump capture.

why it needs special handling compared to the other config options.

I just want to print the CORE_COLLECTOR during dump collection to see which options were used by the CORE_COLLECTOR. For example, what was the value of --num-threads= for the makedumpfile CORE_COLLECTOR.

[coiby]

Hi @prudo1,

Do you still request any change after Sourabh has provided the explanation?

[prudo1]

Hi Coiby,
Hi Sourabh,

sorry for the late reply.

I still don't see the benefit in adding those debug messages. For me it looks like a workaround without addressing the underlying problem. I'd prefer see a cleanup so that the CORE_COLLECTOR isn't changed for every possible dump target instead of just printing the final result. Especially as the --num-threads implementation is broken for raw dumps in the moment. It combines --num-threads with -F which IIUC shouldn't be possible.

Thanks
Philipp

[sourabhjains]

Hi Coiby, Hi Sourabh,

sorry for the late reply.

I still don't see the benefit in adding those debug messages. For me it looks like a workaround without addressing the underlying problem. I'd prefer see a cleanup so that the CORE_COLLECTOR isn't changed for every possible dump target instead of just printing the final result. Especially as the --num-threads implementation is broken for raw dumps in the moment. It combines --num-threads with -F which IIUC shouldn't be possible.

I understand there might be some confusion around CORE_COLLECTOR, as we don’t yet have a common function to manage it.

That said, this patch only adds a print for CORE_COLLECTOR, and that too only when debug mode is enabled. The kdump service already prints several details, including its log level, but it currently misses one key piece of information - the CORE_COLLECTOR. I believe adding this would help both while debugging and during test automation.

[prudo1]

Hi Sourabh,

Hi Coiby, Hi Sourabh,
sorry for the late reply.
I still don't see the benefit in adding those debug messages. For me it looks like a workaround without addressing the underlying problem. I'd prefer see a cleanup so that the CORE_COLLECTOR isn't changed for every possible dump target instead of just printing the final result. Especially as the --num-threads implementation is broken for raw dumps in the moment. It combines --num-threads with -F which IIUC shouldn't be possible.

I understand there might be some confusion around CORE_COLLECTOR, as we don’t yet have a common function to manage it.

That said, this patch only adds a print for CORE_COLLECTOR, and that too only when debug mode is enabled. The kdump service already prints several details, including its log level, but it currently misses one key piece of information - the CORE_COLLECTOR. I believe adding this would help both while debugging and during test automation.

you are not only adding a print. You are also adding a dependency of an external test on that print, which isn't documented anywhere. So this print adds a big maintenance overhead for us in the long term. That's why I want to have a more generic solution that helps with not just this single problem.

All in all the best way forward is to fix the xtrace issue @coiby is tracking in #108. When this is fixed you can add rd.debug to the kernel command line and get the full xtrace output, i.e. every line of bash code executed in the initrd. Fixing this will help with debugging a lot of issues in the kdump initrd, including yours.

Thanks
Philipp

[sourabhjains]

I will try the changes introduce in #108 and close this pull request.