Hi Minh,
Sorry, my email was a bit confusing.
>> I still don't get how these osafassert(s) are helpful as a part of SG
logic.
In certain situation, it may crash somewhere else. That is the reason,
the logic of checking weird SUSIs should be kept intact along with assert.
Even we are not using avd_sg_validate_headless_cached_rta(), we can
use it for temporary place holder.
In the function, avd_process_state_info_queue(), when
avd_sg_validate_headless_cached_rta() is called, we have SU1 and SU4
having SUSI as Act.
So, two SUs in the same SG has Act assignments. This check should suffice.
So, we can invalidate it here.
Thanks
-Nagu
*From:*minh chau [mailto:[email protected]]
*Sent:* 13 June 2017 09:53
*To:* Nagendra Kumar
*Cc:* [email protected]; Praveen Malviya;
[email protected]; [email protected]
*Subject:* Re: [PATCH 1/1] amf: Replace osafassert by log_warning to
avoid cyclic reboot [#2477]
Hi Nagu,
I seem to remember now, the function
avd_sg_validate_headless_cached_rta() was written in essence it is
just debugging only, invalidating sg is not a solution. There is a
function header comment
/**
* @brief Validate all cached RTAs read from IMM after headless.
This validation is necessary. If AMFD doesn't have this
validation routine and the cached RTAs are invalid,
that would lead into *unpredictably* wrong states, which
is hard to debug (harder if no trace)
* @param Control block (AVD_CL_CB).
* @Return true if valid, false otherwise.
*/
bool avd_sg_validate_headless_cached_rta(AVD_CL_CB *cb) {
I think we are talking about "2 SUSI", more precisely it is problem of
2 different SU(s) being assigned ACTIVE
In the log, SU4 has 3 active SUSIs from 3 different SI
Jun 2 16:05:14.082046 osafamfd [285:285:src/amf/amfd/siass.cc:0440]
>> avd_susi_create: safSu=SU4,safSg=AmfDemoTwon,safApp=AmfDemoTwon
safSi=AmfDemoTwonDep2,safApp=AmfDemoTwon state=1
Jun 2 16:05:14.082080 osafamfd [285:285:src/amf/amfd/siass.cc:0440]
>> avd_susi_create: safSu=SU4,safSg=AmfDemoTwon,safApp=AmfDemoTwon
safSi=AmfDemoTwonDep1,safApp=AmfDemoTwon state=1
Jun 2 16:05:14.082108 osafamfd [285:285:src/amf/amfd/siass.cc:0440]
>> avd_susi_create: safSu=SU4,safSg=AmfDemoTwon,safApp=AmfDemoTwon
safSi=AmfDemoTwon,safApp=AmfDemoTwon state=1
As you pointed that SU1 also has 3 active SUSIs, that we read from IMM
as a helper to perform node_fail(). In this case, 3 SUSIs of SU1 must
be removed and amfd continues to create 3 standby SUSIs for another
SU. When amfd completes to remove 3 SUSIs of SU1, amfd will be able to
create another 3 standby SUSIs as of existing logic of SG.
The osafassert(s) are there just to prevent amfd to remove SUSIs of
SU1 and continue to create standby SUSIs, it even causes cyclic reboot.
I still don't get how these osafassert(s) are helpful as a part of SG
logic. The patch actually does not change any core logic of sg, it
removes the assert to avoid reboot, and make sure
avd_sg_2n_act_susi() always returns active/standby if found (do not
return 2 active SU). The sg logic is still there where sg fsm calls
avd_sg_2n_act_susi() and handling the result of this function.
Thanks,
Minh
On 12/06/17 17:11, Nagendra Kumar wrote:
Hi Minh,
I could see in your trace that 3 SUSI with Act
assignments(state'1' means Act) exists:
Jun 2 16:05:26.441156 osafamfd [285:285:src/amf/amfd/sg_2n_fsm.cc:3379] TEST
>> node_fail: 'safSu=SU1,safSg=AmfDemoTwon,safApp=AmfDemoTwon', 1
Jun 2 16:05:26.441160 osafamfd [285:285:src/amf/amfd/sg_2n_fsm.cc:4095] >>
avd_su_state_determine: SU 'safSu=SU1,safSg=AmfDemoTwon,safApp=AmfDemoTwon'
Jun 2 16:05:26.441163 osafamfd [285:285:src/amf/amfd/sg_2n_fsm.cc:4120] TR
Assigned su'safSu=SU1,safSg=AmfDemoTwon,safApp=AmfDemoTwon',
si'safSi=AmfDemoTwon,safApp=AmfDemoTwon', state'1'
Jun 2 16:05:26.441167 osafamfd [285:285:src/amf/amfd/sg_2n_fsm.cc:4120] TR
Assigned su'safSu=SU1,safSg=AmfDemoTwon,safApp=AmfDemoTwon',
si'safSi=AmfDemoTwonDep1,safApp=AmfDemoTwon', state'1'
Jun 2 16:05:26.441170 osafamfd [285:285:src/amf/amfd/sg_2n_fsm.cc:4120] TR
Assigned su'safSu=SU1,safSg=AmfDemoTwon,safApp=AmfDemoTwon',
si'safSi=AmfDemoTwonDep2,safApp=AmfDemoTwon', state'1'
So, my suggestion is use this information and invalidate the SG as it has
more than one SUSI Act.
Thanks
-Nagu
-----Original Message-----
From: minh chau [mailto:[email protected]]
Sent: 09 June 2017 16:58
To: Nagendra Kumar
Cc:[email protected] <mailto:[email protected]>;
Praveen Malviya;
[email protected]
<mailto:[email protected]>;[email protected]
<mailto:[email protected]>
Subject: Re: [PATCH 1/1] amf: Replace osafassert by log_warning to avoid
cyclic reboot [#2477]
Hi Nagu,
We may have to create 2 active susi(s) so that we can know the sg
invalid. A
question is when the sg gets back to *valid*? We need to remove the
absent
active susi, keep the real active susi and create a new standby susi.
Refreshing sg by deleting all susi(s) may get a complaint from
application
because application's service has gone.
Thanks,
Minh
On 09/06/17 21:13, Nagendra Kumar wrote:
Hi Minh,
1. I am talking about changes in
avd_sg_validate_headless_cached_rta().
Logic could be:
If (assignments are not proper)
{
i_sg->headless_validation = false;
}
2. In avd_cluster_tmr_init_evh():
for (const auto &value : *sg_db) {
AVD_SG *i_sg = value.second;
if ((i_sg->list_of_su.empty() == true) || (i_sg->sg_ncs_spec
== true)) {
continue;
}
==============> Add somewhere below:
If (i_sg->headless_validation == false) { Ignore the SG }
<===================
while (i_sg->any_assignment_absent()) {
// failover with ABSENT SUSI, which had already been
removed during
// headless, until all ABSENT SUSI(s) are failovered
successfully
i_sg->failover_absent_assignment();
}
Thanks
-Nagu
-----Original Message-----
From:[email protected] <mailto:[email protected]>
[mailto:[email protected]]
Sent: 09 June 2017 16:19
To: Nagendra Kumar
Cc: minh chau;[email protected]
<mailto:[email protected]>; Praveen Malviya;
[email protected]
<mailto:[email protected]>;[email protected]
<mailto:[email protected]>
Subject: RE: [PATCH 1/1] amf: Replace osafassert by log_warning
to
avoid cyclic reboot [#2477]
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]
<mailto:[email protected]>; Praveen Malviya;
[email protected]
<mailto:[email protected]>
Cc:[email protected]
<mailto:[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]
<mailto:[email protected]>; Praveen
Malviya;
[email protected]
<mailto:[email protected]>
Cc:[email protected]
<mailto:[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]
<mailto:[email protected]>;
Praveen
Malviya;[email protected]
<mailto:[email protected]>
Cc:[email protected]
<mailto:[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]
<mailto:[email protected]>;[email protected]
<mailto:[email protected]>;
[email protected]
<mailto:[email protected]>;[email protected]
<mailto:[email protected]>
Cc:[email protected]
<mailto:[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
