Great feedback, thanks a bunch, I'll get to it making the mods recommended asap.
On Tue, Sep 28, 2010 at 10:23 PM, Florian Haas <florian.h...@linbit.com> wrote:
> Hi Jonathan,
>
> as this discussion applies to a new RA, let's move it to the -dev list.
> Very helpful contribution, thanks a lot. A few comments below.
>
>> # Fill in some defaults if no values are specified
>> OCF_RESKEY_bin_default="/usr/sbin/conntrackd"
>> OCF_RESKEY_cfg_default="/etc/conntrackd/conntrackd.conf"
>> OCF_RESKEY_lck_default="/var/lock/conntrack.lock"
>
> Please use self-explanatory parameter names wherever possible; makes
> life a whole lot easier for your users. I'd suggest "binary", "config",
> and "lockfile" instead of "bin", "cfg", and "lck".
>
>> CONNTRACKD="${OCF_RESKEY_bin} -C ${OCF_RESKEY_cfg}"
>
> This assignment should come _after_ you initialize those variables with
> their defaults.
>
>> : ${OCF_RESKEY_bin=${OCF_RESKEY_bin_default}}
>> : ${OCF_RESKEY_cfg=${OCF_RESKEY_cfg_default}}
>> : ${OCF_RESKEY_lck=${OCF_RESKEY_lck_default}}
>
>
>> meta_data() {
>> cat <<END
>
> This is a pet peeve, but would you mind using EOF for the here document
> marker to make emacs users happy? :)
>
>> <?xml version="1.0"?>
>> <!DOCTYPE resource-agent SYSTEM "ra-api-1.dtd">
>> <resource-agent name="Conntrackd" version="0.5">
>> <version>1.0</version>
>
> Really minor issue here, but the version numbers should agree.
>
>>
>> <longdesc lang="en">
>> This is a Conntrackd resource to manage primary and secondary state between
>> two firewalls in a cluster.
>> </longdesc>
>> <shortdesc lang="en">Manages primary/backup conntrackd state</shortdesc>
>>
>> <parameters>
>> <parameter name="bin" unique="1">
>
> Nope. That one's definitely not unique.
>
>> <longdesc lang="en">
>> Location of conntrackd binary.
>> </longdesc>
>> <shortdesc lang="en">Conntrackd bin</shortdesc>
>> <contect type="string" default="/usr/sbin/conntrackd"/>
>
> You have a variable defined for the default; it's wise to use it here.
>
>> </parameter>
>>
>> <parameter name="cfg" unique="1">
>
> That one isn't unique either, I think.
>
>> <longdesc lang="en">
>> Location of conntrackd configuration file.
>> </longdesc>
>> <shortdesc lang="en">Conntrackd config</shortdesc>
>> <contect type="string" default="/etc/conntrackd/conntrackd.conf"/>
>
> As above.
>
>> </parameter>
>>
>> <parameter name="lck" unique="1">
>
> This one _might_ be unique, though I really don't think so.
>
>> <longdesc lang="en">
>> Location of conntrackd lock-file.
>> </longdesc>
>> <shortdesc lang="en">Conntrackd lock-file</shortdesc>
>> <contect type="string" default="/var/lock/conntrackd.lock"/>
>
> Again, as above.
>
>> # Call monitor to verify that conntrackd is running
>> conntrackd_monitor
>>
>> if [ $? = $OCF_SUCCESS ]; then
>
> Use -eq instead of = for numeric comparisons.
>
>>
>> # commit the external cache into the kernel table
>> $CONNTRACKD -c
>> if [ $? -eq 1 ]
>> then
>> return $OCF_ERR_GENERIC
>
> As a general rule, there is hardly ever any need to _return_ error
> codes, you could just exit on them. And, whenever you error out, do so
> after logging an error message with ocf_log err. In your case, it's
> likely that you want to run the conntrackd command, capture its output,
> and log it on error. All of which is easily achieved with ocf_run, like so:
>
> ocf_run $CONNTRACKD -c || exit $OCF_ERR_GENERIC
>
> You can probably apply that to most of your invocations of $CONNTRACKD.
>
>> conntrackd_monitor() {
>>
>> # Define conntrackd_pid variable
>> local conntrackd_pid=`pidof ${OCF_RESKEY_bin}`
>
> Bashism. If you decĺare the resource agent as bourne shell compatible
> (#!/bin/sh), this should be:
>
> local conntrackd_pid
> conntrackd_pid=`pidof ${OCF_RESKEY_bin}`
>
> ... or you force bash with #!/bin/bash.
>
>> # Check for conntrackd lock-file
>> if [ -f $OCF_RESKEY_lck ]; then
>>
>> # Check for conntrackd pid
>> if [ $conntrackd_pid ]; then
>
> I think this is another bashism, Bourne shell would expect [ -n
> "$conntrackd_pid" ].
>
>> # Successfull if lock and pid exists
>> return $OCF_SUCCESS
>> else
>> # Error if pid exists but pid isn't running
>> return $OCF_ERR_GENERIC
>> fi
>> else
>> # False if lock and pid missing
>> $OCF_NOT_RUNNING
>>
>> # Start conntrackd daemon
>> $CONNTRACKD -d
>
> No. It's not the monitor action's job to restart anything. If monitor
> exits with $OCF_NOT_RUNNING, it's Pacemaker's job to recover that
> resource. And it happens to be good at that job. :)
>
>> conntrackd_validate() {
>>
>> # Check if conntrackd binary exists
>> check_binary ${OCF_RESKEY_bin}
>>
>> if [ $? != 0 ]; then
>> return $OCF_ERR_ARGS
>> fi
>
> Not necessary, check_binary already exits with $OCF_ERR_INSTALLED (which
> is the correct error code here) if the binary isn't found.
>
>>
>> # Check if conntrackd config exists
>> if [ ! -f ${OCF_RESKEY_cfg} ]; then
>> return $OCF_ERR_ARGS
>
> This should be $OCF_ERR_INSTALLED.
>
>> fi
>>
>> return $OCF_SUCCESS
>> }
>>
>> case $__OCF_ACTION in
>> meta-data) meta_data
>> exit $OCF_SUCCESS
>> ;;
>> start) conntrackd_start;;
>> stop) conntrackd_stop;;
>> monitor) conntrackd_monitor;;
>> migrate_to) ocf_log info "Migrating ${OCF_RESOURCE_INSTANCE} to
>> ${OCF_RESKEY_CRM_meta_migrate_to}."
>> conntrackd_stop
>> ;;
>> migrate_from) ocf_log info "Migrating ${OCF_RESOURCE_INSTANCE} to
>> ${OCF_RESKEY_CRM_meta_migrated_from}."
>> conntrackd_start
>
> Migrate actions are really not needed here.
>
>> ;;
>> reload) ocf_log err "Reloading..."
>> conntrackd_start
>> ;;
>> validate-all) conntrackd_validate;;
>
> You might want to invoke conntrackd_validate before start, or even
> before any action except metadata, help, and usage.
>
>> usage|help) conntrackd_usage
>> exit $OCF_SUCCESS
>> ;;
>> *) conntrackd_usage
>> exit $OCF_ERR_UNIMPLEMENTED
>> ;;
>> esac
>> rc=$?
>> ocf_log debug "${OCF_RESOURCE_INSTANCE} $__OCF_ACTION : $rc"
>> exit $rc
>>
>
> Thanks. Keep up the good work!
>
> Cheers,
> Florian
>
>
> _______________________________________________
> Linux-HA mailing list
> linux...@lists.linux-ha.org
> http://lists.linux-ha.org/mailman/listinfo/linux-ha
> See also: http://linux-ha.org/ReportingProblems
>
_______________________________________________________
Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
