On Tue, 10 Oct 2023 10:30:30 +0800
Tao Liu <ltao(a)redhat.com> wrote:
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?
I understand and agree that the "remote folder mount" is currently a
functionality that is missing in kdumpctl. The problem is that by
having kdumpctl and mkdumprd as separate scripts they both build
slightly different parallel structures which don't work well together.
See for example the problem with the temp dir.
In addition there are quite a lot of nits in the old mkduprd code I
would like to cleanup instead of copying it over to kdumpctl. For
example, why does check_user_configured_target and
handle_default_target fail when no SAVE_PATH exists while on a ssh
target the SAVE_PATH is created (mkdir_save_path_ssh)? And why does
mkdumprd even create the SAVE_PATH in this case when
check_and_wait_network_ready in kdumpctl already did it? And why are
the "user configured" and "default" targets handled differently?
Isn't
the "default" target the same like setting path to /var/crash? And why
are the file system and ssh cases treated differently? They both
execute the same commands, only difference is that the command needs to
be prefixed with ssh and it's options. Similar for raw targets. If you
switch to lsblk the only difference between a raw and fs target when
determining the size is the option to use, i.e. FSAVAIL vs. SIZE.
Combining everything above for example you could merge all the
get_*_size functions into one, similar to this (untested)
ssh_cmd_prefix=( ssh -q -n -i "$SSH_KEY_LOCATION" -o BatchMode=yes -o
StrictHostKeyChecking=yes )
get_size()
{
local fstype=$1 target=$2
local -a cmd=(lsblk --nodeps -npb)
if [[ "$fstype" == raw ]]; then
cmd+=( -o SIZE )
else
cmd+=( -o FSAVAIL )
fi
if [[ "$fstype" == ssh ]]; then
cmd=( ${ssh_cmd_prefix[@]} ${cmd[@]} )
fi
${cmd[@]} $target
}
Which is much shorter and could easily be shared. Plus by having the
ssh_cmd_prefix a similar schema could be used with all the other cases
where the fs and ssh functions are basically identical. Additional plus
is that all calls to ssh are now also guaranteed to use the same
options rather than every function defining it's own set.
> 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.
How about 3) kdumpctl test creates the dir and passes it to
dracut-kdump.sh.
The way I imagine it to work is that kdumpctl test creates the
diretory, say <path>/tests/<ip>-<time>, and a file, say
<path>/.<ip>.test, which only content is the dir name relative to
<path>, i.e. tests/<ip>-<time>.
Before dracut-kdump.sh creates the new directory it checks whether
.<ip>.test file exists. If it does it uses the directory contained
instead of creating a new directory and removes the file. If the file
doesn't exist dracut-kdump.sh proceeds as usual.
That should work, shouldn't it.
Thanks
Philipp
> 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(-)
> >
>