Ack with minor comments:
* a node lock does not cause fail-over but switch-over

* the commit message is too long and it feels like it is missing the essence of 
this change. How about:

"amfd: defer switch-over of dependent SI to after sponsor SI [#803]

where I mean "defer" is the essence

* in two places of si_dep.cc the use of avd_sidep_si_dep_state_set is missing:

950:                            dep_si->si_dep_state = AVD_SI_READY_TO_ASSIGN;
961:                    dep_si->si_dep_state = AVD_SI_READY_TO_UNASSIGN;

this makes the trace wrong (skipping states) and the SI dep state machine is 
not visualized. Please change these 
locations to use the setter function.

* see inline

Also verified the solution.

Thanks,
Hans

P.S. For the future this code needs refactoring. "bool 
avd_sidep_is_si_failover_possible" seems like it is just checking 
something but it is actually changing state! It also has global scope which is 
not needed.


On 03/04/2014 02:04 PM, [email protected] wrote:
>   osaf/services/saf/amf/amfd/si_dep.cc |  3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
>
> Problem: When SI dependency is configured across the application, then during 
> node lock AMF is not performing
> failover dependent even after successful failover of sponsor. Instead if this 
> AMF deletes both active and
> standby assignmetns of the dependent SI.

- failover -> switch-over
- last sentence should be moved to "Reason"
- assignmetns->assignments
- blank line before "Reason"

> Reason: During node lock AMF sends quisced assignments to all the SUs hosted 
> on the node. In the present
- quisced->quiesced


> case successful quiesced response first for dependent. Since AMF does not see 
> any active sponsor for this dependent, it  deletes all the assignment of this 
> dependent SI ignoring the case that its sponsor it in the failover phase.

- blank line before "Fix"
- add hard line breaks at 80 chars

> Fix: Since sponsor is undergoing failover, AMF should mark the dep state of 
> dependent SI as FAILOVER_UNDER_PROGRESS. Once failover of sponsor completes, 
> dependent should be failovered to its respetive standby.

- failovered -> switch over
- respetive -> respective

>
> diff --git a/osaf/services/saf/amf/amfd/si_dep.cc 
> b/osaf/services/saf/amf/amfd/si_dep.cc
> --- a/osaf/services/saf/amf/amfd/si_dep.cc
> +++ b/osaf/services/saf/amf/amfd/si_dep.cc
> @@ -1765,7 +1765,8 @@ bool avd_sidep_is_si_failover_possible(A
>       TRACE_ENTER2("SI: '%s'",si->name.value);
>
>       /* Role failover triggered because of node going down */
> -     if(su->su_on_node->saAmfNodeOperState == SA_AMF_OPERATIONAL_DISABLED) {
> +     if ((su->su_on_node->saAmfNodeOperState == SA_AMF_OPERATIONAL_DISABLED) 
> ||
> +                     (su->su_on_node->saAmfNodeAdminState == 
> SA_AMF_ADMIN_LOCKED)) {
>               assignmemt_status = si_assignment_check_during_failover(si, su, 
> &active_sisu, &valid_stdby_sisu);
>               if (si->spons_si_list == NULL) {
>                       /* Check if the susi is in unassigned state
>
>

------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works. 
Faster operations. Version large binaries.  Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Reply via email to