I will have a look into this today, and fix these things. Thx for all
Some comments inline...
On 25.11.2010 09:31, Florian Haas wrote: > On 2010-11-24 19:46, Frank Lazzarini wrote:
>> sure no problem we can name it firebird.
> OK, firebird it is.
>> Packaging it with the firebird
>> packages I don't really find a good idea, because not every user that
>> needs firebird 2.5 will use it in a cluster.
> That's why there would typically be a subpackage RPM named
> firebird-resource-agent or whatever, and no-one would be obliged to
> install that.
Yes this would be an idea but I would really appreciate beeing in the
cluster-agents package. >> Therefore I would
>> appreciate it if it would be packages with the pacemaker package, if
>> that package includes all the ocf RA's.
> It's not pacemaker, it's the resource-agents (on Debian: cluster-agents)
> package, but sure, we can ship it.
>> Thanks again for the support.
> A few more issues I noticed:
> 1. you use sudo, any particular reason why you're not doing "su -c" like
> all the other RAs do? Besides, you should add a check_binary test (for
> either sudo or su) to your validate function.
I had some problems using "su -c" but I think it was because the pidfile
couldn't be written,
the fbguard process isn't that clever on giving out some error messages
it just exits. Gonna
go and change that to su, or like you say check wether su or sudo exists. > 2. Even if starting the daemon fails, you still pretend it succeeded:
>> # start fbguard with the specified user
>> sudo -u $OCF_RESKEY_user $OCF_RESKEY_binary -pidfile $OCF_RESKEY_pid -daemon -forever
>> return $OCF_SUCCESS
> You can either exit on error after sudo (with "|| exit
> $OCF_ERR_GENERIC"), or spin on monitor after start, or both.
> 3. This too looks fishy:
>> if [ -f $OCF_RESKEY_pid ]
>> kill `cat $OCF_RESKEY_pid`
>> if [ $? = 0 ]; then
>> return $OCF_SUCCESS
> All that $? tells you is whether you were able to _send_ the signal. It
> says nothing about whether the process did the right thing with the
> signal, and it _certainly_ does not imply that the process shut down
> completely. Returning successfully here is premature. Ditch that if
> block. Because just below:
>> while firebird25_monitor; do
>> ocf_log debug "Resource has not stopped yet, waiting"
>> sleep 1
> ... you're doing the right thing and ask your monitor function to see if
> the daemon is in fact dead (and if it's not, you wait till it is).
> 4. In monitor, you do this:
>> pid=`cat $OCF_RESKEY_pid`
>> kill -s 0 $pid>/dev/null 2>&1
> ... and then test based on $rc. Now, kill -s 0 is generally fine to test
> whether a process with that pid is currently running, but the problem
> with that approach is that it _will_ succeed on a process that is a
> zombie, which really is a failed resource. Thus, it's often a good idea
> to combine this with an additional test, preferably one where you can
> ask the daemon, or a client, whether the application is really
> operational. Does Firebird offer such a facility?
I am not really sure about this, the only thing I can think of is
something similar than the mysql
resource agent does, which is doing a sql on some Database system
tables, but Firebird
doesn't really offer that possibility as those tables only exist in the
databases it self, so if there
is no database currently attached to the firebird server I wouldn't have
these system tables. But
I will have a look into this, and maybe figure out a way to check this. > 5. There's an easier way to do this:
>> if [ ! -x $OCF_RESKEY_binary ]; then
>> ocf_log err "fbguard binary $OCF_RESKEY_binary does not exist or is not executable"
>> exit $OCF_ERR_INSTALLED
> You just replace those 4 lines with "check_binary $OCF_RESKEY_binary".
Alrighty didn't know of that function. Hmm this just gives me an idea,
remember I use a function to check wether the pid
directory is writable or not, wouldn't that be an idea to add to
.ocf-binaries ? Like I say just an idea. > 6. Also,
>> getent passwd $OCF_RESKEY_user>/dev/null 2>&1
>> if [ ! $? -eq 0 ]; then
> can just be replaced with
> if ! getent passwd $OCF_RESKEY_user>/dev/null 2>&1; then
> ...for additional brevity.
> As far as I'm concerned, once you submit a version with these issues
> fixed, this RA is in good shape to be merged. Lars, Dejan, and the other
> usual suspects, if you have any objections to pushing this, please let
> me know.
I will get to it, this afternoon to correct these things :), once again
thx for all your comments. > Cheers,
> Linux-HA-Dev: Linux-HA-Dev@lists.linux-ha.org
> Home Page: http://linux-ha.org/