Hi Minh,
Ack.
Thanks
-Nagu
> -----Original Message-----
> From: minh chau [mailto:[email protected]]
> Sent: 05 July 2017 06:24
> To: Nagendra Kumar; [email protected];
> [email protected]; Praveen Malviya
> Cc: [email protected]
> Subject: Re: [PATCH 1/1] amfd: Avoid to create duplicated 2N ACTIVE SUSI
> for absent assignment V2 [#2477]
>
> Hi Nagu,
>
> Plesae find my comments with [Minh]
>
> Thanks,
> Minh
> On 04/07/17 22:53, Nagendra Kumar wrote:
> > Hi Minh,
> > Thanks for clarification.
> > 1. But, I didn't get the logic of (tail == tail). What will happen if the
> > message
> is last i.e. tail, then will it go into loop?
> [Minh]: The if condition is (tail == cb->dnd_list.tail), the @tail is to
> memorize the last item in queue before any change.
> When we pop a susi message for example, change msg_id, push it back, now
> the dnd_list.tail is changed. So the if (tail == cb->dnd_list.tail) is
> to know if we reach all items of the queue
> > 2. So, after this patch what is the outcome of scenario listed in the
> > ticket. Is
> it able to do a failover ?
> We now don't create an absent active susi so we will not do any
> absent_failover(). With only one active susi, sg fsm will continue to
> create another standby susi. The outcome is:
> safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwonDep1,safApp=AmfDemoTwon
> saAmfSISUHAState=ACTIVE(1)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> \--
> safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwonDep2,safApp=AmfDemoTwon
> saAmfSISUHAState=ACTIVE(1)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwon,safApp=AmfDemoTwon
> saAmfSISUHAState=ACTIVE(1)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> --
> safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwon,safApp=AmfDemoTwon
> saAmfSISUHAState=STANDBY(2)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> --
> safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwonDep2,safApp=AmfDemoTwon
> saAmfSISUHAState=STANDBY(2)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
> safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=A
> mfDemoTwonDep1,safApp=AmfDemoTwon
> saAmfSISUHAState=STANDBY(2)
> saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
>
> SU4 has active assignment which was assigned from failover during both
> SC were going down. SU1 has newly standby assignments
>
>
> >
> > Thanks
> > -Nagu
> >
> >> -----Original Message-----
> >> From: minh chau [mailto:[email protected]]
> >> Sent: 02 July 2017 03:28
> >> To: Nagendra Kumar; [email protected];
> >> [email protected]; Praveen Malviya
> >> Cc: [email protected]
> >> Subject: Re: [PATCH 1/1] amfd: Avoid to create duplicated 2N ACTIVE
> SUSI
> >> for absent assignment V2 [#2477]
> >>
> >> Hi Nagu,
> >>
> >> Please find my replies with [Minh]
> >>
> >> Thanks,
> >> Minh
> >>
> >> On 01/07/17 00:33, Nagendra Kumar wrote:
> >>> Hi Minh,
> >>> Please find comment inlined with [Nagu].
> >>>
> >>> Thanks
> >>> -Nagu
> >>>
> >>>> -----Original Message-----
> >>>> From: Minh Chau [mailto:[email protected]]
> >>>> Sent: 20 June 2017 07:11
> >>>> To:[email protected];[email protected];
> >>>> [email protected];[email protected]
> >>>> Cc:[email protected]; Minh Chau
> >>>> Subject: [PATCH 1/1] amfd: Avoid to create duplicated 2N ACTIVE SUSI
> for
> >>>> absent assignment V2 [#2477]
> >>>>
> >>>> In large cluster, the IMM sync calls mostly take no effect when both
> SCs
> >>>> abruptly go down, thus amfd may leaves the susi assignments in amfnd
> >> and
> >>>> IMM in unexpected states.
> >>>>
> >>>> The scenario is same as #2416, but in #2477 amfd can also see 2
> ACTIVE
> >>>> assignments of 2 SUs for the same 2N SI. That leads to osafassert(),
> >> causes
> >>>> node reboot, and the same osafassert() repeatedly happens after node
> >>>> comes up.
> >>>>
> >>>> Patch refactors the creation of absent assignment by adding new
> function
> >>>> avd_susi_validate_absent_assignment(), and adds a check to avoid
> >> creating 2
> >>>> ACTIVE assignments for one SI. Also, a fix
> >>>> avnd_diq_rec_check_buffered_msg() is needed in this scenario
> >>>> ---
> >>>> src/amf/amfd/siass.cc | 127 ++++++++++++++++++++++++++++++++--
> ---
> >> -----
> >>>> --------
> >>>> src/amf/amfd/susi.h | 4 +-
> >>>> src/amf/amfnd/di.cc | 6 ++-
> >>>> 3 files changed, 89 insertions(+), 48 deletions(-)
> >>>>
> >>>> diff --git a/src/amf/amfd/siass.cc b/src/amf/amfd/siass.cc index
> >>>> 6a13836f9..d14d279dc 100644
> >>>> --- a/src/amf/amfd/siass.cc
> >>>> +++ b/src/amf/amfd/siass.cc
> >>>> @@ -218,13 +218,20 @@ void
> >>>> avd_susi_read_headless_cached_rta(AVD_CL_CB *cb) {
> >>>> std::string(strstr(osaf_extended_name_borrow(&dn), "safSi"));
> >>>> assert(si_name.empty() == false);
> >>>> AVD_SI *si = si_db->find(si_name);
> >>>> - osafassert(si);
> >>>> + if (si == nullptr) {
> >>>> + LOG_ER("SI:'%s' does not exist in AMF's sidb", si_name.c_str());
> >>>> + continue;
> >>>> + }
> >>> [Nagu]: If we have SUSI in Imm DB, then SI should exist. Did you get any
> >> scenario which hits this.
> >>>> SaNameT su_name;
> >>>> avsv_sanamet_init_from_association_dn(&dn, &su_name, "safSu",
> >>>> si->name.c_str());
> >>>> AVD_SU *su = su_db->find(Amf::to_string(&su_name));
> >>>> + if (su == nullptr) {
> >>>> + LOG_ER("SU:'%s' does not exist in AMF's sudb",
> >>>> + Amf::to_string(&su_name).c_str());
> >>>> + continue;
> >>>> + }
> >>>> osaf_extended_name_free(&su_name);
> >>>> - osafassert(su);
> >>> [Nagu]: If we have SUSI in Imm DB, then SU should exist. Did you get any
> >> scenario which hits this.
> >> [Minh]: We have failed into this assert in cluster with many "real"
> >> applications. It is basically not AMF problem, it appeared to be a link
> >> problem. But this osafassert() was causing the cyclic reboot and it's
> >> hard to collect the log. It didn't help to recover, and replace it by
> >> LOG_ER might leave SG unstable but it's way better of cyclic reboot. I
> >> think we need to look at this kind of asserts elsewhere and need to
> >> enable/disable it in debug/production build.
> >>>> susi = avd_su_susi_find(cb, su, si->name);
> >>>> rc = immutil_getAttr("osafAmfSISUFsmState", attributes, 0,
> >>>> &imm_susi_fsm);
> >>>> osafassert(rc == SA_AIS_OK);
> >>>> @@ -232,10 +239,11 @@ void
> >>>> avd_susi_read_headless_cached_rta(AVD_CL_CB *cb) {
> >>>> osafassert(rc == SA_AIS_OK);
> >>>>
> >>>> if (susi) { // FOR PRESENT SUSI found in AMFND(s)
> >>>> - TRACE("SISU:'%s', old(imm) fsm state: %d, new(sync) fsm state:
> %d",
> >>>> - Amf::to_string(&dn).c_str(), imm_susi_fsm, susi->fsm);
> >>>> + TRACE("Check present SUSI:'%s', old(imm) fsm state: %d,"
> >>>> + "new(sync) fsm state: %d", Amf::to_string(&dn).c_str(),
> >>>> + imm_susi_fsm, susi->fsm);
> >>>>
> >>>> - if (avd_susi_validate_headless_cached_rta(susi, imm_ha_state,
> >>>> + if (avd_susi_validate_present_assignment(susi, imm_ha_state,
> >>>> imm_susi_fsm) ==
> >>>> false) {
> >>>> continue;
> >>>> }
> >>>> @@ -279,38 +287,29 @@ void
> >>>> avd_susi_read_headless_cached_rta(AVD_CL_CB *cb) {
> >>>> if (susi->su->su_on_node->admin_ng == nullptr)
> >>>> avd_ng_restore_headless_states(cb, susi);
> >>>> } else { // For ABSENT SUSI
> >>>> - if (su->sg_of_su->sg_ncs_spec == false &&
> >>>> - su->sg_of_su->sg_redundancy_model !=
> >>>> SA_AMF_NO_REDUNDANCY_MODEL &&
> >>>> - su->sg_of_su->sg_redundancy_model !=
> >>>> - SA_AMF_N_WAY_ACTIVE_REDUNDANCY_MODEL &&
> >>>> - (su->sg_of_su->any_assignment_in_progress() == true ||
> >>>> - su->sg_of_su->any_assignment_assigned() == true)) {
> >>>> - TRACE("Absent SUSI, ha_state:'%u', fsm_state:'%u'",
> imm_ha_state,
> >>>> - imm_susi_fsm);
> >>>> - if (imm_susi_fsm != AVD_SU_SI_STATE_UNASGN) {
> >>>> - absent_susi = avd_susi_create(avd_cb, si, su, imm_ha_state,
> false,
> >>>> - AVSV_SUSI_ACT_BASE);
> >>>> - // Restore the fsm of this absent SUSI, which is used to
> determine
> >>>> - // whether a SU should be added in SG's SUOperationList
> >>>> - // Memorize it in temporary var @absent
> >>>> - // The fsm of this SUSI will be changed to
> >> AVD_SU_SI_STATE_ABSENT
> >>>> - // after restoring SUOperationList
> >>>> - absent_susi->fsm = imm_susi_fsm;
> >>>> - absent_susi->absent = true;
> >>>> - if (absent_susi->si->saAmfSIAdminState ==
> >> SA_AMF_ADMIN_LOCKED
> >>>> ||
> >>>> - absent_susi->si->saAmfSIAdminState ==
> >>>> - SA_AMF_ADMIN_SHUTTING_DOWN) {
> >>>> - if (absent_susi->fsm == AVD_SU_SI_STATE_MODIFY &&
> >>>> - (absent_susi->state == SA_AMF_HA_QUIESCED ||
> >>>> - absent_susi->state == SA_AMF_HA_QUIESCING)) {
> >>>> - m_AVD_SET_SG_ADMIN_SI(cb, si);
> >>>> - }
> >>>> - }
> >>>> - } else {
> >>>> - avd_saImmOiRtObjectDelete(Amf::to_string(&dn));
> >>>> - }
> >>>> - } else {
> >>>> + TRACE("Check absent SUSI, ha_state:'%u', fsm_state:'%u'",
> >>>> imm_ha_state,
> >>>> + imm_susi_fsm);
> >>>> + if (avd_susi_validate_absent_assignment(su, si,
> >>>> + imm_ha_state, imm_susi_fsm) == false) {
> >>>> avd_saImmOiRtObjectDelete(Amf::to_string(&dn));
> >>>> + continue;
> >>>> + }
> >>>> + absent_susi = avd_susi_create(avd_cb, si, su, imm_ha_state, false,
> >>>> + AVSV_SUSI_ACT_BASE);
> >>>> + // Restore the fsm of this absent SUSI, which is used to determine
> >>>> + // whether a SU should be added in SG's SUOperationList
> >>>> + // Memorize it in temporary var @absent
> >>>> + // The fsm of this SUSI will be changed to
> AVD_SU_SI_STATE_ABSENT
> >>>> + // after restoring SUOperationList
> >>>> + absent_susi->fsm = imm_susi_fsm;
> >>>> + absent_susi->absent = true;
> >>>> + if (absent_susi->si->saAmfSIAdminState ==
> SA_AMF_ADMIN_LOCKED
> >> ||
> >>>> + absent_susi->si->saAmfSIAdminState ==
> >>>> SA_AMF_ADMIN_SHUTTING_DOWN) {
> >>>> + if (absent_susi->fsm == AVD_SU_SI_STATE_MODIFY &&
> >>>> + (absent_susi->state == SA_AMF_HA_QUIESCED ||
> >>>> + absent_susi->state == SA_AMF_HA_QUIESCING)) {
> >>>> + m_AVD_SET_SG_ADMIN_SI(cb, si);
> >>>> + }
> >>>> }
> >>>> }
> >>>> }
> >>>> @@ -321,13 +320,58 @@ done:
> >>>> TRACE_LEAVE();
> >>>> }
> >>> [Nagu]: I find below function and above changes are just refactoring. It
> is
> >> not making any logic change, please confirm?
> >> [Minh]: The main change is at check of 2N SG where the old code doesn't.
> >> It avoids to create an absent ACTIVE SUSI read from IMM while AMFD is
> >> having another present ACTIVE SUSI for the same SI.
> >>>> /**
> >>>> + * Validate the creation of absent assignment
> >>>> + * @param su: The SU associated with absent assignment
> >>>> + * @param si: The SI associated with absent assignment
> >>>> + * @return: true of valid, false otherwise */ bool
> >>>> +avd_susi_validate_absent_assignment(AVD_SU *su, AVD_SI *si,
> >>>> + SaAmfHAStateT imm_ha_state, AVD_SU_SI_STATE imm_fsm_state)
> {
> >>>> + bool valid = false;
> >>>> + TRACE_ENTER();
> >>>> + // Must be application sg
> >>>> + if (su->sg_of_su->sg_ncs_spec == true) goto done;
> >>>> + // skip for removing SUSI assignment
> >>>> + if (imm_fsm_state == AVD_SU_SI_STATE_UNASGN) goto done;
> >>>> + // Must have present SUSI assignment
> >>>> + if (su->sg_of_su->any_assignment_in_progress() == false &&
> >>>> + su->sg_of_su->any_assignment_assigned() == false) {
> >>>> + goto done;
> >>>> + }
> >>>> + // Support: 2N, NoRed, NwayActive. Not support: NpM, Nway
> >>>> + if (su->sg_of_su->sg_redundancy_model ==
> >>>> SA_AMF_NPM_REDUNDANCY_MODEL ||
> >>>> + su->sg_of_su->sg_redundancy_model ==
> >>>> SA_AMF_N_WAY_REDUNDANCY_MODEL) {
> >>>> + LOG_WA("Unsupported SG Redundancy model(%d) for SC Absence",
> >>>> + su->sg_of_su->sg_redundancy_model);
> >>>> + }
> >>>> + // Skip for NoRed and NWayActive
> >>>> + if (su->sg_of_su->sg_redundancy_model ==
> >>>> SA_AMF_NO_REDUNDANCY_MODEL ||
> >>>> + su->sg_of_su->sg_redundancy_model ==
> >>>> + SA_AMF_N_WAY_ACTIVE_REDUNDANCY_MODEL) {
> >>>> + goto done;
> >>>> + }
> >>>> + // No need to create absent SUSI assignment for the 2N SI that
> >>>> +already has
> >>>> + // ACTIVE SUSI
> >>>> + if (su->sg_of_su->sg_redundancy_model ==
> >>>> SA_AMF_2N_REDUNDANCY_MODEL) {
> >>>> + if (si->list_of_sisu != nullptr &&
> >>>> + si->list_of_sisu->state == SA_AMF_HA_ACTIVE &&
> >>>> + imm_ha_state == SA_AMF_HA_ACTIVE)
> >>>> + goto done;
> >>>> + }
> >>>> +
> >>>> + valid = true;
> >>>> +done:
> >>>> + TRACE_LEAVE();
> >>>> + return valid;
> >>>> +}
> >>>> +/**
> >>>> * Validate cached RTA read from IMM
> >>>> * @param present_susi
> >>>> * @param ha_fr_imm: Ha state of @present_susi read from IMM
> >>>> * @param fsm_fr_imm: Fsm state of @present susi read from IMM
> >>>> * @return: true of valid, false otherwise
> >>>> */
> >>>> -bool avd_susi_validate_headless_cached_rta(AVD_SU_SI_REL
> >>>> *present_susi,
> >>>> +bool avd_susi_validate_present_assignment(AVD_SU_SI_REL
> >> *present_susi,
> >>>> SaAmfHAStateT ha_fr_imm,
> >>>> AVD_SU_SI_STATE
> >>>> fsm_fr_imm) {
> >>>> std::string dn = present_susi->si->name + "," + present_susi->su-
> >name;
> >>>> @@ -335,15 +379,8 @@ bool
> >>>> avd_susi_validate_headless_cached_rta(AVD_SU_SI_REL
> *present_susi,
> >>>> bool valid = true;
> >>>> // rule 1: valid ha state
> >>>> if (ha_fr_imm != present_susi->state) {
> >>>> - if (ha_fr_imm == SA_AMF_HA_QUIESCING || ha_fr_imm ==
> >>>> SA_AMF_HA_QUIESCED) {
> >>>> - // That's fine
> >>>> - ;
> >>>> - } else {
> >>>> - LOG_ER("SISU:'%s', old(imm) ha state: %d, new(sync) ha state: %d",
> >>>> + LOG_WA("SISU:'%s', old(imm) ha state: %d, new(sync) ha state:
> >>>> + %d",
> >>>> dn.c_str(), ha_fr_imm, present_susi->state);
> >>>> - valid = false;
> >>>> - goto done;
> >>>> - }
> >>>> }
> >>> [Nagu]: This change will allow the present susi to go ahead in function
> and
> >> will not mark valid as false.
> >>> This means that avd_susi_validate_present_assignment
> >>> will
> return
> >> true. So, how this change is helping
> >>> In solving the problem, please explain.
> >> [Minh]: This change needs to return true to update the assignment
> >> counter of active present susi, since we will not let the 2nd active
> >> absent susi to be created (as in previous comment). In V1 patch, we
> >> allowed to create the 2nd absent susi, then failover it by node_fail()
> >> and from there the assignment counter will be updated.
> >>>> // rule 2: if ha_fr_imm is QUIESCING, one of relevant entities must
> >>>> // have adminState is SHUTTINGDOWN, otherwise re-adjust if
> possible
> >> diff
> >>>> --git a/src/amf/amfd/susi.h b/src/amf/amfd/susi.h index
> >>>> 94bd04033..5d925baae 100644
> >>>> --- a/src/amf/amfd/susi.h
> >>>> +++ b/src/amf/amfd/susi.h
> >>>> @@ -143,9 +143,11 @@ AVD_SU_SI_REL *avd_susi_create(
> >>>> AVD_SU_SI_REL *avd_susi_find(AVD_CL_CB *cb, const std::string
> >>>> &su_name,
> >>>> const std::string &si_name); void
> >>>> avd_susi_update_fsm(AVD_SU_SI_REL *susi, AVD_SU_SI_STATE
> >>>> new_fsm_state); -bool
> >>>> avd_susi_validate_headless_cached_rta(AVD_SU_SI_REL
> *present_susi,
> >>>> +bool avd_susi_validate_present_assignment(AVD_SU_SI_REL
> >> *present_susi,
> >>>> SaAmfHAStateT ha_fr_imm,
> >>>> AVD_SU_SI_STATE
> >>>> fsm_fr_imm);
> >>>> +bool avd_susi_validate_absent_assignment(AVD_SU *su, AVD_SI *si,
> >>>> + SaAmfHAStateT imm_ha_state, AVD_SU_SI_STATE imm_fsm_state);
> >>>> void avd_susi_read_headless_cached_rta(AVD_CL_CB *cb); extern
> void
> >>>> avd_susi_update(AVD_SU_SI_REL *susi, SaAmfHAStateT ha_state);
> >>>>
> >>> [Nagu]: Below logic change has no comment, not sure how this is
> related
> >> to the issue.
> >> [Minh]: The below change came from a test of double cyclic reboot of SC
> >> in the scenario of this issue.
> >> - after SC asbence stage, SC1 restarts, all msg_id of buffered msgs are
> >> corrected, plus amfnd has one node_up msg and it's waiting for ack
> >> - Now SC1 reboot, when we get mdsncs_down, we don't clean all
> buffered
> >> anymore, thus the previous node_up msg is still in queue, this node_up
> >> msg (or maybe some other msgs was around 1st reboot) should be
> removed.
> >>
> >> The "break" (before this patch) has made the loop quit at just the first
> >> time that amfnd found the SUSI_ASSIGN/OPER_STATE being *already*
> >> corrected, then amfnd hasn't gone through all the buffered list. amfnd
> >> will go through all the list for this first time of uncorrected buffered
> >> msg, but it doesn't go for the next time. So this patch is for this
> >> scenario.
> >>>> diff --git a/src/amf/amfnd/di.cc b/src/amf/amfnd/di.cc index
> >>>> 6f0a76cda..0da907566 100644
> >>>> --- a/src/amf/amfnd/di.cc
> >>>> +++ b/src/amf/amfnd/di.cc
> >>>> @@ -1290,6 +1290,7 @@ void avnd_di_msg_ack_process(AVND_CB
> *cb,
> >>>> uint32_t mid) { void avnd_diq_rec_check_buffered_msg(AVND_CB *cb)
> {
> >>>> if ((cb->dnd_list.head != nullptr)) {
> >>>> AVND_DND_MSG_LIST *rec = 0;
> >>>> + AVND_DND_MSG_LIST *tail = cb->dnd_list.tail;
> >>>> bool found = true;
> >>>> while (found) {
> >>>> found = false;
> >>>> @@ -1318,7 +1319,7 @@ void
> >>>> avnd_diq_rec_check_buffered_msg(AVND_CB *cb) {
> >>>> rec->msg.info.avd->msg_info.n2d_su_si_assign.msg_id);
> >>>> }
> >>>> m_AVND_DIQ_REC_PUSH(cb, rec);
> >>>> - break;
> >>>> + if (tail == cb->dnd_list.tail) break;
> >>>> } else if (rec->msg.info.avd->msg_type ==
> >>>> AVSV_N2D_OPERATION_STATE_MSG) {
> >>>> if (rec->msg.info.avd->msg_info.n2d_opr_state.msg_id != 0) {
> @@
> >> -
> >>>> 1337,11 +1338,12 @@ void
> >> avnd_diq_rec_check_buffered_msg(AVND_CB
> >>>> *cb) {
> >>>> .raw);
> >>>> }
> >>>> m_AVND_DIQ_REC_PUSH(cb, rec);
> >>>> - break;
> >>>> + if (tail == cb->dnd_list.tail) break;
> >>>> } else {
> >>>> // delete other messages for now
> >>>> avnd_diq_rec_del(cb, rec);
> >>>> rec = cb->dnd_list.head;
> >>>> + tail = cb->dnd_list.tail;
> >>>> }
> >>>> }
> >>>> }
> >>>> --
> >>>> 2.11.0
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
