kdump: Show message in 'Test configuration' to verify the crash #19107

skobyda · 2023-07-18T12:45:06Z

This adds message showing a location where user can verify whetever crash was successful:

Local filesystem: "To verify whether the crash was successful check /var/crash location for kernel crash dump(vmcore)."
SSH: "To verify whether the crash was successful check /var/crash location at [email protected] for kernel crash dump(vmcore)"
-NFS: "To verify whether the crash was successful check /var/crash location at myserver.com:/export/cores for kernel crash dump(vmcore)."

Closes https://issues.redhat.com/browse/RHELPLAN-125375
Closes https://bugzilla.redhat.com/show_bug.cgi?id=2097440

Kdump: Show location of kdump to verify the successful configuration test

You can now see the location where kdump can be found if testing of kdump settings is successful.

marusak · 2023-07-18T12:49:04Z

Nice! One more additional idea is whether we can check this automatically? That once you reboot and visit kdupm page again it would show big green check mark that there is fresh kernel dump in designed location?

skobyda · 2023-07-18T12:59:08Z

Nice! One more additional idea is whether we can check this automatically? That once you reboot and visit kdupm page again it would show big green check mark that there is fresh kernel dump in designed location?

I have been thinking also about this to send as followup (since this PR is really simple and fixes jira/bugzilla, I think it's worth it to merge separately ASAP).

And about the automatic check. I guess we have to save to browser's localStorage the timestamp of the last simulated kernel crash. And then we can check the last write to the crash dump location. As long as we only support it for crash dump location at Local filesystem/SSH/NFS, it should be fairly easy to check. For others, I think it's right now not feasible to implement.

EDIT: Or maybe just looking at kdump service logs + the timestamp should be enough

garrett · 2023-07-25T12:21:58Z

Local:

SSH:

NFS:

Additionally, most people aren't going to remember where to check, especially if it takes a while to test. Therefore, we should also make sure this information is easy to see on the front page (like perhaps highlight a crash report if there was a crash on the last boot).

skobyda · 2023-08-03T11:55:58Z

@garrett updated the design:

Also, if you configure kdump to use SSH, it will not move it to [email protected]:/home/you but [email protected]:${path} where path is configurable by the user through our UI

NFS:

SSH:

Local:

garrett

Looks great!

Minor nitpicks:

The modals should have an icon (specifically, the golden <!> icon, like in the mockups)
There should be an X on closable modals (either with "Cancel", as the case here, and also on "Close" ones (not the case here; just pointing that out))

Additionally: I notice the width is wider in the screenshots than in the modal. Generally, for less complex modals (especially confirmations), going with the narrower ones would be better for readability. However, there is some path and filename info here; going too narrow could provide more opportunities for wrapping (not great here). So it's probably fine at your width. This is just a comment, not something to change. 😉

skobyda · 2023-08-15T11:52:07Z

Updated the design, tests seem green, I think it's ready for code-review:

liutgnu · 2023-08-16T08:06:23Z

Hi skobyda,

I'm Tao Liu from kdump team, came from bz2215024. Thanks a lot for your efforts on this. Since cockpit support is one subtask of bz2215024, and I have worked on the TUI part for sometime, I can share some of my thoughts:

It will be better to have an automatic test check mechanism, as @marusak pointed out, after machine reboot and user re-enter the kdump page, a sign indicating if the test is successful or not will be good. Relying on user himself to check if the "test vmcore" been generated is inconvenient, there might be plenty of vmcores located in a nfs/ssh server for "nfs/ssh kdump", the manual checking will be a boring work.
It may not be the final desgin, I planed to introduce 2 new shell command line options for kdumpctl: "kdumpctl test" and "kdumpctl test-check". "kdumpctl test" will generate a timestamp, which will be used to mark the vmcore that it belongs to a specifc test. "kdumpctl test-check" will check the kdump folder after reboot back, no matter if it's a local folder or a nfs/ssh remote folder, to see a vmcore with the specific timestamp string exist or not, which then be the indicator of the test is successful or not. Maybe you can call "kdumpctl test-check" in cockpit if it is easier.
If the user modifies the configuration file of kdump, the prior test result will be invalid and notify he should retest based on the current kdump config. I hadn't figure out the tech details, maybe some file monitor service to be involved.

Anyway, it's currently an early design phase for me, we can share thoughts or discuss kdump APIs if you need some underlying support.

Thanks,
Tao Liu

skobyda · 2023-08-16T08:39:35Z

It will be better to have an automatic test check mechanism, as @marusak pointed out, after machine reboot and user re-enter the kdump page, a sign indicating if the test is successful or not will be good. Relying on user himself to check if the "test vmcore" been generated is inconvenient, there might be plenty of vmcores located in a nfs/ssh server for "nfs/ssh kdump", the manual checking will be a boring work.

Yes, that will be a followup

It may not be the final desgin, I planed to introduce 2 new shell command line options for kdumpctl: "kdumpctl test" and "kdumpctl test-check". "kdumpctl test" will generate a timestamp, which will be used to mark the vmcore that it belongs to a specifc test. "kdumpctl test-check" will check the kdump folder after reboot back, no matter if it's a local folder or a nfs/ssh remote folder, to see a vmcore with the specific timestamp string exist or not, which then be the indicator of the test is successful or not. Maybe you can call "kdumpctl test-check" in cockpit if it is easier.

That's exactly what I have as work-in-progress locally. My plan was to generate timestamp when "Test configuration" is triggered and store in in browser's storage. But "kdumpctl test-check" would work be even better! while using the similar principle.

If the user modifies the configuration file of kdump, the prior test result will be invalid and notify he should retest based on the current kdump config. I hadn't figure out the tech details, maybe some file monitor service to be involved.

From our point of view, we are alsofine with calling some command to reset the timestamp manually when user uses our UI to change configuration file.

liutgnu · 2023-08-16T11:01:41Z

It will be better to have an automatic test check mechanism, as @marusak pointed out, after machine reboot and user re-enter the kdump page, a sign indicating if the test is successful or not will be good. Relying on user himself to check if the "test vmcore" been generated is inconvenient, there might be plenty of vmcores located in a nfs/ssh server for "nfs/ssh kdump", the manual checking will be a boring work.

Yes, that will be a followup

^_^

It may not be the final desgin, I planed to introduce 2 new shell command line options for kdumpctl: "kdumpctl test" and "kdumpctl test-check". "kdumpctl test" will generate a timestamp, which will be used to mark the vmcore that it belongs to a specifc test. "kdumpctl test-check" will check the kdump folder after reboot back, no matter if it's a local folder or a nfs/ssh remote folder, to see a vmcore with the specific timestamp string exist or not, which then be the indicator of the test is successful or not. Maybe you can call "kdumpctl test-check" in cockpit if it is easier.

That's exactly what I have as work-in-progress locally. My plan was to generate timestamp when "Test configuration" is triggered and store in in browser's storage. But "kdumpctl test-check" would work be even better! while using the similar principle.

OK, I will try to design the kdumpctl APIs in consideration of cockpit, hope it can be clean and easy to use. Just FYI, there is the repo which we used for implementing TUI "kdumpctl test/test-check" etc, currently just a POC in the ltao-dev branch.

If the user modifies the configuration file of kdump, the prior test result will be invalid and notify he should retest based on the current kdump config. I hadn't figure out the tech details, maybe some file monitor service to be involved.

From our point of view, we are alsofine with calling some command to reset the timestamp manually when user uses our UI to change configuration file.

OK, thanks!

garrett · 2023-08-16T15:20:30Z

I'm working on integrating the discussion into a new mockup!

Here's what I have so far:

Source: Kdump.penpot in Kdump.zip

It's still lacking a lot, such as: the modals for each option, the alert for when the service is out of sync (that is: if it's enabled and the service isn't running), other error cases, etc. — but the bulk of the direction to modernize the Kdump page (and bring it up to established patterns in both Cockpit and PatternFly) is there.

garrett · 2023-08-16T16:35:44Z

I've opened up a discussion @ #19203 with a summary of what @liutgnu, @skobyda, and I have contributed here... so we can continue talking about kdump more in general, without it having to be tied to this PR (as this PR will be merged pretty soon and I'd love to keep the conversation going).

