On Fri, Mar 28, 2014 at 05:42:02AM -0400, Martin Perina wrote:
> Can you provide changelog explaining what this patch is doing.
> 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:
Author: Martin Perina <mperina(a)redhat.com>
Date: Tue Mar 25 12:33:04 2014 +0100
Rename FENCE_KDUMP_CONFIG to FENCE_KDUMP_CONFIG_FILE
Signed-off-by: Martin Perina <mperina(a)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
Typically we follow following format.
Author: Arthur Zou <zzou(a)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(a)redhat.com>
Acked-by: WANG Chao <chaowang(a)redhat.com>
Acked-by: Vivek Goyal <vgoyal(a)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
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
Do you mind if we change it and ask user to specify options in
/etc/kdump.conf using fence_kdump_args options.