Re: [PATCH 6/6] fence_kdump for generic clusters v4: Add fence_kdump support for generic clusters

----- Original Message -----
> From: "Vivek Goyal" 
> To: "Martin Perina" 
> Cc: [e-mail]
> Sent: Tuesday, April 1, 2014 4:26:37 PM
> Subject: Re: [PATCH 6/6] fence_kdump for generic clusters v4: Add fence_kdump support for generic clusters
>
> On Tue, Apr 01, 2014 at 12:54:26PM +0200, Martin Perina wrote:
> > Adds two new options to kdump.conf to be able to configure fence_kdump
> > support for generic clusters:
> >
> > fence_kdump_args <arg(s)>
> >     - Command line arguments for fence_kdump_send (it can contain all
> >       valid arguments except hosts to send notification to)
> >
> > fence_kdump_nodes <node(s)>
> >     - List of cluster node(s) separated by space to send fence_kdump
> >       notification to (this option is mandatory to enable fence_kdump)
> >
> > Generic clusters fence_kdump configuration take precedence over older
> > method of fence_kdump configuration for Pacemaker clusters. It means
> > that if fence_kdump is configured using above options in kdump.conf, old
> > Pacemaker configuration is not used even if it exists.
> >
> > Bug-Url: https://bugzilla.redhat.com/1078134
> > Signed-off-by: Martin Perina
>
> Hi Martin,
>
> This patch series is almost there. Some minor nits below.
>
> [..]
> > diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> > index e64bc41..5541fae 100755
> > --- a/dracut-module-setup.sh
> > +++ b/dracut-module-setup.sh
> > @@ -20,7 +20,7 @@ depends() {
> >          _dep="$_dep drm"
> >      fi
> > if is_pcs_fence_kdump; then
> > if [ is_generic_fence_kdump -o is_pcs_fence_kdump ]; then
> >     _dep="$_dep network"
> > fi
> >
> > @@ -282,10 +282,13 @@ kdump_install_conf() {
> >          core_collector)
> >              dracut_install "${config_val%%[[:blank:]]*}"
> >              ;;
> >    fence_kdump_nodes)
> >        FENCE_KDUMP_NODES="$config_val"
> >        ;;
>
> I would rather like to avoid setting global varibales as much as possible.
> This install() function is called from dracut context. See more below.

OK
> [..]
> > +# setup fence_kdump in cluster
> > +# setup proper network and install needed files
> > +kdump_configure_fence_kdump () {
> > local kdump_cfg_file=$1
> > if is_generic_fence_kdump; then
> >    # fence_kdump nodes already read from kdump.conf
> >    :
> > elif is_pcs_fence_kdump; then
> >    FENCE_KDUMP_NODES=$(get_pcs_fence_kdump_nodes)
>
> Same here. We can define a "local fence_nodes" and use that in this
> function instead of trying to set list of nodes in global context.
>
> Also we could define a generic function to get list of cluster nodes.
>
> get_list_of_cluster_nodes() {
>    if generic_clsuter
>    	return generic cluster nodes
>    else
>    	return pcs_cluster_nodes
> }
>
> fence_nodes=$(get_list_of_cluster_nodes)

OK
> >    # set appropriate options in kdump.conf
> >    echo "fence_kdump_nodes $FENCE_KDUMP_NODES" >> ${kdump_cfg_file}
> >    FENCE_KDUMP_ARGS=$(get_pcs_fence_kdump_args)
> >    if [ -n "$FENCE_KDUMP_ARGS" ]; then
> >        echo "fence_kdump_args $FENCE_KDUMP_ARGS" >> ${kdump_cfg_file}
>
> KEEP FENCE_KDUMP_ARGS local too. "local fence_kdump_args".
>
> Then we will not have to cehck for generic cluster again in this function
> and our if loop will simplify to.
>
> fence_nodes=$(get_list_of_cluster_nodes)
>
> if is_pcs_fence_kdump;then
>    local fencing_args
>    fencing_args=$(get_pcs_fence_kdump_args)
>    echo "fence_kdump_args $fencing_args" >> ${kdump_cfg_file}
>    echo "fence_kdump_nodes $fence_nodes" >> ${kdump_cfg_file}
> fi

OK
> >    f
> > else
> >    # fence_kdump not configured
> >    return 1
> > fi
> >    kdump_install_net $nodename
> > # setup network for each node
> > for node in ${FENCE_KDUMP_NODES}; do
> >    kdump_install_net $node
> > done
> >
> > echo
> >
> > echo "$nodes" > ${initdir}/$FENCE_KDUMP_NODES_FILE
> > dracut_install $FENCE_KDUMP_SEND
> >
> > dracut_install -o $FENCE_KDUMP_CONFIG_FILE
> >
> > }
> >
> > # Install a random seed used to feed /dev/urandom
> > diff --git a/kdump-in-cluster-environment.txt
> > b/kdump-in-cluster-environment.txt
> > index c27a5d7..de1eb5e 100644
> > --- a/kdump-in-cluster-environment.txt
> > +++ b/kdump-in-cluster-environment.txt
> > @@ -34,11 +34,11 @@ recovery service, fence_kdump_send will periodically
> > send messages to all
> >  cluster nodes. When the fence_kdump agent receives a valid message from
> >  the
> >  failed nodes, fencing is complete.
> >
> > -How to configure cluster environment:
> > +How to configure Pacemaker cluster environment:
> >
> > -If we want to use kdump in cluster environment, fence-agents-kdump should
> > be
> > -installed in every nodes in the cluster. You can achieve this via the
> > following
> > -command:
> > +If we want to use kdump in Pacemaker cluster environment,
> > fence-agents-kdump
> > +should be installed in every nodes in the cluster. You can achieve this
> > via
> > +the following command:
> >
> > # yum install -y fence-agents-kdump
> >
> > @@ -61,6 +61,31 @@ Then enable stonith
> >
> > How to configure kdump:
> >
> > -Actually there is nothing special in configuration between normal kdump
> > and
> > -cluster environment kdump. So please refer to Kexec-Kdump-howto file for
> > more
> > -information.
> > +Actually there are two ways how to configure fence_kdump support:
> >
> > +1) Pacemaker based clusters
> > If you have successfully configured fence_kdump in Pacemaker, there
> >
> > is
> > no need to add some special configuration in kdump. So please refer
> >
> > to
> > Kexec-Kdump-howto file for more information.
> >
> > +2) Generic clusters
> > For other types of clusters there are two configuration options in
> > kdump.conf which enables fence_kdump support:
> >   fence_kdump_nodes <node(s)>
> >        Contains list of cluster node(s) separated by space to send
> >        fence_kdump notification to (this option is mandatory to
> >
> > enable
> >        fence_kdump)
> >   fence_kdump_args <arg(s)>
> >        Command line arguments for fence_kdump_send (it can contain
> >        all valid arguments except hosts to send notification to)
> > These options will most probably be configured by your cluster
> >
> > software,
> > so please refer to your cluster documentation how to enable
> >
> > fence_kdump
> > support.
> >
> > +Please be aware that these two ways cannot be combined and 2) has
> > precedence
> > +over 1). It means that if fence_kdump is configured using
> > fence_kdump_nodes
> > +and fence_kdump_args options in kdump.conf, Pacemaker configuration is not
> > +used even if it exists.
>
> [..]
> > diff --git a/kdump-lib.sh b/kdump-lib.sh
> > index 7103ba9..659ead3 100755
> > --- a/kdump-lib.sh
> > +++ b/kdump-lib.sh
> > @@ -5,7 +5,6 @@
> >
> > FENCE_KDUMP_CONFIG_FILE="/etc/sysconfig/fence_kdump"
> >  FENCE_KDUMP_SEND="/usr/libexec/fence_kdump_send"
> > -FENCE_KDUMP_NODES_FILE="/etc/fence_kdump_nodes"
>
> Why are we removing it? I thought you needed this to maintain backward
> compatibility with pcs?

