Mailing List Archive

Hi,

On Thu, Nov 10, 2011 at 10:27:36AM +0100, Florian Haas wrote:
> On 2011-11-09 12:02, Martin Gerhard Loschwitz wrote:
> > Hello everybody,
> >
> > I wrote an asterisk OCF resource agent which I am hereby putting up
> > for discussion. Any feedback is welcome.
> >
> > It's available from
> > https://github.com/fghaas/resource-agents/blob/master/heartbeat/asterisk
>
> Let's move this thread to the -dev list where it really belongs.
>
> FWIW, I consider this RA in pretty good shape -- I did review it rather
> thoroughly and sent a few patches, for which Martin was kind enough to
> include me, undeservingly, in the authors list. Feedback from others
> would still be very much appreciated (even if it's just a "+1 for
> merge"). Thanks!

Just a few remarks:

Ending commands with ';' is not necessary:

return $OCF_ERR_INSTALLED;

i.e. ';' serves as a command separator. (:%s/;$//)

This construct looks a bit unusual:

if [ ! $? -eq 0 ]; then

More direct would be:

if [ $? -ne 0 ]; then

Is this necessary (in asterisk_status):

if [ -d /proc -a -d /proc/1 ]; then
[ "u$pid" != "u" -a -d /proc/$pid ]
else
ocf_run kill -s 0 $pid
fi

Why not just:

kill -s 0 $pid

Line 273 in monitor is going to produce a lot of logging, better
reduce severity to debug:

ocf_log info "Asterisk PBX monitor succeeded";

In asterisk_start() $ASTRUNDIR is first created using install(8),
then again checked in lines 292-296 and created using mkdir,
chown, etc. Superfluous.

Start may exit with some arbitrary error code (line 324 in
asterisk_start()).

Perhaps to move all local statements to the top of the function
in asterisk_start().

[nitpicking] start_wait is not needed, why not just

[ $rc -eq $OCF_SUCCESS ] && break

Should check content of $pid before line 377 in stop.

Thanks for great work!

Cheers,

Dejan

> Cheers,
> Florian
>
> --
> Need help with High Availability?
> http://www.hastexo.com/now
> _______________________________________________________
> 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/
_______________________________________________________
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/

Hi Dejan,

thanks for the feedback! We've worked in most of your suggested changes,
see below:

On 2011-11-10 13:14, Dejan Muhamedagic wrote:
> Hi,
>
> On Thu, Nov 10, 2011 at 10:27:36AM +0100, Florian Haas wrote:
>> On 2011-11-09 12:02, Martin Gerhard Loschwitz wrote:
>>> Hello everybody,
>>>
>>> I wrote an asterisk OCF resource agent which I am hereby putting up
>>> for discussion. Any feedback is welcome.
>>>
>>> It's available from
>>> https://github.com/fghaas/resource-agents/blob/master/heartbeat/asterisk
>>
>> Let's move this thread to the -dev list where it really belongs.
>>
>> FWIW, I consider this RA in pretty good shape -- I did review it rather
>> thoroughly and sent a few patches, for which Martin was kind enough to
>> include me, undeservingly, in the authors list. Feedback from others
>> would still be very much appreciated (even if it's just a "+1 for
>> merge"). Thanks!
>
> Just a few remarks:
>
> Ending commands with ';' is not necessary:
>
> return $OCF_ERR_INSTALLED;
>
> i.e. ';' serves as a command separator. (:%s/;$//)

https://github.com/fghaas/resource-agents/commit/088ba39b855d4ca6375a17500aa0c0e1a2578db8#heartbeat/asterisk

> This construct looks a bit unusual:
>
> if [ ! $? -eq 0 ]; then
>
> More direct would be:
>
> if [ $? -ne 0 ]; then

https://github.com/fghaas/resource-agents/commit/bbe7a0ba38d366b25067b09141208087a9e44850#heartbeat/asterisk

> Is this necessary (in asterisk_status):
>
> if [ -d /proc -a -d /proc/1 ]; then
> [ "u$pid" != "u" -a -d /proc/$pid ]
> else
> ocf_run kill -s 0 $pid
> fi
>
> Why not just:
>
> kill -s 0 $pid

https://github.com/fghaas/resource-agents/commit/d77afe185d9cec53388c2248ce3b290f95e4cad5

> Line 273 in monitor is going to produce a lot of logging, better
> reduce severity to debug:
>
> ocf_log info "Asterisk PBX monitor succeeded";

https://github.com/fghaas/resource-agents/commit/cf130ce1a3d1b9502ad07df6361f611a256ee560#heartbeat/asterisk

> In asterisk_start() $ASTRUNDIR is first created using install(8),
> then again checked in lines 292-296 and created using mkdir,
> chown, etc. Superfluous.
>
> Start may exit with some arbitrary error code (line 324 in
> asterisk_start()).
>
> Perhaps to move all local statements to the top of the function
> in asterisk_start().
>
> [nitpicking] start_wait is not needed, why not just
>
> [ $rc -eq $OCF_SUCCESS ] && break

https://github.com/fghaas/resource-agents/commit/80ea432336a56cf2680f30c389b06c20f43eef79#heartbeat/asterisk

> Should check content of $pid before line 377 in stop.

I'm unsure what you're suggesting here.

- Just check that the pid file is non-empty?
- Check whether its contents are numeric?
- Read the pid and do a kill -0 before kill -TERM?

Cheers,
Florian

--
Need help with High Availability?
http://www.hastexo.com/now
_______________________________________________________
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/

On Thu, Nov 10, 2011 at 04:11:16PM +0100, Florian Haas wrote:
> Hi Dejan,
>
> thanks for the feedback! We've worked in most of your suggested changes,
> see below:

> > More direct would be:
> >
> > if [ $? -ne 0 ]; then

$? in a test is almost always an error.
Because you lose the actual value it had.
So rather ex=$?, [ $ex ... ], and add $ex to the log message.

Yo have that error also later in "Asterisk start command failed: $?",
that will always log 0.

compare the output of the next two lines.
( (exit 7); if [ $? -ne 0 ]; then echo "ALWAYS 0!! exit code: $?"; fi )
( (exit 7); ex=$?; if [ $ex -ne 0 ]; then echo "exit code: $ex"; fi )

Btw,
"User $OCF_RESKEY_user doesn't exit"
there is an s missing.


--
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com
_______________________________________________________
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/

Hey Florian,

On Thu, Nov 10, 2011 at 04:11:16PM +0100, Florian Haas wrote:
> Hi Dejan,
>
> thanks for the feedback! We've worked in most of your suggested changes,
> see below:
>
> On 2011-11-10 13:14, Dejan Muhamedagic wrote:
> > Hi,
[...]
> > Start may exit with some arbitrary error code (line 324 in
> > asterisk_start()).

I was referring to this, it should really be fixed:

ocf_run ${OCF_RESKEY_binary} -G $OCF_RESKEY_group -U
$OCF_RESKEY_user \
-C $OCF_RESKEY_config \
$OCF_RESKEY_additional_parameters \
$asterisk_extra_params

if [ $? -ne 0 ]; then
ocf_log err "Asterisk PBX start command failed: $?"
return $?
fi

You need to:

rc=$?
if [ $rc -ne 0 ]; then
ocf_log err "Asterisk PBX start command failed: $rc"
return $OCF_ERR_GENERIC
fi

[...]
> > Should check content of $pid before line 377 in stop.
>
> I'm unsure what you're suggesting here.
>
> - Just check that the pid file is non-empty?
> - Check whether its contents are numeric?

Both? Perhaps it's not necessary though, depending on the
outcome of _status. But perhaps still better not to assume, but
check anyway. Perhaps we need a helper function for this in
ocf-shellfuncs, sth like ocf_is_pidfile_valid.

> - Read the pid and do a kill -0 before kill -TERM?

No :) I think that would be an overkill.

Cheers,

Dejan

> Cheers,
> Florian
>
> --
> Need help with High Availability?
> http://www.hastexo.com/now
> _______________________________________________________
> 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/
_______________________________________________________
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/

Hi Lars,

Pity I didn't see this earlier, could've saved meself some time :)

On Thu, Nov 10, 2011 at 04:33:02PM +0100, Lars Ellenberg wrote:
> On Thu, Nov 10, 2011 at 04:11:16PM +0100, Florian Haas wrote:
> > Hi Dejan,
> >
> > thanks for the feedback! We've worked in most of your suggested changes,
> > see below:
>
> > > More direct would be:
> > >
> > > if [ $? -ne 0 ]; then
>
> $? in a test is almost always an error.

Unless you don't need the outcome later.

> Because you lose the actual value it had.
> So rather ex=$?, [ $ex ... ], and add $ex to the log message.
>
> Yo have that error also later in "Asterisk start command failed: $?",
> that will always log 0.
>
> compare the output of the next two lines.
> ( (exit 7); if [ $? -ne 0 ]; then echo "ALWAYS 0!! exit code: $?"; fi )
> ( (exit 7); ex=$?; if [ $ex -ne 0 ]; then echo "exit code: $ex"; fi )
>
> Btw,
> "User $OCF_RESKEY_user doesn't exit"
> there is an s missing.

"user doesn't exit" sounds good too ;-)

Cheers,

Dejan

>
> --
> : Lars Ellenberg
> : LINBIT | Your Way to High Availability
> : DRBD/HA support and consulting http://www.linbit.com
> _______________________________________________________
> 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/
_______________________________________________________
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/

On 2011-11-10 17:08, Dejan Muhamedagic wrote:
> Hi Lars,
>
> Pity I didn't see this earlier, could've saved meself some time :)
>
> On Thu, Nov 10, 2011 at 04:33:02PM +0100, Lars Ellenberg wrote:
>> On Thu, Nov 10, 2011 at 04:11:16PM +0100, Florian Haas wrote:
>>> Hi Dejan,
>>>
>>> thanks for the feedback! We've worked in most of your suggested changes,
>>> see below:
>>
>>>> More direct would be:
>>>>
>>>> if [ $? -ne 0 ]; then
>>
>> $? in a test is almost always an error.
>
> Unless you don't need the outcome later.

