Hi Thuan,Please see comments inline marked with [M2], other than that no more comments from me.
Thanks Minh On 10/7/20 12:32 pm, Thuan Tran wrote:
[M2]: I see, it's good to mention the reboot's from RDE here, so reader can understand immediately there will not be 2 active co-existed.Hi Minh, Yes, #3185 is only help for multi partition cluster rejoin. As verbal discussion, I will move this patch under #2936 umbrella. See my replies for inline comments. Best Regards, ThuanTr -----Original Message----- From: Minh Hon Chau <minh.c...@dektech.com.au> Sent: Thursday, July 9, 2020 8:39 PM To: Thuan Tran <thuan.t...@dektech.com.au>; Thang Duc Nguyen <thang.d.ngu...@dektech.com.au> Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1/1] amf: enhance to work in roaming SC and headless [#3185] Hi Thuan, Is this changed only needed if roamingSC is enabled? I remember last time I tested #2936 Vu's prototype without roamingSC (headless partitions rejoin) I didn't need any change from amf. And some comments inline. Thanks Minh On 3/7/20 7:23 pm, thuan.tran wrote:- amfd reset msg id counter for node that ignore amfnd down event to avoid nodes reboot once more due to mismatch msg id after reboot up from reboot order for sending node_up after sync window. - amfd active order reboot its standby if it detect another active amfd (multi partition cluster rejoin).[M] how's about the "another active", is it going to reboot too? [T] Two active will be handled by RDE detect split-brain. If AMF active reboot, other active may not reboot (as not yet see this active) then it break legacy split-brain detected behavior of RDE.
- amfd standby should reboot itself if see two active peers to avoid standby do cold-sync or be updated with wrong active.[M] how's about one of "two active peers", is it going to reboot too? [T] As above explain, it will reboot but by RDE.
[M2]: As above.
[M2] This is not a major problem, but I still see it's good to move it the "switch", since it's new NCSMDS_RED_UP case and will be separated to NCSMDS_UP/DOWN. It would not be messing up with existing "case", but it show the code is organized and being prepared to handle all i_change cases consistently under the "switch" cases.- amfd just become standby (out of sync) but see active down should reboot itself. --- src/amf/amfd/dmsg.cc | 8 ++++++++ src/amf/amfd/evt.h | 1 + src/amf/amfd/main.cc | 3 +++ src/amf/amfd/mds.cc | 36 ++++++++++++++++++++++++++++++++++-- src/amf/amfd/msg.h | 1 + src/amf/amfd/ndfsm.cc | 2 ++ src/amf/amfd/proc.h | 1 + src/amf/amfd/role.cc | 27 +++++++++++++++++++++++++++ src/amf/amfd/util.cc | 2 +- src/amf/amfnd/amfnd.cc | 2 +- 10 files changed, 79 insertions(+), 4 deletions(-) diff --git a/src/amf/amfd/dmsg.cc b/src/amf/amfd/dmsg.cc index cf4019d8a..5273f358c 100644 --- a/src/amf/amfd/dmsg.cc +++ b/src/amf/amfd/dmsg.cc @@ -75,6 +75,8 @@ void avd_mds_d_enc(MDS_CALLBACK_ENC_INFO *enc_info) { ncs_encode_32bit(&data, msg->msg_info.d2d_chg_role_rsp.role); ncs_encode_32bit(&data, msg->msg_info.d2d_chg_role_rsp.status); break; + case AVD_D2D_ROAMING_SC_SPLITBRAIN:[M] We can name AVD_D2D_REBOOT, the message name should be reflect the purpose of the message. [T] OK, will rename it.+ break; default: LOG_ER("%s: unknown msg %u", __FUNCTION__, msg->msg_type); break; @@ -120,6 +122,8 @@ void avd_mds_d_dec(MDS_CALLBACK_DEC_INFO *dec_info) { static_cast<SaAmfHAStateT>(ncs_decode_32bit(&data)); d2d_msg->msg_info.d2d_chg_role_rsp.status = ncs_decode_32bit(&data); break; + case AVD_D2D_ROAMING_SC_SPLITBRAIN: + break; default: LOG_ER("%s: unknown msg %u", __FUNCTION__, d2d_msg->msg_type); break; @@ -210,6 +214,10 @@ uint32_t avd_d2d_msg_rcv(AVD_D2D_MSG *rcv_msg) { osafassert(0); } break; + case AVD_D2D_ROAMING_SC_SPLITBRAIN: + LOG_ER("Reboot order from Active as roaming SC split-brain detected"); + opensaf_quick_reboot("Split-brain detected"); + break; default: LOG_ER("%s: unknown msg %u", __FUNCTION__, rcv_msg->msg_type); break; diff --git a/src/amf/amfd/evt.h b/src/amf/amfd/evt.h index a9028cde3..a08dccebb 100644 --- a/src/amf/amfd/evt.h +++ b/src/amf/amfd/evt.h @@ -72,6 +72,7 @@ typedef enum avd_evt_type { AVD_IMM_REINITIALIZED, AVD_EVT_UNASSIGN_SI_DEP_STATE, AVD_EVT_ND_MDS_VER_INFO, + AVD_EVT_ROAMING_SC_SPLITBRAIN, AVD_EVT_MAX } AVD_EVT_TYPE;diff --git a/src/amf/amfd/main.cc b/src/amf/amfd/main.ccindex 3b1536721..3cc0d9741 100644 --- a/src/amf/amfd/main.cc +++ b/src/amf/amfd/main.cc @@ -132,6 +132,9 @@ static const AVD_EVT_HDLR g_actv_list[AVD_EVT_MAX] = { invalid_evh, /* AVD_EVT_INVALID */ avd_sidep_unassign_evh, /* AVD_EVT_UNASSIGN_SI_DEP_STATE */ avd_avnd_mds_info_evh, /* AVD_EVT_ND_MDS_VER_INFO*/ + + /* Roaming SC split-brain processing */ + avd_roaming_sc_split_brain_evh, /* AVD_EVT_ROAMING_SC_SPLITBRAIN */ };/* list of all the function pointers related to handling the eventsdiff --git a/src/amf/amfd/mds.cc b/src/amf/amfd/mds.cc index 108f9b8bd..625616f5a 100644 --- a/src/amf/amfd/mds.cc +++ b/src/amf/amfd/mds.cc @@ -396,13 +396,38 @@ static uint32_t avd_mds_svc_evt(MDS_CALLBACK_SVC_EVENT_INFO *evt_info) { uint32_t rc = NCSCC_RC_SUCCESS; AVD_CL_CB *cb = avd_cb;+ if ((evt_info->i_svc_id == NCSMDS_SVC_ID_AVD) &&+ (evt_info->i_change == NCSMDS_RED_UP) && + (evt_info->i_role == V_DEST_RL_ACTIVE) && + (cb->node_id_avd != evt_info->i_node_id) && + (cb->other_avd_adest) && + (cb->node_id_avd_other != evt_info->i_node_id)) { + if (cb->avail_state_avd == SA_AMF_HA_STANDBY) { + LOG_ER("Standby peer see two peers: %x and %x", + cb->node_id_avd_other, evt_info->i_node_id); + opensaf_reboot(0, NULL, "Standby peer see two peers"); + } else if (cb->avail_state_avd == SA_AMF_HA_ACTIVE) { + // Send reboot order to known standby (multi clusters rejoin) + AVD_EVT *evt = new AVD_EVT(); + evt->rcv_evt = AVD_EVT_ROAMING_SC_SPLITBRAIN; + if (m_NCS_IPC_SEND(&cb->avd_mbx, evt, NCS_IPC_PRIORITY_HIGH) != + NCSCC_RC_SUCCESS) { + LOG_ER("%s: ncs_ipc_send failed", __FUNCTION__); + delete evt; + } + } + return rc; + } +[M]: We can move the evt_info->i_change, evt_info->svc_id nested in the current "switch". One question, we have mds telling that evt_info->i_node_id having the role RL_ACTIVE, why we don't reboot evt_info->i_node_id? [T]: I try to avoid touch current "switch" make it difficult for reading/review. All new code in one separate block is better for reading/review. [T]: Why active detect active not reboot, see explain at comment (1), (2)
switch (evt_info->i_change) { case NCSMDS_UP: switch (evt_info->i_svc_id) { case NCSMDS_SVC_ID_AVD: + TRACE("NCSMDS_UP AVD %x", evt_info->i_node_id); /* if((Is this up from other node) && (Is this Up from an Adest)) */ if ((evt_info->i_node_id != cb->node_id_avd) && - (m_MDS_DEST_IS_AN_ADEST(evt_info->i_dest))) { + (m_MDS_DEST_IS_AN_ADEST(evt_info->i_dest)) && + (cb->other_avd_adest == 0)) { cb->node_id_avd_other = evt_info->i_node_id; cb->other_avd_adest = evt_info->i_dest; cb->stby_sync_state = AVD_STBY_OUT_OF_SYNC; @@ -444,11 +469,18 @@ static uint32_t avd_mds_svc_evt(MDS_CALLBACK_SVC_EVENT_INFO *evt_info) { case NCSMDS_DOWN: switch (evt_info->i_svc_id) { case NCSMDS_SVC_ID_AVD: + TRACE("NCSMDS_DOWN AVD %x", evt_info->i_node_id); /* if(Is this down from an Adest) && (Is this adest same as Adest in * CB) */ if (m_MDS_DEST_IS_AN_ADEST(evt_info->i_dest) && m_NCS_MDS_DEST_EQUAL(&evt_info->i_dest, &cb->other_avd_adest)) { - memset(&cb->other_avd_adest, '\0', sizeof(MDS_DEST)); + cb->other_avd_adest = 0; + if ((cb->avail_state_avd == SA_AMF_HA_STANDBY) && + (cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC)) { + LOG_ER("FAILOVER StandBy --> Active FAILED," + " Standby OUT OF SYNC"); + opensaf_quick_reboot("FAILOVER failed"); + } } break;diff --git a/src/amf/amfd/msg.h b/src/amf/amfd/msg.hindex 4b699db71..ef5b40b07 100644 --- a/src/amf/amfd/msg.h +++ b/src/amf/amfd/msg.h @@ -40,6 +40,7 @@ typedef enum { AVD_D2D_CHANGE_ROLE_REQ = AVSV_D2D_CHANGE_ROLE_REQ, AVD_D2D_CHANGE_ROLE_RSP, + AVD_D2D_ROAMING_SC_SPLITBRAIN, AVD_D2D_MSG_MAX, } AVD_D2D_MSG_TYPE;diff --git a/src/amf/amfd/ndfsm.cc b/src/amf/amfd/ndfsm.ccindex 674ef863a..089c41971 100644 --- a/src/amf/amfd/ndfsm.cc +++ b/src/amf/amfd/ndfsm.cc @@ -812,6 +812,8 @@ void avd_mds_avnd_down_evh(AVD_CL_CB *cb, AVD_EVT *evt) { // Ignore amfnd down event in late after clm cb node left then joined // But not ignore if after headless LOG_WA("Ignore '%s' amfnd down event", node->node_name.c_str()); + node->rcv_msg_id = 0; + node->snd_msg_id = 0; TRACE_LEAVE(); return; } diff --git a/src/amf/amfd/proc.h b/src/amf/amfd/proc.h index 4052aecac..188aa4860 100644 --- a/src/amf/amfd/proc.h +++ b/src/amf/amfd/proc.h @@ -86,6 +86,7 @@ void avd_node_down_func(AVD_CL_CB *cb, AVD_AVND *avnd); void avd_nd_sisu_state_info_evh(AVD_CL_CB *cb, struct AVD_EVT *evt); void avd_nd_compcsi_state_info_evh(AVD_CL_CB *cb, struct AVD_EVT *evt); void avd_avnd_mds_info_evh(AVD_CL_CB *cb, AVD_EVT *evt); +void avd_roaming_sc_split_brain_evh(AVD_CL_CB *cb, AVD_EVT *evt); uint32_t avd_node_down(AVD_CL_CB *cb, SaClmNodeIdT node_id); AVD_AVND *avd_msg_sanity_chk(AVD_EVT *evt, SaClmNodeIdT node_id, AVSV_DND_MSG_TYPE msg_typ, uint32_t msg_id); diff --git a/src/amf/amfd/role.cc b/src/amf/amfd/role.cc index 24374de7c..bd497579b 100644 --- a/src/amf/amfd/role.cc +++ b/src/amf/amfd/role.cc @@ -1394,3 +1394,30 @@ uint32_t avd_rde_set_role(SaAmfHAStateT role) { } return rc; } + +/****************************************************************************\ + * Function: avd_roaming_sc_split_brain_evh + * + * Purpose: Send reboot order to standby. + * + * Input: cb - the AVD control block + * evt - The event information. + * + * Returns: None. + * +\**************************************************************************/ +void avd_roaming_sc_split_brain_evh(AVD_CL_CB *cb, AVD_EVT *evt) { + (void)evt; + AVD_D2D_MSG d2d_msg; + TRACE_ENTER(); + + if ((cb->node_id_avd_other == 0) || (cb->other_avd_adest == 0)) { + return; + } + d2d_msg.msg_type = AVD_D2D_ROAMING_SC_SPLITBRAIN; + if (avd_d2d_msg_snd(cb, &d2d_msg) != NCSCC_RC_SUCCESS) { + LOG_ER("Reboot order send failed for Standby AMFD"); + } + + TRACE_LEAVE(); +} diff --git a/src/amf/amfd/util.cc b/src/amf/amfd/util.cc index f2b3c5f2a..6915bddb6 100644 --- a/src/amf/amfd/util.cc +++ b/src/amf/amfd/util.cc @@ -1823,7 +1823,7 @@ void avd_d2n_reboot_snd(AVD_AVND *node) { AVD_DND_MSG *d2n_msg = new AVD_DND_MSG();d2n_msg->msg_type = AVSV_D2N_REBOOT_MSG;- d2n_msg->msg_info.d2n_reboot_info.node_id = node->node_info.nodeId; + d2n_msg->msg_info.d2n_reboot_info.node_id = avd_cb->node_id_avd; d2n_msg->msg_info.d2n_reboot_info.msg_id = ++(node->snd_msg_id);if (avd_d2n_msg_snd(avd_cb, node, d2n_msg) != NCSCC_RC_SUCCESS) {diff --git a/src/amf/amfnd/amfnd.cc b/src/amf/amfnd/amfnd.cc index 9e8739bee..54a3d75fa 100644 --- a/src/amf/amfnd/amfnd.cc +++ b/src/amf/amfnd/amfnd.cc @@ -432,7 +432,7 @@ uint32_t avnd_evt_avd_reboot_evh(AVND_CB *cb, AVND_EVT *evt) { } }- LOG_NO("Received reboot order, ordering reboot now!");+ LOG_NO("Received reboot order from %x, ordering reboot now!", info->node_id); opensaf_reboot(cb->node_info.nodeId, osaf_extended_name_borrow(&cb->node_info.executionEnvironment), "Received reboot order");
_______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
