Hi Neil,
I still could not find the necessary changes within usr/src/pkgdefs/* in
your webrev - so it is not complete and I doubt it will properly build
as-is.
Which means my review is not yet complete.
Apart from that, here is my review to the existing files:
general: most scripts do not have the .ksh extension, although your
Makefile does expect them!
usr/src/cmd/ha-services/gds-agents/ids/control_ids
- your file is named control_ids, but the Makefile really expects
control_ids.ksh - need to rename!
- line 108, since you later "return 1", doesn't this need to be
of servity daemon.error instead of daemon.notice?
The SCMSGS is missing because this is used with other agents
as well?
- line 133, although you setup $OUTPUT (which logs into
${LOGFILE}),
where is that output ever been printed out? In a former gds
template
we had log_message(), which you don't have. What is logging
$LOGFILE to syslog in the end?
usr/src/cmd/ha-services/gds-agents/ids/functions:
- your file is named functions, but the Makefile really expects
functions.ksh - need to rename!
- line 95 and 109, your are hard coding "informix" as user and
group that must exist. Is that really a Informix requirement?
Wouldn't it make sense to make it configurable if this changes
for future versions? If not, can you please add an comment that
Informix really requires this, maybe with a pointer to the docs?
- line 103 and 117, the @user_action is not helpfull. Does this
mean the
user must create this user manually? Or is this something
performed
by the Informix installation procedure?
- line 127, if the user does not exists, wouldn't that create an
error message to stderr (ie. show on the console)?
- line 129, your if clause looks strange - what are you
checking for?
Maybe there is a "-n" missing?
- line 181, you should quote ${INFORMIXDIR}
- line 192, missing \ at the end of the line!
- line 193, you should quote ${INFORMIXDIR}
- line 197, maybe a comment would be helpful as of how the
ONCONFIG file looks like as an example - that would help to
understand what you validate for in the following.
- line 201 and 216 and 246, note that your grep would also match
names where the one you rep for is included, but does have a
different pre- or postfix - it is not checking for an exact
match.
Is that intended?
- line 281, doesn't the rm create a failure message to stderr if
${LOGFILE} does not exist? Maybe redirect the output to
/dev/null?
- line 294 - when would check_ids actually return 1?
Looking at the function I can not see where.
- line 32ff., I am a bit surprised by the user_action, especially
"The Solaris Cluster resource will attempt to restart the
Informix Server."
That is not always correct, is it? If "oninit -y" always
fails, then
this will result into failed or stop_failed?
- line 349, the servity for syslog should be "daemon.error"
- line 408/410 - can you explain with a comment why you do the
"grep ."?
- general on check_ids() - you rely a lot on the output of onstat.
Is that a stable interface to parse? If not, any way to get the
various status via a stable interface?
- general on check_ids() - you nearly never quote the variables you
provide to scds_syslog - you should quote them!
- line 469, missing " before HANG_SYSTEM"
- line 485, in the comment you say with the "HANG_SYSTEM" or
"MEDIA FAILURE" the DBA intervention is necessary - but you keep
restarting the resource?
- line 490-502, I find the comment confusing - you say
something about
return code 1 - but in the following code it well rc will never
be set to 1? In fact you either set 100 or 0. Is that intended?
If so, please correct the comment.
- line 508-521, same thing as with 490-502 - I do not fully
understand
the comment return code 1.
- line 567 and 568, the check is not specifying an operation,
don't you need -n in the [ ] test?
- line 634ff - again the confusion about return 1 - I have not yet
understood when check_ids() does return 1 in the code.
- line 669-674 distincts between zone and non-zone systems,
while later (line 677ff) you always call the -mcopbZ to ipcs.
Is that intended? Doesn't this limit the use of the agent to
Solaris 10 and newer? The whole block line 677-704 is S10 only.
- usr/src/cmd/ha-services/gds-agents/ids/ids_register
- your file is named ids_register, but the Makefile really expects
ids_register.ksh - need to rename!
- line 29, you should not put temp files into /opt. I suggest
you use /tmp or /var/tmp for that. but I guess you need it there
for the register within the non-global zone / HA Container case.
In that case, why not transfer it (again to /tmp or /var/tmp) of
the non-global zone via the method shown at line 224 of
http://src.opensolaris.org/source/xref/ohac/ohacds/usr/src/cmd/ha-services/gds-agents/mys/ha_mysql_register.ksh
- line 81, 87, 93, 102, 137, 142 and 151 does always exit 1 without
cleaning up the temp file - which would disturb a clean pkgrm.
This is also an argument to place it on /tmp or /var/tmp.
- line 104, you need to grep for "^Zonename=", otherwise you might
match a comment or old commented out entry.
- line 110-118 - do you really want the leading tab there?
- line 129, you just check if the state of the resource is
"OFFLINE".
But the "clrs delete ${RS}" does only succeed if the resource
is also disabled - so you also need to check for -O ON_OFF_SWITCH
via scha_resource_get if the resource is DISABLED.
- usr/src/cmd/ha-services/gds-agents/ids/ids_smf_register
- your file is named ids_smf_register, but the Makefile really
expects
ids_smf_register.ksh - need to rename!
- line 125, you should check if $1 is really existing.
- usr/src/cmd/ha-services/gds-agents/ids/ids_smf_remove
- your file is named ids_smf_remove, but the Makefile really
expects
ids_smf_remove.ksh - need to rename!
- line 44 and 50, you should quote the variables
- line 56, shouldn't the check be
[ "${rc}" -ne 0 ] && exit ${rc}
All for now.
Greets
Thorsten
Thorsten Frueauf wrote:
> Hi Neil et al,
>
> one early comment about the webrev, while browsing through it I think
> you are missing the package source part within usr/src/pkgdefs/* for the
> new ids package.
>
> Greets
> Thorsten
>
> Neil Garthwaite wrote:
>> Hi Everyone,
>>
>> The HA-Informix project page
>> http://opensolaris.org/os/project/ha-informix has been updated to
>> include the initial code for the HA-Informix agent.
>>
>> I'd welcome a code review for this agent, a webrev can be found here
>>
>> http://cr.opensolaris.org/~neilg/ohacds-informix
>>
>> Alternatively, the agent is available for download. Please use the
>> accompanying cheatsheet if you want to try out the agent.
>>
>> http://opensolaris.org/os/project/ha-informix/cheatsheet_ohac_ids.pdf
>>
>> Finally, many thanks to everyone that reviewed the Design Doc and Caller
>> Sequence Doc, I've updated those docs to reflect your comments.
>>
>> Regards
>> Neil
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Sitz der Gesellschaft:
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Amtsgericht Mu"nchen: HRB 161028
Gescha"ftsfu"hrer: Thomas Schro"der, Wolfgang Engels, Dr. Roland Bo"mer
Vorsitzender des Aufsichtsrates: Martin Ha"ring
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended
recipient, please contact the sender by reply email and destroy
all copies of the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
