Hi Nagu,
Plesae find my comments with [Minh]
Thanks,
Minh
On 04/07/17 22:53, Nagendra Kumar wrote:
Hi Minh,
Thanks for clarification.
1. But, I didn't get the logic of (tail == tail). What will happen if the
message is last i.e. tail, then will it go into loop?
[Minh]: The if condition is (tail == cb->dnd_list.tail), the @tail is to
memorize the last item in queue before any change.
When we pop a susi message for example, change msg_id, push it back, now
the dnd_list.tail is changed. So the if (tail == cb->dnd_list.tail) is
to know if we reach all items of the queue
2. So, after this patch what is the outcome of scenario listed in the ticket.
Is it able to do a failover ?
We now don't create an absent active susi so we will not do any
absent_failover(). With only one active susi, sg fsm will continue to
create another standby susi. The outcome is:
safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwonDep1,safApp=AmfDemoTwon
saAmfSISUHAState=ACTIVE(1)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
\--
safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwonDep2,safApp=AmfDemoTwon
saAmfSISUHAState=ACTIVE(1)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
safSISU=safSu=SU4\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwon,safApp=AmfDemoTwon
saAmfSISUHAState=ACTIVE(1)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
--
safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwon,safApp=AmfDemoTwon
saAmfSISUHAState=STANDBY(2)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
--
safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwonDep2,safApp=AmfDemoTwon
saAmfSISUHAState=STANDBY(2)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
safSISU=safSu=SU1\,safSg=AmfDemoTwon\,safApp=AmfDemoTwon,safSi=AmfDemoTwonDep1,safApp=AmfDemoTwon
saAmfSISUHAState=STANDBY(2)
saAmfSISUHAReadinessState=READY_FOR_ASSIGNMENT(1)
SU4 has active assignment which was assigned from failover during both
SC were going down. SU1 has newly standby assignments
Thanks
-Nagu
-----Original Message-----
From: minh chau [mailto:[email protected]]
Sent: 02 July 2017 03:28
To: Nagendra Kumar; [email protected];
[email protected]; Praveen Malviya
Cc: [email protected]
Subject: Re: [PATCH 1/1] amfd: Avoid to create duplicated 2N ACTIVE SUSI
for absent assignment V2 [#2477]
Hi Nagu,
Please find my replies with [Minh]
Thanks,
Minh
On 01/07/17 00:33, Nagendra Kumar wrote:
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]: 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.
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]: 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.
/**
+ * 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]: 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.
// 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.
[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.
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
