I would also agree with these comments with one additional comment: The below check is not required in complete_siswap(): + + if (su->su_switch == AVSV_SI_TOGGLE_STABLE) + goto done; +
As we are not sure the removed code used to set to stable from unstable/stable, because there is no check in removed code, so we need to keep the code as it is without changing the logic. Thanks -Nagu > -----Original Message----- > From: praveen malviya > Sent: 02 April 2014 14:54 > To: Hans Feldt > Cc: Nagendra Kumar; [email protected] > Subject: Re: [devel] [PATCH 1 of 1] amfd: return AIS_OK instead of BAD_OP for > si-swap admin op [#823] > > Some comments: > 1) In the patch with the change: > su_ha_state = avd_su_state_determine(su); > - if (su_ha_state == SA_AMF_HA_QUIESCED) { > - if (su->su_switch == AVSV_SI_TOGGLE_SWITCH) { > - AVD_SU_SI_REL *temp_susi; > - for (temp_susi = su->list_of_susi; > temp_susi != NULL; temp_susi = temp_susi->su_next) { > - if (temp_susi->si->invocation != > 0) { > - avd_saImmOiAdminOperationResult(cb->immOiHandle, > - temp_susi->si->invocation, SA_AIS_ERR_BAD_OPERATION); > - temp_susi->si->invocation = 0; > - } > - } > - } > - m_AVD_SET_SU_SWITCH(cb, su, AVSV_SI_TOGGLE_STABLE); > - } else if (su_ha_state == SA_AMF_HA_QUIESCING) { > + if (su_ha_state == SA_AMF_HA_QUIESCING) { > > The else part is executed for quiesced assignments, but it is coded for only > two > cases mentioned already there. > /* This may happen > 1. when Act SU is locked and it is transitioning > into Quisced and > the same > time stdby SU faulted. > 2. Durin SI-SWAP when Act SU goes to Quiesced, > Std SU goes to > Act and Quiesced SU > goes to Std and if it fails, then we seed to > delete the assignments. > */ But the case of #823 is different. In #823 quiesced assignment has faulted > which is not the above two cases. > So the removed "if " statement with " m_AVD_SET_SU_SWITCH(cb, su, > AVSV_SI_TOGGLE_STABLE);" should be there. > > 2)In the change > @@ -2382,9 +2390,6 @@ static uint32_t avd_sg_2n_susi_sucss_su_ > if (flag == true) { > node_admin_state_set(su_node_ptr, > SA_AMF_ADMIN_LOCKED); > } > - } else if (su->su_switch == AVSV_SI_TOGGLE_SWITCH) { > - /* this SU switch state is true change > to false. */ > - m_AVD_SET_SU_SWITCH(cb, su, > AVSV_SI_TOGGLE_STABLE); > } > > The code is removed, but there is no replacement with complete_siswap(). > Not sure, which use case it will violate. So let us keep this. > > > Thanks > Praveen > On 02-Apr-14 2:20 PM, Hans Feldt wrote: > > Please check and comment the attached patch. > > Thanks, > > Hans > > > >> -----Original Message----- > >> From: Nagendra Kumar [mailto:[email protected]] > >> Sent: den 2 april 2014 09:01 > >> To: Hans Feldt; Praveen Malviya > >> Cc: [email protected] > >> Subject: RE: [devel] [PATCH 1 of 1] amfd: return AIS_OK instead of > >> BAD_OP for si-swap admin op [#823] > >> > >> If the recovery of failed Su doesn't happen i.e. goes into > >> instantiatin failed state/termination failed state, then are you going to > >> rerun > failed operation ? > >> > >> Thanks > >> -Nagu > >>> -----Original Message----- > >>> From: Hans Feldt [mailto:[email protected]] > >>> Sent: 02 April 2014 12:14 > >>> To: praveen malviya > >>> Cc: [email protected] > >>> Subject: Re: [devel] [PATCH 1 of 1] amfd: return AIS_OK instead of > >>> BAD_OP for si-swap admin op [#823] > >>> > >>> Well my view point still stands. By separating the su_switch from > >>> invocation_id we introduce even more sub-states and we already have > enough of those! > >>> Basically the same patch as you have with a small addition causes > >>> the admin response to be sent after recovery of the failed SU. Will send a > patch. > >>> /Hans > >>> > >>> On 04/02/2014 08:28 AM, praveen malviya wrote: > >>>> Any update/comment on this. > >>>> > >>>> Thanks, > >>>> Praveen > >>>> On 01-Apr-14 2:27 PM, praveen malviya wrote: > >>>>> On 01-Apr-14 1:27 PM, Hans Feldt wrote: > >>>>>> On 03/28/2014 02:14 PM, [email protected] wrote: > >>>>>>> osaf/services/saf/amf/amfd/sg_2n_fsm.cc | 23 > >>>>>>> +++++++++++++--------- > >>> - > >>>>>>> 1 files changed, 13 insertions(+), 10 deletions(-) > >>>>>>> > >>>>>>> > >>>>>>> Problem: During si-swap if quiesced assignment faults, AMF > >>>>>>> returns BAD_OPERATION even though si-swap completes successfuly > >>>>>>> after > >>> recovery. > >>>>>>> Reason: During si-swap, AMF sends quiesced assignment to active SU. > >>>>>>> During quiesced assignments, one of the components faults. > >>>>>>> AMF performs cleanup of this failed component and perfroms the > >>>>>>> failover of assignments to standby SU. For the faulted SU > >>>>>>> assignments are deleted and when it gets sucessfully repaired, > >>>>>>> AMF assigns it with standby assignments. > >>>>>>> Thus si-swap operation eventually gets successful, but AMF > >>>>>>> returns BAD_OPERATION for the operation. > >>>>>>> > >>>>>>> Fix: Since AMF performs si-swap successfuly despite fault in > >>>>>>> quiesced state, this patch ensures that AMF returns SA_AIS_OK > >>>>>>> for the operation. > >>>>>>> > >>>>>>> diff --git a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc > >>>>>>> b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc > >>>>>>> --- a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc > >>>>>>> +++ b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc > >>>>>>> @@ -888,16 +888,6 @@ static uint32_t avd_sg_2n_su_fault_su_op > >>>>>>> if (su->sg_of_su->su_oper_list.su == su) { > >>>>>>> su_ha_state = avd_su_state_determine(su); > >>>>>>> if (su_ha_state == SA_AMF_HA_QUIESCED) { > >>>>>>> - if (su->su_switch == AVSV_SI_TOGGLE_SWITCH) { > >>>>>>> - AVD_SU_SI_REL *temp_susi; > >>>>>>> - for (temp_susi = su->list_of_susi; temp_susi != > >>>>>>> NULL; temp_susi = temp_susi->su_next) { > >>>>>>> - if (temp_susi->si->invocation != 0) { > >>>>>>> - avd_saImmOiAdminOperationResult(cb->immOiHandle, > >>>>>>> - temp_susi->si->invocation, SA_AIS_ERR_BAD_OPERATION); > >>>>>>> - temp_susi->si->invocation = 0; > >>>>>>> - } > >>>>>>> - } > >>>>>>> - } > >>>>>>> m_AVD_SET_SU_SWITCH(cb, su, > >>>>>>> AVSV_SI_TOGGLE_STABLE); > >>>>>> Why isn't the above line removed also? > >>>>>> > >>>>>> This state should be kept until the admin op has been responded, > >>>>>> then we forget all about it. > >>>>>> > >>>>> In the AMF code nowhere we are checking su->su_switch and > >>>>> si->invocation simultaneously which means they are kept very much > >>>>> independent. So responding for the admin operation is not very > >>>>> much dependent on the toggling of su_switch. So we can keep the > >>>>> above line. > >>>>>>> } else if (su_ha_state == SA_AMF_HA_QUIESCING) { > >>>>>>> if > >>>>>>> (avd_sidep_si_dependency_exists_within_su(su)) { @@ -2095,6 > +2085,19 @@ static uint32_t avd_sg_2n_susi_sucss_su_ > >>>>>>> } > >>>>>>> > >>>>>>> m_AVD_SET_SG_FSM(cb, (su->sg_of_su), > >>>>>>> AVD_SG_FSM_SG_REALIGN); > >>>>>>> + > >>>>>>> + if (su->su_switch == AVSV_SI_TOGGLE_STABLE) { > >>>>>> if the above line is removed, this code won't get triggered. The > >>>>>> same needs to be placed somewhere else. > >>>>>> > >>>>> I think if "if statement" is removed, the code inside it will > >>>>> always be executed which we do not want, just to not break existing > functionality. > >>>>> I did not get what do you mean by removing and placing it somewhere > else. > >>>>> > >>>>> Thanks > >>>>> Praveen > >>>>> > >>>>>>> + for (AVD_SU_SI_REL *temp_susi = > >>>>>>> su->list_of_susi; > >>>>>>> + temp_susi != NULL; > >>>>>>> + temp_susi = temp_susi->su_next) { > >>>>>>> + if (temp_susi->si->invocation != 0) { > >>>>>>> + avd_saImmOiAdminOperationResult(cb->immOiHandle, > >>>>>>> + temp_susi->si->invocation, SA_AIS_OK); > >>>>>>> + temp_susi->si->invocation = 0; > >>>>>>> + } > >>>>>>> + } > >>>>>>> + } > >>>>>>> + > >>>>>>> } > >>>>>>> } else if ((act == AVSV_SUSI_ACT_MOD) && (state == > >>>>>>> SA_AMF_HA_STANDBY) && > >>>>>>> (su->sg_of_su->su_oper_list.su == su)) { > >>>>>>> > >>>>>>> > >>>>> ------------------------------------------------------------------ > >>>>> --- > >>>>> --------- _______________________________________________ > >>>>> Opensaf-devel mailing list > >>>>> [email protected] > >>>>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel > >>>> > >>>> > >>> -------------------------------------------------------------------- > >>> ---------- _______________________________________________ > >>> Opensaf-devel mailing list > >>> [email protected] > >>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel > ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
