Hi Hemachandran,
I am fine with the changes as far as I am concerned.
Detlef
Hemachandran Namachivayam wrote:
> Hi Detlef,
>
> Thanks for spotting that. I have made the changes.
>
> -Regards
> Hemachandran
>
> On 06/29/09 20:39, Detlef Ulherr wrote:
>> Sorry but fond some more
>> control_xvm.ksh
>> 45 R*) exit 1;;
>> remove the R
>>
>> Rest looks fine.
>>
>> Again, Sorry I should have spotted it before.
>>
>> Detlef
>>
>> Hemachandran Namachivayam wrote:
>>> Hi Detlef, Neil, Tim , Jonathan and Sreejith,
>>>
>>> Thanks for your valuable comments.
>>>
>>> Detlef, Please see my replies inline.
>>>
>>> On 06/29/09 17:43, Detlef Ulherr wrote:
>>>> Hi Hemachandran,
>>>>
>>>> here is what I found:
>>>>
>>>> spatz:~ sunmicrosystems$ cat feedback
>>>> SUNW.ldom and SUNW.xvm
>>>>
>>>> I would suggest to change the tunability of the start stop, probe
>>>> and validate commands to WHEN_DISABLED.
>>>>
>>>> I know there is nothing to specify right now, but for further
>>>> enhancements it will make things more easy.
>>>>
>>> The start/stop/probe and validate commands are not expected to be
>>> tuned by the administrators. If they are being modified, the agent
>>> would not produce the desired results. Hence they are set to NONE.
>>>> control_xvm.ksh
>>>>
>>>> 28 MYNAME=`basename ${0}`
>>>> 29 MYDIR=`dirname ${0}`
>>>>
>>>> I would change the ` to $(), it improves the ksh93 compliance
>>>>
>>>> validate_xvm.ksh
>>>> 28 MYNAME=`basename ${0}`
>>>> 29 MYDIR=`dirname ${0}`
>>>>
>>>> I would change the ` to $(), it improves the ksh93 compliance
>>>> The same applies in line 90,95 and 100
>>>>
>>> Incorporated the above two changes.
>>>>
>>>> functions.ksh
>>>> 40 PREP=/usr/bin/pgrep
>>>>
>>>> There is a typo in the variable change it to PGREP
>>>>
>>> Fixed it.
>>>> 550 sleep ${START_TIMEOUT} &
>>>> I would use the absolute path for ksh93 compliance, the behavior
>>>> changed. Probably use a command variable.
>>>>
>>> Fixed it.
>>>> 621 if ! ${CCRADM} delkey --key noop_${RESOURCE}
>>>> ${CCR_TABLE} >> $LOGFILE 2>&1
>>>> 622 then
>>>> 623 # SCMSGS
>>>> 624 # @explanation
>>>> 625 # Failed to add the NO-OP flag to CCR.
>>>> 626 # @user_action
>>>> 627 # Check the syslog for further messages.
>>>> 628 # Determine why the NO-OP flag was not added to
>>>> the CCR.
>>>> 629 scds_syslog -p daemon.error -t $(syslog_tag) -m \
>>>> 630 "Failed to delete NO-OP flag for %s domain" \
>>>> 631 "${DOMAIN}"
>>>>
>>>> The explanation is obviously wrong, it fails to delete the NO-OP flag.
>>>>
>>> Fixed it.
>>>> 845
>>>> PROBE_TIMEOUT=$(${SCHA_RESOURCE_GET}-O Extension -R ${RESOURCE} -G
>>>> ${RESOURCEGROUP} Probe_timeout)
>>>>
>>>> 846 847
>>>> PROBE_TIMEOUT=$(/usr/bin/echo ${PROBE_TIMEOUT} | ${AWK} '{print $2}')
>>>> could be doen faster in in one statement:
>>>> PROBE_TIMEOUT=$(${SCHA_RESOURCE_GET}-O Extension -R ${RESOURCE} -G
>>>> ${RESOURCEGROUP} Probe_timeout|tail -1)
>>>>
>>>> It is the probe so efficiency is key.
>>>>
>>> Incorporated the changes.
>>>> 852
>>>> HATIMERUN_TIMEOUT=$((PROBE_TIMEOUT*90/100))
>>>> 853 854
>>>> output=$(${HATIMERUN} -t ${HATIMERUN_TIMEOUT} -k 9 ${PLUGIN_PROBE}
>>>> ${HATIMERUN_TIMEOUT})
>>>> 855 rc=${?}
>>>>
>>>> I know it was already in the code, but I fear that 90 percent of
>>>> the probe timeout might be too long. Given the fact that you have
>>>> been doing things before and you do not take this into account. I
>>>> would suggest to work with SECONDS and reduce the HATIMERUN_TIMEOUT
>>>> by the elapsed seconds. Then you are sure to have 10 seconds rest
>>>> for housekeeping.
>>>>
>>> Incorporated the changes.
>>>> 1269 debug_message "Function: migrate_xvm - End"
>>>> 1270 return ${?}
>>>>
>>>> I do not think that you want to return with the return code of
>>>> debug_message. I would have expected, that you stored the return
>>>> code of the ha_timerun command.
>>>>
>>>> 1287 debug_message "Function: migrate_ldom - End"
>>>> 1288 return ${?}
>>>>
>>>> Dito
>>>>
>>>> 1466 debug_message "Function: shutdown_xvm - End"
>>>> 1467 return ${?}
>>>>
>>>> Dito
>>>>
>>> Fixed the above three changes.
>>>
>>> The updated webrev is @ http://cr.opensolaris.org/~hnamachi/ha-ldom/
>>>
>>> -Thanks and Regards
>>> Hemachandran
>>>> Rest looks good.
>>>>
>>>> Regards
>>>> Detlef
>>>>
>>>> Hemachandran Namachivayam wrote:
>>>>> Hi Neil,
>>>>>
>>>>> Thanks for your review comments. Please see my replies inline.
>>>>>
>>>>> On 06/29/09 13:24, Neil Garthwaite wrote:
>>>>>> Hi Hemachandran,
>>>>>> Many thanks for this. I have just a few comments below, however
>>>>>> please note that I have not been able to verify the x86 non-LDoms
>>>>>> deployment, i.e. I don't have xVM + OHAC installed to test.
>>>>>> - functions.ksh
>>>>>>
>>>>>> Lines 84-86, are you missing SET_DEBUG=
>>>>> Fixed it.
>>>>>>
>>>>>> Lines 543-540, the re-work has replaced ${ADMIN} with a CCR table
>>>>>> entry, as such this message needs to be changed as ${ADMIN}
>>>>>> doesn't exist now.
>>>>>>
>>>>> Fixed it.
>>>>>> Line 912, function stop_domain has variable rc specified as
>>>>>> global, i.e. there is no typeset -i rc=0
>>>>>>
>>>>> Incorporated this.
>>>>>> Line 1219, /opt/SUNWscxvm/bin/ldm_migrate is being run. I can see
>>>>>> an entry within prototype_sparc however I can't see a webrev
>>>>>> entry for this file.
>>>>>>
>>>>> The webrev now is able to list this file in the review.
>>>>>> General comments for functions.ksh.
>>>>>> - Some functions do not have debug_message Begin|End, as such
>>>>>> ${SET_DEBUG} may not be set.
>>>>> I have included the debug_message for all the functions.
>>>>>
>>>>> -Thanks and Regards
>>>>> Hemachandran
>>>>>> The rest looks good.
>>>>>>
>>>>>> Regards
>>>>>> Neil
>>>>>>
>>>>>> On 29 Jun 2009, at 05:53, Hemachandran Namachivayam wrote:
>>>>>>
>>>>>>> Hi Neil,
>>>>>>>
>>>>>>> On 06/28/09 23:50, Neil Garthwaite wrote:
>>>>>>>> Hi Hemachandran,
>>>>>>>> I note that things have been redesigned and reworked
>>>>>>>> accordingly, as such there is a reasonable amount to review.
>>>>>>>> Nevertheless, I plan to go through that later.
>>>>>>>> I do note that the CCR is being used to store, amend and
>>>>>>>> retrieve information, as such if I refer to the design document
>>>>>>>> http://opensolaris.org/os/project/ha-xvm/ha-ldoms-design-doc_v2.pdf,
>>>>>>>> I can see that /usr/cluster/lib/sc/ccrutil is referenced
>>>>>>>> however within the code you have /usr/cluster/lib/sc/ccradm, is
>>>>>>>> this a typo within the design document? Either way, as this is
>>>>>>>> an Open HA Cluster review, the ability to use the CCR to store,
>>>>>>>> amend and retrieve information is likely to be extremely useful
>>>>>>>> to others as simply put it provides a synchronized repository
>>>>>>>> across the cluster.
>>>>>>> Thanks for pointing this out. The table file operations to
>>>>>>> store, amend and retrieve information are now contained in
>>>>>>> "ccradm" utility, to fulfill the HA-xVM needs to share the
>>>>>>> information across cluster nodes. I have changed the references
>>>>>>> of ccrutil to "ccradm" in the design document. Please find the
>>>>>>> updated document at
>>>>>>> http://opensolaris.org/os/project/ha-xvm/ha-ldoms-design-doc_v2.pdf
>>>>>>>
>>>>>>>> As such I feel you need to expand on how the ccradm utility can
>>>>>>>> be used in the context of storing, amending and retrieving
>>>>>>>> information. I could only find
>>>>>>>> http://src.opensolaris.org/source/xref/ohac/ohac/usr/src/cmd/dtk/man/man1m/ccradm.1m
>>>>>>>>
>>>>>>>> which does not contain the arguments you are using.
>>>>>>> A revised edition of the man page is yet to be committed to the
>>>>>>> gate. For the time being, You can refer to the man page changes
>>>>>>> at http://opensolaris.org/os/project/ha-xvm/ccradm.pdf
>>>>>>>
>>>>>>> -Thanks and Regards
>>>>>>> Hemachandran
>>>>>>>> Regards
>>>>>>>> Neil
>>>>>>>>
>>>>>>>> On 26 Jun 2009, at 07:05, Hemachandran Namachivayam wrote:
>>>>>>>>
>>>>>>>>> Hi Folks,
>>>>>>>>>
>>>>>>>>> I'd welcome the code review for the "Solaris cluster failover
>>>>>>>>> agent for LDoms Guest domains"
>>>>>>>>>
>>>>>>>>> The failover agent for LDoms Guest domains is an extension of
>>>>>>>>> the HA-xVM agent and introduces a new resource type SUNW.ldom.
>>>>>>>>> The agent is redesigned to use the Generic Data Service
>>>>>>>>> (SUNW.gds) binaries and hence the resource type (SUNW.xvm)
>>>>>>>>> have been reworked upon. The HA-xVM agent makes use of CCR to
>>>>>>>>> store the xVM/LDom guest domain configuration; thereby a need
>>>>>>>>> to configure a separate admin directory is eliminated.
>>>>>>>>>
>>>>>>>>> To refer to the requirements specification and design
>>>>>>>>> document, please visit http://opensolaris.org/os/project/ha-xvm/
>>>>>>>>>
>>>>>>>>> The webrev, for review, is located at
>>>>>>>>> http://cr.opensolaris.org/~hnamachi/ha-ldom/
>>>>>>>>> <http://cr.opensolaris.org/%7Ehnamachi/ha-ldom/>
>>>>>>>>>
>>>>>>>>> -Thanks and Regards
>>>>>>>>> Hemachandran
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> ha-clusters-discuss mailing list
>>>>>>>>> ha-clusters-discuss at opensolaris.org
>>>>>>>>> <mailto:ha-clusters-discuss at opensolaris.org>
>>>>>>>>> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> ha-clusters-discuss mailing list
>>>>> ha-clusters-discuss at opensolaris.org
>>>>> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
>>>>>
>>>>
>>>
>>
>
--
*****************************************************************************
Detlef Ulherr
Staff Engineer Tel: (++49 6103) 752-248
Availability Engineering Fax: (++49 6103) 752-167
Sun Microsystems GmbH
Amperestr. 6 mailto:detlef.ulherr at sun.com
63225 Langen http://www.sun.de/
*****************************************************************************
Sitz der Gesellschaft:
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Amtsgericht M?nchen: HRB 161028
Gesch?ftsf?hrer: Thomas Schr?der, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin H?ring
*****************************************************************************
