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.
> 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?
> /**
> + * 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.
> // 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.
> 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
