[Linux-HA] Apache RA patch and IPcloneaddr RA

[Lars Marowsky-Bree]

On 2006-07-31T14:59:01, Tim Verhoeven <tim.verhoeven.be at gmail.com> wrote:

Hi Tim, thanks!

But everyone, please post code & patches to the -dev list; otherwise,
they might get lost even more easily ;-)

I've got some feedback on these changes:

> --- apache.orig	2006-07-31 14:40:00.602303568 +0200
> +++ apache	2006-07-26 16:29:43.981539801 +0200
> @@ -58,6 +58,8 @@
>  HTTPDOPTS="-DSTATUS"
>  DEFAULT_IBMCONFIG=/opt/IBMHTTPServer/conf/httpd.conf
>  DEFAULT_NORMCONFIG="/etc/apache2/httpd.conf"
> +DEFAULT_STARTTRIES=3
> +DEFAULT_STARTWAIT=1
>  #
>  # You can also set
>  #	HTTPD
> @@ -518,6 +549,8 @@
>    STATUSURL="$OCF_RESKEY_statusurl"
>    CONFIGFILE="$OCF_RESKEY_configfile"
>    OPTIONS="$OCF_RESKEY_options"
> +  STARTWAIT="$OCF_RESKEY_startwait"
> +  STARTTRIES="$OCF_RESKEY_starttries"
>  else
>    usage
>  fi
> @@ -572,6 +605,16 @@
>    *)		;;
>  esac
>  
> +case "$STARTWAIT" in
> +  "") STARTWAIT=$DEFAULT_STARTWAIT;;
> +  *)		;;
> +esac
> +
> +case "$STARTTRIES" in
> +  "") STARTTRIES=$DEFAULT_STARTTRIES;;
> +  *)		;;
> +esac

You can more easily use the ${VALUE:=DEFAULT} notation, that's a bit
easier to read than the case statement:

	STARTWAIT=${OCF_RESKEY_startwait:=1}


On the clone RA:

> IPADDR2=${OCF_ROOT}/resource.d/heartbeat/IPaddr2
> ME=${OCF_RESOURCE_TYPE}
> IPS=${OCF_RESKEY_ips}
> IPLEN=$(echo ${IPS} | wc -w)
> declare -a IPLIST=( $IPS )
> CLONENR=$OCF_RESKEY_CRM_meta_clone

The array syntax is bash, not pure bourne shell, or is it? In that case,
changing the #! to /bin/bash seems sane. (It is probably safe to assume
that this is installed on all Linux distributions anyway.)

> meta_data() {
> 	cat <<END
> <?xml version="1.0"?>
> <!DOCTYPE resource-agent SYSTEM "ra-api-1.dtd">
> <resource-agent name="IPcloneaddr">
> <version>1.0</version>

... it'd be nicer if you truly delegated this call to the IPaddr2
script, and then just inserted your own bits. Yes, inheritance in shell
scripts sucks ;-) But, inserting something should be just about doable.
That'll make your script more resilient to future changes.

(Hm, I think while I'm trying to get you coding, I might as well try to
convince you that a few general library functions to make inheritance
easier would be much welcome ;-)

> <actions>
> <action name="start"   timeout="90" />
> <action name="stop"    timeout="100" />
> <action name="status" depth="10"  timeout="20s" interval="10s" start-delay="5s" />
> <action name="monitor" depth="10"  timeout="20s" interval="10s" start-delay="5s" />
> <action name="meta-data"  timeout="5s" />
> <action name="validate-all"  timeout="20s" />
> </actions>
> </resource-agent>

status is superfluous for an OCF RA, btw ;-)

(And 10s interval seems stressing.)

> # See if we are root or not
> if
>   case $__OCF_ACTION in
>     start|stop)		ocf_is_root;;
>     *)			true;;
>   esac
> then
>   : YAY!
> else
>   ocf_log err "You must be root for $__OCF_ACTION operation."
>   exit $OCF_ERR_PERM
> fi

As you're just inheriting from IPaddr2, you might as well leave that
check out here.

> 
> # See if we can find the IPaddr2 resource agent
> if [ ! -x "${IPADDR2}" ]; then
>   ocf_log err "I cannot find the IPaddr2 resource agent in ${OCF_ROOT}/resource.d/heartbeat/."
>   exit $OCF_ERR_INSTALLED
> fi

Might as well log the $IPADDR2 name here too, not just the directory.

> echo "IPLEN = $IPLEN"
> echo "Max clone = $OCF_RESKEY_CRM_meta_clone_max"

echo statements of course need to be removed before inclusion ;-)

> # See if the number of given IP's is the same as the max number of clones
> if [ $IPLEN -ne $OCF_RESKEY_CRM_meta_clone_max ] ; then
>   ocf_log err "The number of provides IP addresses if different from the number of configured clones."
>   exit $OCF_ERR_ARGS
> fi

> # See if we are run a cloned resource
> if [ -z "$OCF_RESKEY_CRM_meta_clone_max" ]; then
>   ocf_log err "${ME} must be run as a clone."
>   exit $OCF_ERR_GENERIC
> fi

The metadata operation of course needs to work even without these set.
Only check these for start/stop/monitor please.

(Or maybe "notify", if you ever care for that.)

> #######################################################################
> 
> # Reconfigure the environment to match IPaddr2 expectations.
> echo ${IPLIST[$CLONENR]}

See above.

> ipclone_start() {
> 	${IPADDR2} start
> 	#return $OCF_SUCCESS
> }
> 
> ipclone_stop() {
> 	${IPADDR2} stop
> 	#return $OCF_SUCCESS
> }
> 
> ipclone_status() {
> 	${IPADDR2} status
> 	#return $OCF_SUCCESS
> }
> 
> ipclone_monitor() {
> 	${IPADDR2} monitor
> 	#return $OCF_SUCCESS
> }
> 
> ipclone_validate() {
> 	${IPADDR2} validate
> 	#return $OCF_SUCCESS
> }
> 
> case $__OCF_ACTION in
> meta-data)	meta_data
> 		;;
> start)		ipclone_start
> 		;;
> stop)		ipclone_stop
> 		;;
> status)		ipclone_status
> 		;;
> monitor)	ipclone_monitor
> 		;;
> validate-all)	ipclone_validate
> 		;;

This you can probably simply do with

start|stop|monitor|status|validate-all)	
	 	${IPADDR2} $__OCF_ACTION
		;;

and shrink the code down quite a bit.


Thanks again,
	Lars

-- 
High Availability & Clustering
SUSE Labs, Research and Development
SUSE LINUX Products GmbH - A Novell Business	 -- Charles Darwin
"Ignorance more frequently begets confidence than does knowledge"