Hi Minh, Thank you so much, appreciated ! Can you please help with the steps : how did you get the errors added by the patch. I run the following commands on opensaf directory(with the patch): 1. make cpplint > /tmp/cpplint 2>&1 ==> This gives lots of errors. This command didn't work in amfnd folder, only works in opensaf folder. 2. make cppcheck.xml > /tmp/cppcheck 2>&1 ==> This is running on all the code base. This command didn't work in amfnd folder.
Please find my responses with [Anand2]. Thanks Anand Sundararaj Senior Solutions Architect | +1 480 686 4772 www.GetHighAvailability.com (https://am2.myprofessionalmail.com/appsuite/www.GetHighAvailability.com) Get High Availability Today! NJ, USA: +1 508-507-6507 > On 08/05/2020 5:06 PM minhchau <minh.c...@dektech.com.au> wrote: > > > Hi Anand, > > I have attached the static code check errors in tickets, and please see > my replies with [M2] > > Thanks > > Minh > > On 6/8/20 9:18 am, s.an...@gethighavailability.com wrote: > > Hi Minh, > > Many thanks for your response and comments. > > I will check cpplint/cppcheck errors. Also, can you please share these > > errors, just to see if they are matching with mine (to avoid the versions > > issues). > > Please check my responses with [Anand] below. > > > > Thanks > > Anand Sundararaj > > Senior Solutions Architect | +1 480 686 4772 > > www.GetHighAvailability.com > > (https://am2.myprofessionalmail.com/appsuite/www.GetHighAvailability.com) > > Get High Availability Today! > > NJ, USA: +1 508-507-6507 > > > >> On 08/05/2020 2:43 PM minhchau <minh.c...@dektech.com.au> wrote: > >> > >> > >> Hi Anand, > >> > >> Sorry for late reply, being busy with other things. > >> > >> We have been using cpplint/cppcheck in > >> https://sourceforge.net/p/opensaf/wiki/static%20analysis/, there are > >> errors reported by cpplint/cppcheck that the patch is adding, would be > >> good if you correct them. > >> > >> And a few other comments inline with [M]. > >> > >> Thanks > >> > >> Minh > >> > >> On 31/7/20 9:44 pm, s.an...@gethighavailability.com wrote: > >>> From: Anand Sundararaj <s.an...@gethighavailability.com> > >>> > >>> --- > >>> src/amf/amfnd/amfnd.cc | 22 ++-- > >>> src/amf/amfnd/avnd_cb.h | 2 + > >>> src/amf/amfnd/avnd_comp.h | 2 + > >>> src/amf/amfnd/clm.cc | 33 +++++- > >>> src/amf/amfnd/err.cc | 23 ++++- > >>> src/amf/amfnd/imm.cc | 258 > >>> ++++++++++++++++++++++++++++++++++++++++------ > >>> 6 files changed, 294 insertions(+), 46 deletions(-) > >>> > >>> diff --git a/src/amf/amfnd/amfnd.cc b/src/amf/amfnd/amfnd.cc > >>> index 1bf057d..9c0aa03 100644 > >>> --- a/src/amf/amfnd/amfnd.cc > >>> +++ b/src/amf/amfnd/amfnd.cc > >>> @@ -27,7 +27,6 @@ > >>> #include <cinttypes> > >>> > >>> #include "amf/amfnd/avnd.h" > >>> - > >>> // Remember MDS install version of Agents. It can be used to send msg > >>> to Agent > >>> // based on their versions. > >>> std::map<MDS_DEST, MDS_SVC_PVT_SUB_PART_VER> agent_mds_ver_db; > >>> @@ -203,12 +202,17 @@ uint32_t > >>> avnd_evt_avnd_avnd_api_resp_msg_hdl(AVND_CB *cb, AVND_EVT *evt) { > >>> AVND_COMP *o_comp = > >>> m_AVND_INT_EXT_COMPDB_REC_GET(cb->internode_avail_comp_db, > >>> comp_name); > >>> if (nullptr == o_comp) { > >>> - LOG_ER("Couldn't find comp in Inter/Ext Comp DB"); > >>> - res = NCSCC_RC_FAILURE; > >>> - goto done; > >>> + TRACE("Couldn't find comp in Inter/Ext Comp DB"); > >>> + // Check if it is a internode responses like error report, etc. > >>> + o_comp = avnd_cb->compdb_internode.find(comp_name); > >>> + if (nullptr == o_comp) { > >>> + LOG_ER("Couldn't find comp in internode comp DB"); > >>> + res = NCSCC_RC_FAILURE; > >>> + goto done; > >>> + } else > >>> + TRACE("Comp '%s' found in internode comp DB", > >>> o_comp->name.c_str()); > >>> } > >>> reg_dest = o_comp->reg_dest; > >>> - > >>> > >>> /***************************************************************************** > >>> * Some processing for registration and unregistration starts > >>> > >>> *****************************************************************************/ > >>> @@ -275,9 +279,13 @@ uint32_t avnd_evt_avnd_avnd_api_resp_msg_hdl(AVND_CB > >>> *cb, AVND_EVT *evt) { > >>> > >>> /* We have to fprwrd this message to AvA. */ > >>> res = avnd_amf_resp_send(cb, resp_info->type, resp_info->rc, > >>> - (uint8_t *)ha_state, ®_dest, > >>> &avnd_msg->mds_ctxt, > >>> + (uint8_t *)ha_state, &o_comp->reg_dest, > >>> &avnd_msg->mds_ctxt, > >>> nullptr, false); > >>> - > >>> + if (o_comp->comp_type == AVND_COMP_TYPE_INTER_NODE_NP) { > >>> + TRACE("Comp '%s' deleted from internode comp DB", > >>> o_comp->name.c_str()); > >>> + cb->compdb_internode.erase(o_comp->name); > >>> + delete o_comp; > >>> + } > >>> if (NCSCC_RC_SUCCESS != res) { > >>> LOG_ER("%s: Msg Send to AvA Failed:Comp:%s ,Type: %u, rc:%u, > >>> Dest:%" PRIu64, > >>> __FUNCTION__, comp_name.c_str(), resp_info->type, > >>> resp_info->rc, > >>> diff --git a/src/amf/amfnd/avnd_cb.h b/src/amf/amfnd/avnd_cb.h > >>> index 8b0cc23..a2e5213 100644 > >>> --- a/src/amf/amfnd/avnd_cb.h > >>> +++ b/src/amf/amfnd/avnd_cb.h > >>> @@ -125,6 +125,8 @@ typedef struct avnd_cb_tag { > >>> SaTimeT scs_absence_max_duration; > >>> /* the timer for supervision of the absence of SC */ > >>> AVND_TMR sc_absence_tmr; > >>> + // Used for storing comp, residing on other node. > >>> + AmfDb<std::string, AVND_COMP> compdb_internode; > >>> } AVND_CB; > >>> > >>> #define AVND_CB_NULL ((AVND_CB *)0) > >>> diff --git a/src/amf/amfnd/avnd_comp.h b/src/amf/amfnd/avnd_comp.h > >>> index 49c610e..bbc2aa0 100644 > >>> --- a/src/amf/amfnd/avnd_comp.h > >>> +++ b/src/amf/amfnd/avnd_comp.h > >>> @@ -296,6 +296,8 @@ typedef struct avnd_pxied_rec { > >>> #define AVND_COMP_TYPE_LOCAL_NODE 0x00000001 > >>> #define AVND_COMP_TYPE_INTER_NODE 0x00000002 > >>> #define AVND_COMP_TYPE_EXT_CLUSTER 0x00000004 > >>> +// Not for proxy or proxied, it is used for error report, etc. > >>> +#define AVND_COMP_TYPE_INTER_NODE_NP 0x00000008 > >>> > >>> > >>> /*########################################################################## > >>> COMPONENT DEFINITION (TOP LEVEL) > >>> diff --git a/src/amf/amfnd/clm.cc b/src/amf/amfnd/clm.cc > >>> index 73c8ff8..8841631 100644 > >>> --- a/src/amf/amfnd/clm.cc > >>> +++ b/src/amf/amfnd/clm.cc > >>> @@ -226,9 +226,36 @@ static void clm_track_cb( > >>> (avnd_cb->node_info.member == SA_TRUE)) { > >>> // Act only once on CLM callback. > >>> clm_node_left(notifItem->clusterNode.nodeId); > >>> - } > >>> - } > >>> - > >>> + } else { > >> [M] I think the below block can move to clm_node_left() where it has > >> dealt with other @internode_avail_comp_db, @compdb, so the callback is > >> purely for membership handling. > > [Anand] : Because external/internode proxy-proxied component features is > > broken, I avoided to go into those flows. Also, I think, the code of > > internode_avail_comp_db is not getting called in clm_node_left because > > clm_node_left is called for the same node and inside clm_node_left, it hits > > goto done: > > if (node_id == avnd_cb->node_info.nodeId) { > > /* if you received a node left indication from clm for self node > > terminate all the non_ncs components; ncs components :-TBD */ > > > > LOG_NO("This node has exited the cluster"); > > avnd_cb->node_info.member = SA_FALSE; > > for (comp = avnd_compdb_rec_get_next(avnd_cb->compdb, ""); comp != > > nullptr; > > comp = avnd_compdb_rec_get_next(avnd_cb->compdb, comp->name)) { > > if (comp->su->is_ncs != true) { > > avnd_comp_clc_fsm_run(avnd_cb, comp, > > AVND_COMP_CLC_PRES_FSM_EV_TERM); > > } > > } > > goto done; > > } > > [M2]: I'm a bit curious of your statement "proxy-proxied features is > broken", I may have missed something, can you explain? [Anand2]: Local proxy-proxied feature is supported where proxy and proxied components run on the same node. When proxy runs on one node and proxied runs on another node or proxied component runs external to the cluster, then this is inter-node proxy-proxied feature, which is broken. This requires adding the internode/external components in internode_avail_comp_db. > > [M2]: The clm_node_left() also deals with the local comp as well, not > just the proxied ones. You can absolutely change the clm_node_left() to > update the internode components that you are adding, the point is that > the clm_node_left is to handle the components as a consequence of a node > being left, whereas the clm callback does not. [Anand2]: Yes, Minh, clm_node_left() is only called when the same node(self) exits the cluster and not when the other node exists the cluster. if ((notifItem->clusterNode.nodeId == avnd_cb->node_info.nodeId) && (avnd_cb->node_info.member == SA_TRUE)) { // Act only once on CLM callback. clm_node_left(notifItem->clusterNode.nodeId); } else { So, I added code in the "else" part of it. The "else" is also in "if ((notifItem->clusterChange == SA_CLM_NODE_LEFT) ||" so shouldn't be any issue. > > >> [M]: The ERR_TIMEOUT could make the component keeps retrying. Now the > >> node is left, I think the error code should be ERR_UNAVAILABLE. > > [Anand] : I think user can't know whether the call could complete or not as > > node went down. So, as per Sepcs, I thought, Timeout is more appropriate. > > This is more consistent with other flows/API calls. > > SA_AIS_ERR_TIMEOUT - An implementation-dependent timeout occurred before the > > call could complete. It is unspecified whether the call succeeded or > > whether it did not. > > [Anand] : When user tries again, user will get ERR_UNAVAILABLE: > > if ((oper_status == SA_AMF_OPERATIONAL_DISABLED) || (is_member == > > false)) { > > amf_rc = SA_AIS_ERR_UNAVAILABLE; > > goto immOmAccessor_finalize; > > } > [M2]: OK, I agree. [Anand2]: Thanks > >>> + AVND_COMP *comp = nullptr; > >>> + uint32_t rc; > >>> + std::string name = ""; > >>> + LOG_NO("Node Id:'%d'", notifItem->clusterNode.nodeId); > >>> + // Delete the responses, which was expected to come from this > >>> node. > >>> + while (nullptr != (comp = > >>> + avnd_compdb_rec_get_next(avnd_cb->compdb_internode, ""))) { > >>> + name = comp->name; > >>> + // Check the node id > >>> + TRACE("Comp '%s'", comp->name.c_str()); > >>> + if (comp->node_id == notifItem->clusterNode.nodeId) { > >>> + TRACE("Comp '%s' matched", comp->name.c_str()); > >>> + if (comp->comp_type == AVND_COMP_TYPE_INTER_NODE_NP) { > >>> + // We have to forward this message to AvA. As of now > >>> + // hardcode the value of ERROR_REPORT. > >>> + rc = avnd_amf_resp_send(avnd_cb, AVSV_AMF_ERR_REP, > >>> SA_AIS_ERR_TIMEOUT, > >>> + nullptr, &comp->reg_dest, &comp->mds_ctxt, > >>> + nullptr, false); > >>> + if (NCSCC_RC_SUCCESS != rc) { > >>> + LOG_ER("Sending error response failed for %s", > >>> comp->name.c_str()); > >>> + } > >>> + TRACE("Comp '%s' deleted from internode comp DB", > >>> comp->name.c_str()); > >>> + avnd_cb->compdb_internode.erase(comp->name); > >>> + delete comp; > >>> + } // comp type > >>> + } // Node id match > >>> + } // while loop > >>> + } // Different node id. > >>> + } // if (false == avnd_cb->first_time_up) > >>> } else if (notifItem->clusterChange == SA_CLM_NODE_RECONFIGURED) { > >>> if (avnd_cb->node_info.nodeId == notifItem->clusterNode.nodeId) { > >>> TRACE("local CLM node reconfigured"); > >>> diff --git a/src/amf/amfnd/err.cc b/src/amf/amfnd/err.cc > >>> index aeec373..b442d98 100644 > >>> --- a/src/amf/amfnd/err.cc > >>> +++ b/src/amf/amfnd/err.cc > >>> @@ -59,9 +59,10 @@ > >>> */ > >>> > >>> #include "amf/amfnd/avnd.h" > >>> +#include "osaf/immutil/immutil.h" > >>> +#include "amf/amfnd/imm.h" > >>> > >>> /* static function declarations */ > >>> - > >>> static uint32_t avnd_err_escalate(AVND_CB *, AVND_SU *, AVND_COMP *, > >>> uint32_t *); > >>> > >>> @@ -173,10 +174,24 @@ uint32_t avnd_evt_ava_err_rep_evh(AVND_CB *cb, > >>> AVND_EVT *evt) { > >>> > >>> /* check if component exists on local AvND node */ > >>> comp = avnd_compdb_rec_get(cb->compdb, > >>> Amf::to_string(&err_rep->err_comp)); > >>> - if (!comp) amf_rc = SA_AIS_ERR_NOT_EXIST; > >>> - > >>> - /* determine other error codes, if any */ > >>> > >>> + if (!comp) { > >>> + // Send the message to imm reader thread for further processing > >>> + TRACE("Err_Rep: Sending to Imm thread."); > >>> + AVSV_NDA_AVA_MSG *ava_msg = > >>> + static_cast<AVSV_NDA_AVA_MSG *>(calloc(1, > >>> sizeof(AVSV_NDA_AVA_MSG))); > >>> + memcpy(ava_msg, evt->info.ava.msg, sizeof(AVSV_NDA_AVA_MSG)); > >>> + AVND_EVT *evt_er = avnd_evt_create(cb, AVND_EVT_AVA_ERR_REP, > >>> + &evt->mds_ctxt, &api_info->dest, (void *)ava_msg, 0, 0); > >>> + rc = m_NCS_IPC_SEND(&ir_cb.mbx, evt_er, evt_er->priority); > >>> + if (NCSCC_RC_SUCCESS != rc) { > >>> + LOG_CR("AvND send event to IR mailbox failed, type = %u", > >>> evt_er->type); > >>> + /* if failure, free the event */ > >>> + if (evt_er) avnd_evt_destroy(evt_er); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + } else > >>> + goto done; > >>> + } > >>> /* We need not entertain errors when comp is not in shape */ > >>> if (comp && (m_AVND_COMP_PRES_STATE_IS_UNINSTANTIATED(comp) || > >>> m_AVND_COMP_PRES_STATE_IS_INSTANTIATIONFAILED(comp) || > >>> diff --git a/src/amf/amfnd/imm.cc b/src/amf/amfnd/imm.cc > >>> index f6b175a..f3a83a7 100644 > >>> --- a/src/amf/amfnd/imm.cc > >>> +++ b/src/amf/amfnd/imm.cc > >>> @@ -20,6 +20,7 @@ > >>> #include <atomic> > >>> #include "amf/amfnd/avnd.h" > >>> #include <poll.h> > >>> +#include "osaf/immutil/immutil.h" > >>> #include "amf/amfnd/imm.h" > >>> > >>> ir_cb_t ir_cb; > >>> @@ -106,45 +107,238 @@ void ImmReader::ir_process_event(AVND_EVT *evt) { > >>> uint32_t res = NCSCC_RC_SUCCESS; > >>> AVND_SU *su = 0; > >>> TRACE_ENTER2("Evt type:%u", evt->type); > >>> + if(AVND_EVT_IR == evt->type) { > >>> + /* get the su */ > >>> + su = avnd_sudb_rec_get(avnd_cb->sudb, > >>> + Amf::to_string(&evt->info.ir_evt.su_name)); > >>> + if (!su) { > >>> + TRACE("SU'%s', not found in DB", > >>> + osaf_extended_name_borrow(&evt->info.ir_evt.su_name)); > >>> + goto done; > >>> + } > >>> > >>> - /* get the su */ > >>> - su = avnd_sudb_rec_get(avnd_cb->sudb, > >>> - Amf::to_string(&evt->info.ir_evt.su_name)); > >>> - if (!su) { > >>> - TRACE("SU'%s', not found in DB", > >>> - osaf_extended_name_borrow(&evt->info.ir_evt.su_name)); > >>> - goto done; > >>> - } > >>> - > >>> - if (false == su->is_ncs) { > >>> - res = avnd_comp_config_get_su(su); > >>> - } else { > >>> - res = NCSCC_RC_FAILURE; > >>> - } > >>> + if (false == su->is_ncs) { > >>> + res = avnd_comp_config_get_su(su); > >>> + } else { > >>> + res = NCSCC_RC_FAILURE; > >>> + } > >>> > >>> - { > >>> - AVND_EVT *evt_ir = 0; > >>> - SaNameT copy_name; > >>> - TRACE("Sending to main thread."); > >>> - osaf_extended_name_alloc(su->name.c_str(), ©_name); > >>> - evt_ir = > >>> + { > >>> + AVND_EVT *evt_ir = 0; > >>> + SaNameT copy_name; > >>> + TRACE("Sending to main thread."); > >>> + osaf_extended_name_alloc(su->name.c_str(), ©_name); > >>> + evt_ir = > >>> avnd_evt_create(avnd_cb, AVND_EVT_IR, 0, nullptr, ©_name, > >>> 0, 0); > >>> - osaf_extended_name_free(©_name); > >>> - if (res == NCSCC_RC_SUCCESS) > >>> - evt_ir->info.ir_evt.status = true; > >>> - else > >>> - evt_ir->info.ir_evt.status = false; > >>> + osaf_extended_name_free(©_name); > >>> + if (res == NCSCC_RC_SUCCESS) > >>> + evt_ir->info.ir_evt.status = true; > >>> + else > >>> + evt_ir->info.ir_evt.status = false; > >>> + > >>> + res = m_NCS_IPC_SEND(&avnd_cb->mbx, evt_ir, evt_ir->priority); > >>> + > >>> + if (NCSCC_RC_SUCCESS != res) > >>> + LOG_CR("AvND send event to main thread mailbox failed, type = > >>> %u", > >>> + evt_ir->type); > >>> + > >>> + TRACE("Imm Read:'%d'", evt_ir->info.ir_evt.status); > >>> + > >>> + /* if failure, free the event */ > >>> + if (NCSCC_RC_SUCCESS != res) avnd_evt_destroy(evt_ir); > >>> + } > >>> + } else if (AVND_EVT_AVA_ERR_REP == evt->type) { > >>> + AVSV_AMF_API_INFO *api_info = &evt->info.ava.msg->info.api_info; > >>> + AVSV_AMF_ERR_REP_PARAM *err_rep = &api_info->param.err_rep; > >>> + SaAisErrorT amf_rc = SA_AIS_OK; > >>> + AVND_COMP *comp = 0; > >>> + SaImmHandleT immOmHandle = 0; > >>> + SaVersionT immVersion = {'A', 2, 15}; > >>> + SaImmAccessorHandleT accessorHandle = 0; > >>> + const SaImmAttrValuesT_2 **attributes; > >>> + std::string su_name; > >>> + SaNameT node_name, clm_node_name; > >>> + SaAmfOperationalStateT oper_status; > >>> + bool is_member; > >>> + SaClmNodeIdT node_id; > >>> + SaAisErrorT error; > >>> + // Check if this component is already in the internode db. > >>> + // If it is, then it means that Error Report operation > >>> + // is already undergoing on this comp, so return TRY_AGAIN. > >>> + if (nullptr != > >>> avnd_cb->compdb_internode.find(Amf::to_string(&err_rep->err_comp))) { > >>> + LOG_NO("Already Error Report Operation undergoing on '%s'", > >>> + Amf::to_string(&err_rep->err_comp).c_str()); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + goto imm_not_initialized; > >>> + } > >>> + // Check if component is configured in the Cluster. > >>> + error = saImmOmInitialize_cond(&immOmHandle, nullptr, &immVersion); > >>> + if (error != SA_AIS_OK) { > >>> + LOG_ER("saImmOmInitialize failed: %u", error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + goto imm_not_initialized; > >>> + } > >>> + > >>> + error = amf_saImmOmAccessorInitialize(immOmHandle, accessorHandle); > >>> + if (error != SA_AIS_OK) { > >>> + LOG_ER("amf_saImmOmAccessorInitialize failed: %u", error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + goto immOmHandle_finalize; > >>> + } > >>> + > >>> + // Find the component in the Imm DB. > >>> + if ((error = amf_saImmOmAccessorGet_o2( > >>> + immOmHandle, accessorHandle, Amf::to_string(&err_rep->err_comp), > >>> + nullptr, (SaImmAttrValuesT_2 ***)&attributes)) != SA_AIS_OK) { > >>> + if (error == SA_AIS_ERR_NOT_EXIST) { > >>> + LOG_NO("Component '%s' is not configured", > >>> + Amf::to_string(&err_rep->err_comp).c_str()); > >>> + amf_rc = SA_AIS_ERR_NOT_EXIST; > >>> + } else { > >>> + LOG_ER("amf_saImmOmAccessorGet_o2(for Comp '%s') failed: %u", > >>> + Amf::to_string(&err_rep->err_comp).c_str(), error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + } > >>> + > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + > >>> + // Component is configured. Now, extract SU name from comp name. > >>> + avnd_cpy_SU_DN_from_DN(su_name, Amf::to_string(&err_rep->err_comp)); > >>> + TRACE("SU name '%s'", su_name.c_str()); > >>> + > >>> + // Get the SU attributes from Imm DB. > >>> + if ((error = amf_saImmOmAccessorGet_o2( > >>> + immOmHandle, accessorHandle, su_name, > >>> + nullptr, (SaImmAttrValuesT_2 ***)&attributes)) != SA_AIS_OK) { > >>> + if (error == SA_AIS_ERR_NOT_EXIST) { > >>> + LOG_NO("SU '%s' is not configured", su_name.c_str()); > >>> + amf_rc = SA_AIS_ERR_NOT_EXIST; > >>> + } else { > >>> + LOG_ER("amf_saImmOmAccessorGet_o2(for SU '%s') failed: %u", > >>> + su_name.c_str(), error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + } > >>> > >>> - res = m_NCS_IPC_SEND(&avnd_cb->mbx, evt_ir, evt_ir->priority); > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + > >>> + // Get Amf node name, where this SU is hosted. > >>> + if (immutil_getAttr( > >>> + const_cast<SaImmAttrNameT>("saAmfSUHostedByNode"), > >>> + attributes, 0, &node_name) != SA_AIS_OK) { > >>> + } > >>> + TRACE("Hosting Amf node '%s'", Amf::to_string(&node_name).c_str()); > >>> > >>> - if (NCSCC_RC_SUCCESS != res) > >>> - LOG_CR("AvND send event to main thread mailbox failed, type = %u", > >>> - evt_ir->type); > >>> + // Get Amf node attributes from Imm DB. > >>> + if ((error = amf_saImmOmAccessorGet_o2( > >>> + immOmHandle, accessorHandle, Amf::to_string(&node_name), > >>> + nullptr, (SaImmAttrValuesT_2 ***)&attributes)) != SA_AIS_OK) { > >>> + if (error == SA_AIS_ERR_NOT_EXIST) { > >>> + LOG_NO("Amf Node '%s' is not configured", > >>> Amf::to_string(&node_name).c_str()); > >>> + amf_rc = SA_AIS_ERR_NOT_EXIST; > >>> + } else { > >>> + LOG_ER("amf_saImmOmAccessorGet_o2(for node '%s') failed: %u", > >>> + Amf::to_string(&node_name).c_str(), error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + } > >>> > >>> - TRACE("Imm Read:'%d'", evt_ir->info.ir_evt.status); > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + TRACE("Hosting Amf node '%s' found.", > >>> Amf::to_string(&node_name).c_str()); > >>> > >>> - /* if failure, free the event */ > >>> - if (NCSCC_RC_SUCCESS != res) avnd_evt_destroy(evt_ir); > >>> + // Find the values of oper status from Amf node attributes. > >>> + if (immutil_getAttr( > >>> + const_cast<SaImmAttrNameT>("saAmfNodeOperState"), > >>> + attributes, 0, &oper_status) != SA_AIS_OK) { > >>> + } > >>> + // Find the Clm Node name from Amf node attributes. > >>> + if (immutil_getAttr( > >>> + const_cast<SaImmAttrNameT>("saAmfNodeClmNode"), > >>> + attributes, 0, &clm_node_name) != SA_AIS_OK) { > >>> + } > >>> + TRACE("Amf node saAmfNodeOperState '%u' and Clm node name '%s'", > >>> + oper_status, Amf::to_string(&clm_node_name).c_str()); > >>> + > >>> + // Get Clm node attributes from Imm DB. > >>> + if ((error = amf_saImmOmAccessorGet_o2( > >>> + immOmHandle, accessorHandle, Amf::to_string(&clm_node_name), > >>> + nullptr, (SaImmAttrValuesT_2 ***)&attributes)) != SA_AIS_OK) { > >>> + if (error == SA_AIS_ERR_NOT_EXIST) { > >>> + LOG_NO("Clm Node '%s' is not configured", > >>> + Amf::to_string(&clm_node_name).c_str()); > >>> + amf_rc = SA_AIS_ERR_NOT_EXIST; > >>> + } else { > >>> + LOG_ER("amf_saImmOmAccessorGet_o2(for clm node '%s') failed: %u", > >>> + Amf::to_string(&clm_node_name).c_str(), error); > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + } > >>> + > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + TRACE("Clm node '%s' found.", > >>> Amf::to_string(&clm_node_name).c_str()); > >>> + > >>> + // Find the configured value of saClmNodeIsMember from Clm node > >>> attributes. > >>> + if (immutil_getAttr( > >>> + const_cast<SaImmAttrNameT>("saClmNodeIsMember"), > >>> + attributes, 0, &is_member) != SA_AIS_OK) { > >>> + } > >>> + > >>> + // Find the configured value of saClmNodeID from Clm node attributes. > >>> + if (immutil_getAttr( > >>> + const_cast<SaImmAttrNameT>("saClmNodeID"), > >>> + attributes, 0, &node_id) != SA_AIS_OK) { > >>> + } > >>> + TRACE("Node id '%u' and Clm member '%u'", node_id, is_member); > >>> + > >>> + // If the node is disabled (DOWN) or is not a member of Clm cluster, > >>> then > >>> + // return Unavailable for any operation like this. > >>> + if ((oper_status == SA_AMF_OPERATIONAL_DISABLED) || (is_member == > >>> false)) { > >>> + amf_rc = SA_AIS_ERR_UNAVAILABLE; > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + > >> [M]: The saAmfNodeOperState and saClmNodeIsMember are runtime attr, > >> reading it here from IMM might not be reflecting the real values, e.g. > >> in a large cluster. How about letting the destined node determine its > >> states since it has to respond the sourced call anyway. > > [Anand] : Because, saAmfNodeOperState and saClmNodeIsMember are cached > > attributes, so Imm is supposed to have the latest information. Also, we > > need to decide if the other node is available or not before sending the > > request. > [M2]: IMM does not have latest value for runtime attr, the implementer > usually does. Also, IMM may have to sync them across immnd(s) while you > are trying to read it, thus the attr value might not be latest one. If > you really need to make decision before sending the request, the node > admin state also needs to be considered too. I don't think the local > node has enough knowledge to validate the requests to be sent to the > remote node. [Anand2]: Minh, got it. I proceeded with information, which I got. I am sure you will agree with me that sending these information to a down node, and then waiting for 10 seconds of wait time or expecting failure and then responding, is not good, sync issue is with all the imm read, I believe. Also, there looks some issues in moving it on the target node? This will break backward compatibility of existing error report api and make it complex, because this check is not available for local component. So, when the node is disabled/clm-locked and if you report error, then return is SA_AIS_ERR_INVALID_PARAM from below code as of now because, the component is already terminated when node is down(apart from node shutdown): /* We need not entertain errors when comp is not in shape */ if (comp && (m_AVND_COMP_PRES_STATE_IS_UNINSTANTIATED(comp) || m_AVND_COMP_PRES_STATE_IS_INSTANTIATIONFAILED(comp) || m_AVND_COMP_PRES_STATE_IS_TERMINATIONFAILED(comp))) amf_rc = SA_AIS_ERR_INVALID_PARAM; Putting node oper state and clm status check before it, will break the backward compatibilty issue the return error is SA_AIS_ERR_UNAVAILABLE and not SA_AIS_ERR_INVALID_PARAM. Keeping check after, will make it redundant. Though adding node oper check with this feature is purely safe. Please suggest. > > > >>> + // We have all the information, send the event to the destined node. > >>> + res = avnd_avnd_msg_send(avnd_cb, (uint8_t *)api_info, > >>> api_info->type, &evt->mds_ctxt, > >>> + node_id); > >>> + if (NCSCC_RC_SUCCESS != res) { > >>> + // We couldn't send this to the destined AvND, tell user to try > >>> again. > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + LOG_ER("avnd_int_ext_comp_hdlr:Msg Send Failed: '%u'", res); > >>> + goto immOmAccessor_finalize; > >>> + } else { > >>> + TRACE("avnd_avnd_msg_send:Msg Send Success"); > >>> + // Add the internode component in the DB > >>> + comp = new AVND_COMP(); > >>> + comp->name = Amf::to_string(&err_rep->err_comp); > >>> + comp->node_id = node_id; > >>> + comp->reg_dest = api_info->dest; > >>> + comp->mds_ctxt = evt->mds_ctxt; > >>> + comp->comp_type = AVND_COMP_TYPE_INTER_NODE_NP; > >>> + // Add to the internode available compdb. > >>> + res = avnd_cb->compdb_internode.insert(comp->name, comp); > >>> + if (NCSCC_RC_SUCCESS != res) { > >>> + delete comp; > >>> + comp = nullptr; > >>> + amf_rc = SA_AIS_ERR_TRY_AGAIN; > >>> + LOG_ER("compdb_internode.insert failed: '%u'", res); > >>> + goto immOmAccessor_finalize; > >>> + } > >>> + immutil_saImmOmAccessorFinalize(accessorHandle); > >>> + immutil_saImmOmFinalize(immOmHandle); > >>> + goto done; > >>> + } > >>> +immOmAccessor_finalize: > >>> + immutil_saImmOmAccessorFinalize(accessorHandle); > >>> +immOmHandle_finalize: > >>> + immutil_saImmOmFinalize(immOmHandle); > >>> +imm_not_initialized: > >>> + /* send the response back to AvA */ > >>> + res = avnd_amf_resp_send(avnd_cb, AVSV_AMF_ERR_REP, amf_rc, 0, > >>> &api_info->dest, > >>> + &evt->mds_ctxt, comp, 0); > >>> + if (NCSCC_RC_SUCCESS != res) { > >>> + LOG_ER("avnd_amf_resp_send() failed: %u", res); > >>> + } > >>> } > >>> done: > >>> if (evt) avnd_evt_destroy(evt); _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