I'm with you, however Lars did evidently spot the one occasion where we
used $? twice trying to get the same return code. So he wins. :)

https://github.com/fghaas/resource-agents/commit/2cbb26648c133ce04b0d51e439c41541dac039e1

I left one invocation in there: "asterisk_validate || exit $?". I hope
that one is acceptable. :)

>> Btw,
>> "User $OCF_RESKEY_user doesn't exit"
>> there is an s missing.
>
> "user doesn't exit" sounds good too ;-)

https://github.com/fghaas/resource-agents/commit/1568a990dfcac03ddfe5785c5d65940ed230068c

We also tossed in a few more changes:

Properly handle multiple instances of the "astcanary" watchdog daemon:
https://github.com/fghaas/resource-agents/commit/c257d4a57f9131a4353143991fe101f02b51d790

Remove a pointless "return $?"
https://github.com/fghaas/resource-agents/commit/812f2b55b9a7a9f8bdd270cca8d037f07f0e980a

Fix a couple of log messages, and exit where we shouldn't return:
https://github.com/fghaas/resource-agents/commit/58b5f55da4ac5537acf9a36b56f8b35f2b96da56

Cheers,
Florian


--
Need help with High Availability?
http://www.hastexo.com/now
_______________________________________________________
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/

Just FYI, I noticed I erroneously put the asterisk changes in the master
branch on my github repo; I've now moved them to a separate "asterisk"
branch. The direct links to commits, which I posted earlier, should
still work as the SHA IDs are unchanged. They just point to commits in a
different branch now.

For those just tuning in, the current state of the RA is here:

https://github.com/fghaas/resource-agents/blob/asterisk/heartbeat/asterisk

Cheers,
Florian

--
Need help with High Availability?
http://www.hastexo.com/now
_______________________________________________________
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/