Hi Stephane,
This is a very, very late response to your contribution. Very
sorry about that.
On Tue, Dec 23, 2008 at 01:26:36PM +0100, Stephane Neveu wrote:
> #######################################################################
> # OCF parameters:
> # OCF_RESKEY_ldap_bin : Executable file
> # OCF_RESKEY_ldap_conf : Configuration file
> # OCF_RESKEY_ldap_pidfile: Process id file
> # OCF_RESKEY_ldap_port : Port number
> # OCF_RESKEY_ldap_user : Ldap user
> # OCF_RESKEY_ldap_urls : LDAP URL's (ldap and/or ldaps)
> #
> # OCF_RESKEY_ldap_bin, OCF_RESKEY_ldap_conf, OCF_RESKEY_ldap_pidfile
> # OCF_RESKEY_ldap_port OCF_RESKEY_ldap_user must be specified.
ldap_bin could be optional (already defaults to /usr/sbin/slapd).
It would be good to fetch ldap_pidfile from the configuration
file (duplicating configuration is always risky).
ldap_user should be optional. ldap_group should probably be
implemented for completeness.
I'd leave out ldap_port and rely on the pidfile. That's what most
of our resource agents do. At any rate, you can't support
multiple URLs properly with this parameter.
ldap_urls is required according to the code (and it should be),
so the comment should be updated.
> metadata_ldap() {
> cat <<END
Please pay attention to the required/unique attributes.
> <?xml version="1.0"?>
> <!DOCTYPE resource-agent SYSTEM "ra-api-1.dtd">
> <resource-agent name="ldap">
> <version>1.0</version>
> <longdesc lang="en">The OCF resource agent of ldap</longdesc>
> <shortdesc lang="en">The RA for ldap</shortdesc>
It should be "for OpenLDAP" I guess. There are other ldap servers
out there.
> <parameters>
>
> <parameter name="ldap_bin" required="1" unique="0">
> <longdesc lang="en">
> This is a required parameter.
Not necessary to spell-out. It's already in the attributes.
> start_ldap()
> {
> typeset status
>
> monitor_ldap
> status=$?
> if [[ $status != $OCF_NOT_RUNNING ]]; then
> return $status
> fi
>
> set -- "$LDAP_OPTS"
> ocf_run $LDAP_BIN -f $LDAP_CONF -h "$LDAP_URLS:///" -u $LDAP_USER "$@"
This is a bit strange. Why not just:
ocf_run $LDAP_BIN -f $LDAP_CONF -h "$LDAP_URLS:///" -u $LDAP_USER $LDAP_OPTS
> status=$?
> sleep 1
> if [[ $status != $OCF_SUCCESS ]]; then
> return $status
> fi
>
> while true; do
> get_pid
> if is_pid_found; then
> return $OCF_SUCCESS
> else
> ocf_log info "$LDAP_BIN:No pid found after start"
> fi
> done
This while loop would probably be too noisy. Perhaps:
while sleep 1; do
monitor_ldap
rc=$?
if [ $rc -eq $OCF_SUCCESS ]; then
return $OCF_SUCCESS
elif [ $rc -ne $OCF_NOT_RUNNING ]; then
ocf_log err ...
fi
done
Or something like that. I think that there are quite a few
examples of this code in other OCF RAs.
> return $OCF_ERR_GENERIC
> }
>
> stop_ldap()
> {
> monitor_ldap
> `kill $LDAP_PID`
Eh? Why backquotes? Anyway, that doesn't look right. Perhaps:
if monitor_ldap; then
kill $LDAP_PID # LDAP_PID is always non-empty?
else
...
fi
> while true; do
> sleep 1
> get_pid
> if [ is_ldap_dead != "1" ]; then
> rm -f ${LDAP_PIDFILE}
> return $OCF_SUCCESS
> fi
> ocf_log info "LDAP stopped"
> done
Not completely correct. The loop is there to make sure that the
slapd process exited. So, both the return and the log statement
should be replaced by break and moved after the loop.
>
> }
>
> status_ldap()
> {
> monitor_ldap
> return $?
> }
This is equivalent to monitor_ldap.
>
>
> validate_all_ldap()
> {
> return $OCF_SUCCESS
> }
>
> if [ -z "$LDAP_CONF" ]; then
> ocf_log err "LDAP_CONF is not defined"
> exit $OCF_ERR_CONFIGURED
> fi
[...]
> if [ -z "$LDAP_URLS" ]; then
> ocf_log err "LDAP_URLS is not defined"
> exit $OCF_ERR_CONFIGURED
> fi
This should be put into validate_all_ldap, along with the check
if the binary exists. Then, validate_all_ldap should be called
before all other actions, apart from meta-data and usage.
monitor/status should also be treated better, i.e. if the
binary's missing then the service should be deemed stopped. See
the apache OCF RA for example (probably other RAs too).
> COMMAND=$1
>
> case "$COMMAND" in
> start)
> start_ldap
> status=$?
> exit $status
> ;;
> stop)
> stop_ldap
> status=$?
> exit $status
> ;;
> status)
> status_ldap
> status=$?
> if status; then
> ocf_log info "slapd is running"
> elif [status -eq 7]; then
> ocf_log info "slapd is stopped"
> else
> ocf_log info "slapd is dead"
> fi
> ;;
> monitor)
> monitor_ldap
> status=$?
> exit $status
> ;;
> meta-data)
> metadata_ldap
> ;;
> validate-all)
> validate_all_ldap
> exit $?
> ;;
> *)
> usage
> ;;
> esac
>
>
slapd can work in both multi-master and one-master/multi-slave
mode; if would be good to support one of these modes.
Many thanks and sorry again for such a huge delay.
Cheers,
Dejan
> _______________________________________________
> Linux-HA mailing list
> [email protected]
> http://lists.linux-ha.org/mailman/listinfo/linux-ha
> See also: http://linux-ha.org/ReportingProblems
_______________________________________________
Linux-HA mailing list
[email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha
See also: http://linux-ha.org/ReportingProblems