Nodes read from Pacemaker config during setup was written to this file,
then the file was stored in initial ramdisk and loaded during kdump.
Now only fence_kdump_nodes option is used for storing fence_kdump_nodes.
Please see kdump_configure_fence_kdump() in dracut-module-setup.sh
> [..]
> > diff --git a/kdumpctl b/kdumpctl
> > index 8aacf4e..7450ee2 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -166,7 +166,7 @@ function check_config()
> >    	case "$config_opt" in
> >    	#* | "")
> >    		;;
> > raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|dracut_args)
> > raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)
> >   	[ -z "$config_val" ] && {
> >   		echo "Invalid kdump config value for option $config_opt."
> >   		return 1;
> >
> > @@ -249,7 +249,7 @@ function check_rebuild()
> >    EXTRA_BINS="$EXTRA_BINS $CHECK_FILES"
> >    files="$KDUMP_CONFIG_FILE $kdump_kernel $EXTRA_BINS"
> > if [ -f $FENCE_KDUMP_CONFIG_FILE ]; then
> > if [ !is_generic_fence_kdump -a -f $FENCE_KDUMP_CONFIG_FILE ]; then
> > files="$files $FENCE_KDUMP_CONFIG_FILE"
> > fi
>
> Ok, these methods for checking changes to fence kdump config is getting
> confusing. Can we please reorganize it a bit.
>
> Can we create a single function which returns list of modified cluster
> files. And then all the cluster related checks can go in there.
>
> get_cluster_modified_files() {
>    /* If not pcs cluster return */
>    /* Check for cib file related changes */
>    /* Check for changes to FENCE_KDUMP_CONFIG_FILE */
> }
>
> modified_files=`get_cluster_modified_files`
>
> This will allow us to consolidate all cluster related rebuild check in
> one function at one place.
>
> For generic cluster we don't have to do anything as kdump.conf will be
> checked anyway.

OK
> >
> > @@ -573,6 +573,31 @@ function check_kdump_feasibility()
> >    fi
> >  }
> >
> > +function check_fence_kdump_config()
> > +{
> > local hostname=`hostname`
> > while read config_opt config_val; do
> > # remove inline comments after the end of a directive.
> > config_val=$(strip_comments $config_val)
> > case "$config_opt" in
> > 	fence_kdump_nodes)
> > 		FENCE_KDUMP_NODES="$config_val"
> > 		;;
> > 	*)
> > 		;;
>
> If you are not doing anything under *), do we still need to specify it?
> IOW, does the syntax of "case" mandates that default needs to be specified
> even if we are not doing anything.

I just verified, that the default is not required, so I will remove the part
> Also this function is just making sure that config is fine. Why not use
> a local variable fence_kudmp_nodes to store values instead of saving
> it in global variable FENCE_KDUMP_NODES.

OK
> > esac
> > done < $KDUMP_CONFIG_FILE
> > for node in $FENCE_KDUMP_NODES; do
> > if [ "$node" = "$hostname" ]; then
> > 	echo "Option fence_kdump_nodes cannot contain $hostname"
> > 	return 1
> > fi
> > done
> > return 0
> >
> > +}
> >
> > function start()
> >  {
> >    check_config
> > @@ -609,6 +634,12 @@ function start()
> >    	fi
> >    fi
> > check_fence_kdump_config
>
> I think this should be part of check_config(). So that all configuration
> checks can be in one single function. So please this call inside
> check_config().

OK
> Thanks
> Vivek