Hi Nagu, I still think that replacing the osaftassert() does not change the current logic of sg fsm as of now. Probably I don't understand by "invalidating" as you mean, could you please send me the patch based on your idea?
Thanks, Minh > Hi Minh, >>> since locking sequence must go through the sg fsm code while rta >>> problem is still there > May not be; by SG locking, I mean we can just send delete to all > assignments, we can work on it later. > >>> I have just attached to ticket the trace files for your reference, amfd >>> has succeeded to remove the active absent susi (for this ticket) with >>> the patch of #2477 > I think we should avoid changing the core logic and focus on how to > extract information, which is lost in RTA update. > As of now, we can easily avoid the situation by invalidating SG and later > we can work on permanent solution. > > Thanks > -Nagu > >> -----Original Message----- >> From: minh chau [mailto:[email protected]] >> Sent: 09 June 2017 07:06 >> 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, >> >> In the situation of this ticket, we have 2 active susi(s), one is absent >> susi from >> sc, another one is real susi from pl. We can have a check to avoid >> restoring >> the active absent susi if there is a real active susi already, but I >> feel doing this >> is not the way that amfd has been encouraged to use sg fsm to recover >> from >> headless. The skeleton of headless recovery has built up on using sg fsm >> node_fail(), we can add a check of 2 active susi(s), but it is not the >> only form >> of rta problem, there should be many variant symptom. We need to enhance >> the sg fsm code for a wider coverage. >> Locking the sg itself is one option, my feeling is that locking sg may >> not work >> as expected, since locking sequence must go through the sg fsm code >> while >> rta problem is still there. I think we need to failover all absent >> susi(s) (by using >> sg fsm node_fail()) before amfd can do anything further. >> I have just attached to ticket the trace files for your reference, amfd >> has >> succeeded to remove the active absent susi (for this ticket) with the >> patch of >> #2477 >> >> Thanks, >> Minh >> >> On 08/06/17 21:25, Nagendra Kumar wrote: >> > Hi Minh, >> > We can add a check if there are more than 2 Act SUSI(on this >> particular situation) in 2N red model, then we can avoid to call fsm >> later(based on headless_validation) of that SG or take some action like >> locking the SG itself(we can do it in #2310). >> > >> > What do you think? >> > >> > Thanks >> > -Nagu >> > >> >> -----Original Message----- >> >> From: minh chau [mailto:[email protected]] >> >> Sent: 08 June 2017 06:44 >> >> 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, >> >> >> >> It can be viewed as a defect from perspective of a user who has been >> >> using the SC absence. In the scenario of reported ticket, there was >> >> no admin operation on going while stopping both controllers. The user >> >> just simply stopped both SCs, the problem occurred due to failover >> >> during both SC going down, and it is from spare SUs deployment in 2N. >> >> The RTA mentioned in PR doc was mainly meant to the additional >> >> attributes in >> >> #1725 (osafAmfSISUFsmState, osafAmfSGSuOperationList, >> >> osafAmfSUSwitch), but you are also right that we can view as an >> >> enhancement since the assignment SUSI is RTA anyway. >> >> I think the problem is hard and agree that we need more thoughts how >> >> to handle such cases comprehensively (maybe #2310), for now let's fix >> >> the problem in a simple way as much as we can. >> >> >> >> Thanks, >> >> Minh >> >> >> >> On 07/06/17 19:10, Nagendra Kumar wrote: >> >>> 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
