Hi Philipp,
Thanks for reviewing the patchset and your comments!
On Tue, Oct 10, 2023 at 12:24 AM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
On Thu, 28 Sep 2023 21:20:25 +0800
Tao Liu <ltao(a)redhat.com> wrote:
> Previously after setting kdump config, people will just regard the configs
> to be "workable". However sometimes the "workable" kdump config
is just an
> illusion and will not have a vmcore generated when system crashes. The patchset
> implements kdumpctl test/test-check, which will actually perform a crash & dump
> based on the current kdump config, to work as a way to verify if the current
> kdump config can really generate vmcores.
>
> kdumpctl test: It will output the current kdump config list to users, including
> crash kernel size, effective contents of kdump.conf etc. Then it will generate
> a timestamp, which works as a id to mark the current testing. For kdump, the
> id will be appended to 2nd kernel's commandline. For fadump, the id will be
> append to 1st kernel's dmesg through /dev/kmsg.
>
> kdumpctl test-check: After system reboot back, test-check will automatically
> mount the dump target, then check if the id exists within files of dump path.
> For kdump, the id should be in kexec-dmesg.log, similar in vmcore-dmesg.txt for
> fadump. If test-check is successful, a md5sum of the current kdump config will
> be generated. If re-run test-check, it will only check if md5sum, or kdump
> config, is changed. This can speed up re-running test-check, since it will
> not remount dump target nor file string grep.
I have some concerns on your patches.
To begin with I don't like that you pull out code from mkdumprd into
kdump-lib. In my opinion mkdumprd picked up a lot of ballast over time.
In fact it only exists due to historic reasons as today it is simply a
wrapper around dracut which could also be done in a simple function
without all the overhead needed for a separate script. That's why we
should try to make mkdumprd work like kdumpctl not the other way round.
With the goal to slowly get rid of it piece by piece (unless you want
to do it all in one go, that's fine with me too ;-) ).
The purpose of extracting code from mkdumprd into kdump-lib is to
utilize the "remote folder mount" functions. For mkdumprd, it will
check if the dump target space is enough and print warnings like
"Warning: There might not be enough space to save a vmcore." Same as
"kdumpctl test-check" when it will need to mount and check the dump
target for a specific test id-string. Since it will be used by the 2,
so I extract the function into the kdump-lib library.
Even though we don't copy the code from mkdumprd, leave it as it is,
we still need to implement a different version of "remote folder
mount" for kdumpctl test-check. Do you have any suggestions?
Furthermore I find your way of marking a dump as test unnecessarily
difficult as it requires you to search every dmesg on the target just
to find out if it succeeded or not. Personally I would simply change
the save path to some other naming convention (lets say from
<ip>-<time> to <ip>-<time>.test or
tests/<ip>-<time>). This directory
is created by kdumpctl test and contains a "lock file" that keeps the
state of the test run, e.g. 1 when it was initiated by kdumpctl test, 2
after dracut-kdump.sh was executed and kdumpctl test-check removes it
again once it has analyzed the run.
Yeah, I have thought of the naming solution as "<ip>-<time>.test".
Since the dir naming is happening in the 2nd kernel of
dracut-kdump.sh, we need to tell dracut-kdump.sh if we are in "kdump
test" or "normal kdump". I didn't think of a better way: 1) through a
new field in /etc/kdump.conf: bad idea. 2) through a harmless cmdline
arg for 2nd kernel: current solution.
I expect this approach to be much more light weight even when we need
to mount the target every time. Furthermore this approach allows us to
store additional data together with the dump, e.g. the initrd, kernel
version, kexec-tools/kdump-utils version etc.. With that users
(including us) can easily create some sort of "audit trace" on when
they have tested what and what has changed in between a run that worked
and one that didn't.
Agreed.
There are a few more command on the individual patches but I think
those are the two main points we need to work on first.
Thanks
Philipp
> Tao Liu (5):
> Extract some common functions from mkdumprd to kdump-lib.sh
> Small modifications for kdumpctl test/test-check preparation
> kdumpctl test support
> kdumpctl test-check support
> Add md5sum to kdumpctl test-check
>
> dracut-kdump.sh | 1 -
> kdump-lib-initramfs.sh | 6 ++
> kdump-lib.sh | 138 ++++++++++++++++++++++++
> kdumpctl | 232 ++++++++++++++++++++++++++++++++++++++++-
> mkdumprd | 127 ++--------------------
> 5 files changed, 386 insertions(+), 118 deletions(-)
>