Ganesh,
A few comments below.
Ganesh Ram N wrote:
> Hi Nick,
>
> Thanks a lot for the quick review. Please find my response inline ...
>
> Nicholas Solter wrote:
>> Ganesh,
>>
>> Here are a few comments. I should note that I'm not too familiar with
>> either the CLI or the weak membership code.
>>
>> General: You need to change copyrights to 2009 now.
>>
> I will do this change right at the end after I have incorporated all the
> comments.
>> usr/src/cmd/clcommands/commands/clnode/Makefile
>>
>> Because weak membership is not dependent on OpenSolaris, we decided to
>> control weak membership through a separate flag not inherently tied to
>> the version. Note that the core weak membership implementation defines
>> WEAK_MEMBERSHIP in usr/src/common/cl/sys/clconf_int.h
>>
>> This should be tied to the control of the CLI portions as well. So
>> instead of using POST_S10_BUILD in the Makefile here, I think you'll
>> need to define a new macro in some Makefile and use it to specify the
>> WEAK_MEMBERSHIP definition that clconf_int.h can pick up.
>>
>>
> As per Sambit's response on a different email, I have included
> <sys/clconf_int.h> which has the definition for the WEAK_MEMBERSHIP
> macro for all the C/C++ files. Similarly can you or anyone else point me
> to a Makefile macro which I can include and use ?
We discussed this offline.
>> usr/src/cmd/clcommands/commands/clnode/clnode_cmd.h
>>
>> 64 #define CLNODE_SUBCMD_RESOLVE_CONFIG \
>> 65 GETTEXT("Resolves the cluster weak membership during split
>> brain")
>>
>> The rest of the messages seem to be imperative rather than descriptive,
>> so this one should probably be, "Resolve the..." instead of "Resolves
>> the..."
>>
>> Also, I don't think the description is quite right. This doesn't resolve
>> "weak membership", it resolves the CCR configuration changes that
>> occurred during weak membership. And it's not run during split brain,
>> rather after the cluster is back together.
>>
>>
> Lisa commented on the same text and we modified it to "Resolve split
> brain in a weak-membership cluster".
> Should this be modified to "Resolve cluster configuration conflicts in a
> weak-membership cluster" ?
Yes, I prefer "cluster configuration conflicts".
The point I think I was trying to make about "split-brain" was that this
command is run after the split-brain has been resolved. So just changing
"split-brain" to "weak-membership" I don't think really helps. I'd
prefer something like, "Resolve cluster configuration conflicts that
occurred during a split-brain." Or something like that.
>> usr/src/cmd/clcommands/commands/clnode/clnode_resolve_config_options.cc
>>
>> Pointers such as cl_option, node_name, and so on should be assigned NULL
>> by default, not 0 casted to a pointer.
>>
>>
> done
>> usr/src/cmd/clcommands/commands/clnode/clnodeinit_constants.cc
>>
>> I think we may have discussed this, but I can't remember why you need to
>> specify the local node in the resolve-config command. Isn't it obvious
>> based on the node on which you're running the command?
>>
>>
> In New CLI, if there are no operands specified it equates to a WILD_CARD
> operand which will match all the existing objects of that type. Hence
> the requirement for the user to specify the node name as the operand
> explicitly.
>> usr/src/cmd/clcommands/include/ClShow.h
>> usr/src/cmd/clcommands/include/ClStatus.h
>> usr/src/cmd/clcommands/include/clcommands.h
>>
>> I don't like these redundant definitions of WEAK_MEMBERSHIP with each
>> other and with the definition in clconf_int.h. The macro should be
>> defined in one place only.
>>
>>
> Removed the macro from all of the above files and instead I have
> included <sys/clconf_int.h> which has the definition for WEAK_MEMBERSHIP
> macro.
>> usr/src/cmd/clcommands/lib/libclcmds/Makefile.com
>>
>> Same problem about using POST_S10_BUILD rather than tying in with the
>> WEAK_MEMBERSHIP macro.
>>
>>
> See comment above.
>> usr/src/cmd/clcommands/lib/libclcmds/common/clnode/clnode_resolve_config.cc
>>
>> Same point about NULL vs 0.
>>
>>
> Done.
>> Why do you use the WEAK_MEMBERSHIP macro for some portions of this file?
>> Isn't the whole file only for weak membership?
>>
>> usr/src/cmd/clcommands/lib/libclcmds/common/clquorum/clquorum_set.cc
>>
>> "This action might result in DATA LOSS !!!\"
>>
>> This message is a little extreme. I don't think we need even one, let
>> alone three, exclamation marks. Also, I thought we'd agreed to say "data
>> corruption or loss."
>>
>>
> :) Should this be "This action might result in data corruption or loss\n" ?
Yes, that's better.
Thanks,
Nick
>
>> 387 gettext("Are you sure you want to enable
>> multiple "
>> 388 "partitions in the cluster to be
>> operational. "
>> 389 "(y/[n])?"));
>>
>>
>> Shouldn't have a period before the "(y/[n])?"
>>
>>
> I have removed the period as per Lisa's comments earlier.
>> usr/src/head/scadmin/scconf.h
>>
>> Here again you define WEAK_MEMBERSHIP. This definition should be one
>> place only.
>>
>>
> Agreed.
>> usr/src/lib/libscconf/Makefile.com
>>
>> Same problem about using POST_S10_BUILD.
>>
> Agreed
>> usr/src/lib/libscconf/common/scconf_infrafile.c
>>
>>
>> 114 *
>> 115 * SOL_VERSION >= s11
>> 116 * scconf_cluster_multi_partitions
>> 117 * scconf_cluster_ping_targets
>> 118 * END SOL_VERSION >= s11
>>
>> I know this is just comments, but you should use WEAK_MEMBERSHIP, not
>> SOL_VERSION here.
>>
>>
> Yes. I will do so.
>
> Thanks,
> Ganesh Ram
>> Thanks,
>> Nick
>>
>> Ganesh Ram Nagarajan wrote:
>>
>>> Hi All,
>>>
>>> Please review our CLI changes for supporting Weak Membership Model
>>>
>>> Webrev Available @ http://cr.opensolaris.org/~gnr259/colorado-cli/webrev/
>>>
>>> Summary of changes :
>>> ===============
>>>
>>> New Quorum object "global_quorum"
>>> Used for specifying the "ping_targets" property [list of
>>> ping_targets can be added]
>>> Used for specifying the "multiple_partitions" property
>>> True - Specifies we are in weak membership
>>> False - Specifies we are in strong membership
>>>
>>> New subcommand for clnode command "resolve-config"
>>> Used for declaring the winner node after split-brain (Runs in both
>>> cluster & non-cluster mode)
>>> Used for declaring the loser node after split-brain (Run only in
>>> non-cluster mode)
>>> clnode status command used for querying for the presence of
>>> unresolved CCR changes
>>>
>>> Regards & Thanks,
>>> Ganesh Ram & Nandan
>>>
>>>
>>> _______________________________________________
>>> ha-clusters-discuss mailing list
>>> 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
>>
>
> _______________________________________________
> ha-clusters-discuss mailing list
> ha-clusters-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/ha-clusters-discuss
