Hi Nagu, Please find my replies with [Minh]
Thanks, Minh On 01/07/17 00:33, Nagendra Kumar wrote:
[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.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]: 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.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]: 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./** + * 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]: 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.// 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.
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
