I think I've covered most of the input I've gotten so far, in lack of git access internally here's another pastebin. Please let me know if there's any additional changes that should be made.
http://pastebin.com/QMuyTRWB On Tue, Sep 28, 2010 at 10:28 PM, Jonathan Petersson <jpeters...@garnser.se> wrote: > 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/
