----- Original Message -----
From: "Vivek Goyal" <vgoyal(a)redhat.com>
To: "Martin Perina" <mperina(a)redhat.com>
Cc: kexec(a)lists.fedoraproject.org
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 <mperina(a)redhat.com>
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