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?
2. So, after this patch what is the outcome of scenario listed in the ticket.
Is it able to do a failover ?
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
