Hi Minh,
I updated the ticket with the following details, we need to
check with more granular levels, how to handle such cases (may be in an
enhancement ticket):
=====================
Also, to note, it is documented as limitations in Amf PR Doc as below, so this
ticket qualifies as Enhancement (could have been #2416 as well):
2.2.11.3 Limitations
. Possible loss of RTA updates and SI assignment messages
If both SCs go down abruptly (SCs are immediately powered-off for instance),
AMFD could fail to update RTA to IMM, the SI assignment messages sent from
AMFND could not reach to AMFD, or vice versa. In such cases, recovery could be
impossible, applications may have inappropriate assignment states.
========================
What do you think?
Thanks
-Nagu
> -----Original Message-----
> From: minh chau [mailto:[email protected]]
> Sent: 06 June 2017 16:35
> To: Nagendra Kumar; [email protected]; Praveen Malviya;
> [email protected]
> Cc: [email protected]
> Subject: Re: [PATCH 1/1] amf: Replace osafassert by log_warning to avoid
> cyclic reboot [#2477]
>
> Hi Nagu,
>
> At the beginning, I thought the osafassert(s) were there to prevent a
> weird thing continues to happen unpredictably. When those osafassert(s)
> are hit, the active controller reboots and the standby will be ok when
> the standby takes over active role. But in fact, the new active also
> gets into these osafassert(s) and cluster is facing cyclic reboot of
> controllers.
> I think the osafassert(s) would help if the SU(s) are deployed on
> controllers, but if SU(s) are deployed in payloads, the cyclic reboot
> should be happening.
>
> In addition to replacing the osafassert(s), the @stby_susi now ensures
> it must always point to a STANDBY susi, callers of avd_sg_2n_act_susi()
> are mostly in 2n SG Fsm code that already take care of @stby_susi as
> null and not-null. By doing this, the problem in this ticket has gone,
> and it should work with existing cases as @stby_susi should truly point
> to a STANDBY one.
>
> If you have another approach, please let me know, I can try it out.
>
> Thanks,
> Minh
>
> On 06/06/17 19:53, Nagendra Kumar wrote:
> > Hi Minh,
> > I think, we need to avoid this situation by doing something
> else. These logics are kept for putting checks and balances for not happening
> such situations, in my opinion, we shouldn't alter these logics.
> >
> > Thanks
> > -Nagu
> >
> >> -----Original Message-----
> >> From: Minh Chau [mailto:[email protected]]
> >> Sent: 02 June 2017 14:54
> >> To: [email protected]; [email protected];
> >> [email protected]; [email protected]
> >> Cc: [email protected]; Minh Chau
> >> Subject: [PATCH 1/1] amf: Replace osafassert by log_warning to avoid
> cyclic
> >> reboot [#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 a very unexpected states.
> >>
> >> The scenario is same as #2416, but in #2477 amfd can also see 2 ACTIVE
> >> assignments for both SUs of a 2N SG. That leads to osafassert(), causes
> >> node reboot, and the same osafassert() repeatedly happens after node
> >> comes up.
> >>
> >> Patch replaces the osafassert() with LOG_WA, reinforces the checking of
> >> @stby_susi in avd_sg_2n_act_susi(). Also, a fix
> >> avnd_diq_rec_check_buffered_msg()
> >> is needed in this scenario
> >> ---
> >> src/amf/amfd/sg_2n_fsm.cc | 29 +++++++++++++++++++++++------
> >> src/amf/amfd/siass.cc | 20 ++++++++++----------
> >> src/amf/amfnd/di.cc | 6 ++++--
> >> 3 files changed, 37 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/src/amf/amfd/sg_2n_fsm.cc b/src/amf/amfd/sg_2n_fsm.cc
> >> index 3a7609e07..b9748015e 100644
> >> --- a/src/amf/amfd/sg_2n_fsm.cc
> >> +++ b/src/amf/amfd/sg_2n_fsm.cc
> >> @@ -580,7 +580,8 @@ static AVD_SU_SI_REL
> >> *avd_sg_2n_act_susi(AVD_CL_CB *cb, AVD_SG *sg,
> >> /* Determining SUSI for su_2 may not be needed, but to make sure we
> >> have
> >> * correct SUSI.*/
> >> a_susi_2 = su_assigned_susi_find(su_2, &s_susi_2);
> >> - osafassert(a_susi_1 && s_susi_1);
> >> + if (a_susi_1 == nullptr) LOG_WA("a_susi_1 is null");
> >> + if (s_susi_1 == nullptr) LOG_WA("s_susi_1 is null");
> >> /* There is a case where both the SUs become Standby: When SU1 is
> >> locked, it
> >> transitions from Act to Quisced, then SU2 goes to Act from Std.
> >> Now
> if
> >> Act assgnment fails, then SU2 will go into Quisced state. Here
> >> both
> the
> >> @@ -590,11 +591,23 @@ static AVD_SU_SI_REL
> >> *avd_sg_2n_act_susi(AVD_CL_CB *cb, AVD_SG *sg,
> >> standby. */
> >> if ((SA_AMF_HA_QUIESCED == avd_su_state_determine(su_1)) &&
> >> (SA_AMF_HA_QUIESCED == avd_su_state_determine(su_2))) {
> >> - osafassert(a_susi_1->su == s_susi_2->su);
> >> - osafassert(a_susi_2->su == s_susi_1->su);
> >> + if (a_susi_1 && s_susi_2 && a_susi_1->su != s_susi_2->su) {
> >> + LOG_WA("a_susi_1->su:%s != s_susi_2->su:%s",
> >> + a_susi_1->su->name.c_str(), s_susi_2->su->name.c_str());
> >> + }
> >> + if (a_susi_2 && s_susi_1 && a_susi_2->su != s_susi_1->su) {
> >> + LOG_WA("a_susi_2->su:%s != s_susi_1->su:%s",
> >> + a_susi_2->su->name.c_str(), s_susi_1->su->name.c_str());
> >> + }
> >> } else {
> >> - osafassert(a_susi_1->su == a_susi_2->su);
> >> - osafassert(s_susi_1->su == s_susi_2->su);
> >> + if (a_susi_1 && a_susi_2 && a_susi_1->su != a_susi_2->su) {
> >> + LOG_WA("a_susi_1->su:%s != a_susi_2->su:%s",
> >> + a_susi_1->su->name.c_str(), a_susi_2->su->name.c_str());
> >> + }
> >> + if (s_susi_1 && s_susi_2 && s_susi_1->su != s_susi_2->su) {
> >> + LOG_WA("s_susi_1->su:%s != s_susi_2->su:%s",
> >> + s_susi_1->su->name.c_str(), s_susi_2->su->name.c_str());
> >> + }
> >> }
> >> a_susi = a_susi_1;
> >> s_susi = s_susi_1;
> >> @@ -602,7 +615,11 @@ static AVD_SU_SI_REL
> >> *avd_sg_2n_act_susi(AVD_CL_CB *cb, AVD_SG *sg,
> >> }
> >>
> >> done:
> >> - *stby_susi = s_susi;
> >> + if (s_susi && s_susi->state == SA_AMF_HA_STANDBY) {
> >> + *stby_susi = s_susi;
> >> + } else {
> >> + *stby_susi = nullptr;
> >> + }
> >>
> >> TRACE_LEAVE2("act: '%s', stdby: '%s'",
> >> a_susi ? a_susi->su->name.c_str() : nullptr,
> >> diff --git a/src/amf/amfd/siass.cc b/src/amf/amfd/siass.cc
> >> index 6a13836f9..f23f3a947 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;
> >> + }
> >> 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);
> >> susi = avd_su_susi_find(cb, su, si->name);
> >> rc = immutil_getAttr("osafAmfSISUFsmState", attributes, 0,
> >> &imm_susi_fsm);
> >> osafassert(rc == SA_AIS_OK);
> >> @@ -335,15 +342,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;
> >> - }
> >> }
> >> // 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/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
