Hi Minh,
But I agree that we need to avoid rebooting the controllers.
But by avoiding assert, I am not sure, let me check.
Thanks
-Nagu
> -----Original Message-----
> From: Nagendra Kumar
> Sent: 07 June 2017 14:41
> To: minh chau; [email protected]; Praveen Malviya;
> [email protected]
> Cc: [email protected]
> Subject: Re: [devel] [PATCH 1/1] amf: Replace osafassert by log_warning to
> avoid cyclic reboot [#2477]
>
> 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! https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__sdm.link_slashdot&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5
> YTpkKY057SbK10&r=Msq2CEtg63eU7sEKWk3a28RY1AX1Y11SftpoiJrP_ek&m
> =x8kq2JTA5qSX4bUJ9glC-ZfPeXvw3s8NuM-
> KzJCpXf4&s=sdR_EgPO_goQqLliX5C9QYANHE3ghEXMq9tFlvu32oo&e=
> _______________________________________________
> Opensaf-devel mailing list
> [email protected]
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__lists.sourceforge.net_lists_listinfo_opensaf-
> 2Ddevel&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK
> 10&r=Msq2CEtg63eU7sEKWk3a28RY1AX1Y11SftpoiJrP_ek&m=x8kq2JTA5qSX
> 4bUJ9glC-ZfPeXvw3s8NuM-
> KzJCpXf4&s=6TalETbGgKtab_QaRt6nwpgUTtb65SX2XKbNrmNkxwU&e=
------------------------------------------------------------------------------
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