liutgnu · 2023-08-17T10:42:51Z

I've opened up a discussion @ #19203 with a summary of what @liutgnu, @skobyda, and I have contributed here... so we can continue talking about kdump more in general, without it having to be tied to this PR (as this PR will be merged pretty soon and I'd love to keep the conversation going).

OK thanks, we can discuss there.

skobyda · 2023-08-17T11:31:41Z

After a bit more discussion, one last change to the dialogs:

pkg/kdump/kdump.scss

pkg/kdump/kdump-view.jsx

pkg/lib/cockpit-components-dialog.jsx

This adds message showing a location where user can verify whetever crash was successful: Local filesystem: "To verify whether the crash was successful check /var/crash location for kernel crash dump(vmcore)." SSH: "To verify whether the crash was successful check /var/crash location at [email protected] for kernel crash dump(vmcore)" NFS: "To verify whether the crash was successful check /var/crash location at myserver.com:/export/cores for kernel crash dump(vmcore)." Closes https://issues.redhat.com/browse/RHELPLAN-125375 Closes https://bugzilla.redhat.com/show_bug.cgi?id=2097440

cockpituous · 2023-08-23T07:57:40Z

pkg/kdump/kdump-view.jsx

+                    ' ' + _("Results of the crash will be stored in $0 as $1, if kdump is properly configured."),
+                    <span className="pf-v5-u-font-family-monospace-vf">{path}</span>,
+                    <span className="pf-v5-u-font-family-monospace-vf">vmcore</span>);
+            } else if (target.type === "ssh" || target.type == "nfs") {


This added line is not executed by any test. Details

cockpituous · 2023-08-23T07:57:40Z

pkg/kdump/kdump-view.jsx

+                    <span className="pf-v5-u-font-family-monospace-vf">{target.type === "ssh" ? "SSH" : "NFS"}</span>,
+                    <span className="pf-v5-u-font-family-monospace-vf">{`${target.server}:${target.type === "nfs" ? target.export + path : path}`}</span>,


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

Garrett's ack'ed the changes on matrix.

skobyda requested a review from garrett July 18, 2023 12:45

skobyda force-pushed the kdumpMessage branch from bf8cfcb to 0e02683 Compare August 3, 2023 11:55

skobyda requested review from garrett and removed request for garrett August 3, 2023 11:56

garrett previously requested changes Aug 14, 2023

View reviewed changes

skobyda force-pushed the kdumpMessage branch 3 times, most recently from 6006f68 to 55c6ab4 Compare August 15, 2023 11:25

skobyda marked this pull request as ready for review August 15, 2023 11:49

skobyda requested review from marusak and garrett August 15, 2023 11:52

skobyda force-pushed the kdumpMessage branch from 55c6ab4 to 399a4d1 Compare August 15, 2023 12:57

skobyda force-pushed the kdumpMessage branch 3 times, most recently from 54d1cb9 to bc80683 Compare August 17, 2023 09:33

skobyda force-pushed the kdumpMessage branch from bc80683 to edcef2e Compare August 17, 2023 11:17

garrett reviewed Aug 17, 2023

View reviewed changes

pkg/kdump/kdump.scss Outdated Show resolved Hide resolved

Update kdump test settings message

65a2303

skobyda force-pushed the kdumpMessage branch from edcef2e to faa2603 Compare August 17, 2023 12:31

skobyda requested a review from garrett August 17, 2023 12:33

skobyda force-pushed the kdumpMessage branch 2 times, most recently from 7e21bcc to b586196 Compare August 21, 2023 08:46

skobyda requested a review from jelly August 21, 2023 10:59

jelly requested changes Aug 22, 2023

View reviewed changes

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

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

pkg/lib/cockpit-components-dialog.jsx Show resolved Hide resolved

lib: add showClose as Dialog's prop

bfed53d

skobyda force-pushed the kdumpMessage branch 3 times, most recently from 086b465 to 1ea4253 Compare August 22, 2023 22:31

skobyda force-pushed the kdumpMessage branch from 1ea4253 to 45cdb56 Compare August 22, 2023 22:31

skobyda requested a review from jelly August 22, 2023 22:43

skobyda added the release-note label Aug 22, 2023

cockpituous reviewed Aug 23, 2023

View reviewed changes

jelly approved these changes Aug 23, 2023

View reviewed changes

skobyda merged commit 9cceccb into cockpit-project:main Aug 23, 2023
36 checks passed

martinpitt removed the release-note label Sep 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kdump: Show message in 'Test configuration' to verify the crash #19107

kdump: Show message in 'Test configuration' to verify the crash #19107

skobyda commented Jul 18, 2023 •

edited

Loading

marusak commented Jul 18, 2023

skobyda commented Jul 18, 2023 •

edited

Loading

garrett commented Jul 25, 2023

skobyda commented Aug 3, 2023 •

edited

Loading

garrett left a comment

skobyda commented Aug 15, 2023

liutgnu commented Aug 16, 2023

skobyda commented Aug 16, 2023 •

edited

Loading

liutgnu commented Aug 16, 2023

garrett commented Aug 16, 2023

garrett commented Aug 16, 2023

liutgnu commented Aug 17, 2023

skobyda commented Aug 17, 2023

cockpituous Aug 23, 2023

cockpituous Aug 23, 2023

		<span className="pf-v5-u-font-family-monospace-vf">{target.type === "ssh" ? "SSH" : "NFS"}</span>,
		<span className="pf-v5-u-font-family-monospace-vf">{`${target.server}:${target.type === "nfs" ? target.export + path : path}`}</span>,

kdump: Show message in 'Test configuration' to verify the crash #19107

kdump: Show message in 'Test configuration' to verify the crash #19107

Conversation

skobyda commented Jul 18, 2023 • edited Loading

Kdump: Show location of kdump to verify the successful configuration test

marusak commented Jul 18, 2023

skobyda commented Jul 18, 2023 • edited Loading

garrett commented Jul 25, 2023

skobyda commented Aug 3, 2023 • edited Loading

NFS:

SSH:

Local:

garrett left a comment

Choose a reason for hiding this comment

skobyda commented Aug 15, 2023

liutgnu commented Aug 16, 2023

skobyda commented Aug 16, 2023 • edited Loading

liutgnu commented Aug 16, 2023

garrett commented Aug 16, 2023

garrett commented Aug 16, 2023

liutgnu commented Aug 17, 2023

skobyda commented Aug 17, 2023

cockpituous Aug 23, 2023

Choose a reason for hiding this comment

cockpituous Aug 23, 2023

Choose a reason for hiding this comment

skobyda commented Jul 18, 2023 •

edited

Loading

skobyda commented Jul 18, 2023 •

edited

Loading

skobyda commented Aug 3, 2023 •

edited

Loading

skobyda commented Aug 16, 2023 •

edited

Loading