[PATCH 1/6] fence_kdump for generic clusters v3: Rename FENCE_KDUMP_CONFIG to FENCE_KDUMP_CONFIG_FILE

[Vivek Goyal]

On Fri, Mar 28, 2014 at 05:42:02AM -0400, Martin Perina wrote:

[..]
> > Can you provide changelog explaining what this patch is doing. We put
> > appropriate changelog for every patch in the series.
> 
> Vivek, what kind of changelog do you mean?
> 
> This is what I see on my git using git log:
> 
> commit b730272fb04de04f770cbfb9b6afc15dcfcbbcbd
> Author: Martin Perina <mperina at redhat.com>
> Date:   Tue Mar 25 12:33:04 2014 +0100
> 
>     Rename FENCE_KDUMP_CONFIG to FENCE_KDUMP_CONFIG_FILE
>     
>     Bug-Url: https://bugzilla.redhat.com/1078134
>     Signed-off-by: Martin Perina <mperina at redhat.com>
> 
> And this content is also included in the email. Or do you think,
> that "Rename FENCE_KDUMP_CONFIG to FENCE_KDUMP_CONFIG_FILE" is not
> descriptive enaough?

Typically we follow following format.

commit 7f9b45002a940c3ba5604552cda4d687948cf443
Author: Arthur Zou <zzou at redhat.com>
Date:   Thu Mar 13 11:02:07 2014 +0800

    Backport vmcore-dmsg stack smashing in extreme case
    
    In exteme case vmcore-dmesg will overflow. upstream has fixed the
    some problem. so simply backport it
    
    Signed-off-by: Arthur Zou <zzou at redhat.com>
    Acked-by: WANG Chao <chaowang at redhat.com>
    Acked-by: Vivek Goyal <vgoyal at redhat.com>

First line is subject of the mail. Which is more of a short description of
the patch. And that is followed by the long description of patch. And the
all the tags of signing and review follow. I like Bug-url: tag too.

I see that you are renaming a file but in long descpriton it would also
help to know why are you renaming this file and how rest of the patch
series is going to make use of it.

> 
> > 
> > Also in general, have you tested it? Last time you mentioned that from
> > vdsm side you folks are still debating which method to use. I am assuming
> > that means vdsm code is not ready yet and that probably means this
> > patchset might be untested?
> 
> I successfully test those patches using:
> 
>   1) Modify fence_kdump_nodes and fence_kdump_args in kdump.conf on host1
>   2) Execute 'kdumpctl restart' on host1
> 
>   Above steps will be performed automatically by new VDSM module.
> 
>   Then I execute this test:
> 
>   1) I executed fence_kdump listener on host2 which is included in fence_kdump_nodes
>   2) I crashed kernel on host1 using 'echo c > /proc/sysrq-trigger'
>   3) I looked through IPMI to host1 console if kdump was executed sucessfully
>   4) I verified that required notification was received in host2
> 
> I also verified that without specifying fence_kdump the kdump process will work
> correctly.
> 

Excellent. That's a good enough test for this new API.

> I didn't test these patches with Pacemaker, but I think that once you will be
> satisfied with content of those paches, we can ask some one from Pacemake team
> to test them if those patches didn't create new regression.
> 
> Marek, do you agree?

Some regression test to make sure pacemaker use case is not broken will
be required too.

[..]
> > > --- a/kdumpctl
> > > +++ b/kdumpctl
> > > @@ -249,8 +249,8 @@ function check_rebuild()
> > >  	EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> > >  	files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS"
> > >  
> > > -	if [ -f $FENCE_KDUMP_CONFIG ]; then
> > > -		files="$files $FENCE_KDUMP_CONFIG"
> > > +	if [ -f $FENCE_KDUMP_CONFIG_FILE ]; then
> > > +		files="$files $FENCE_KDUMP_CONFIG_FILE"
> > >  	fi
> > 
> > why don't we get rid of this fence kdump config file.  I thought we agreed
> > that fence_kdump_send options will be specified in /etc/kdump.conf using
> > a new option.
> > 
> > No package owns /etc/sysconfig/fence_kdump_send. So trying to retain
> > it does not make sense.
> 
> As we agreed in previous email, current API for Pacemaker will be untouched.

In Fedora generally we don't have the requirement of maintaing full
backward compatibility with older fedora versions. And pacemaker code
is so new in Fedora. It went in very recently.

So I would rather prefer to clean that up and ask pacemaker users to
specify options in /etc/kdump.conf instead of using a file.

Otherwise we will be stuck with this code for a long time.

> 
> So in following patches, I will introduce new options in kdump.conf. With those
> patches kdump will not use /etc/fence_kdump_nodes file (this will be replaced
> with fence_kdump_nodes option in kdump.conf). But not to break Pacemaker API
> during Pacemaker fence_kdump configuration content of /etc/sysconfig/fence_kdump
> will be read and store into fence_kdump_args in kdump.conf in initial ramdisk.

Marek, are you particular about maintaining this file. No package owns
this file so asking user to create this file and provide options here
sounds bad.

Do you mind if we change it and ask user to specify options in
/etc/kdump.conf using fence_kdump_args options.

Thanks
Vivek