Hi, Find my comment inline started with [Zoran_2]
From: Hung Nguyen [mailto:hung.d.ngu...@dektech.com.au] Sent: Wednesday, October 07, 2015 1:24 PM To: Zoran Milinkovic; reddy.neelaka...@oracle.com; Anders Björnerstedt Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [devel] [PATCH 1 of 1] imm: synchronize applier set on all nodes [#1504] Hi Zoran, Please find my comments started with [Hung_2]. Best Regards, Hung Nguyen - DEK Technologies -------------------------------------------------------------------------------- From: Zoran Milinkovic zoran.milinko...@ericsson.com<mailto:zoran.milinko...@ericsson.com> Sent: Wednesday, October 07, 2015 6:10PM To: Hung Nguyen, Neelakanta Reddy, Anders Bjornerstedt hung.d.ngu...@dektech.com.au<mailto:hung.d.ngu...@dektech.com.au>, reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com>, anders.bjornerst...@ericsson.com<mailto:anders.bjornerst...@ericsson.com> Cc: Opensaf-devel opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: RE: [devel] [PATCH 1 of 1] imm: synchronize applier set on all nodes [#1504] Hi Hung, Find my comments inline started with [Zoran] From: Hung Nguyen [mailto:hung.d.ngu...@dektech.com.au] Sent: Wednesday, October 07, 2015 7:11 AM To: Zoran Milinkovic; reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com>; Anders Björnerstedt Cc: opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: Re: [devel] [PATCH 1 of 1] imm: synchronize applier set on all nodes [#1504] Hi Zoran, Please find the inline comments below. This patch fixes the problem but there are a few more things need to be done. 1) The only thing that the remote nodes should know about an appliers is its existence. The class<->applier mapping and object<->applier mapping should not be created on remote nodes at all. In these function: immnd_evt_proc_cl_impl_set() immnd_evt_proc_cl_impl_rel() immnd_evt_proc_obj_impl_set() immnd_evt_proc_obj_impl_rel() if it's an applier and not originated at this node, we should do nothing and return. [Zoran] I would leave this for the future enhancement as Anders proposed. This patch is also going to OpenSAF 4.5 and 4.6, and I don't want to change the protocol there. 2) Since the remote nodes doesn't have the applier mapping, they should not do the ccb interference check. In the ccb check of ImmModel::implementerSet(), 'if(isApplier)' should be changed to 'if(isApplier && conn)'. [Zoran] This is a good point. I would also leave this for the enhancement. The reason is the same as in the above comment. I also have some inline comments for the patch, please find them below. Best Regards, Hung Nguyen - DEK Technologies -------------------------------------------------------------------------------- From: Zoran Milinkovic zoran.milinko...@ericsson.com<mailto:zoran.milinko...@ericsson.com> Sent: Wednesday, October 07, 2015 10:10AM To: Neelakanta Reddy reddy.neelaka...@oracle.com<mailto:reddy.neelaka...@oracle.com> Cc: Opensaf-devel opensaf-devel@lists.sourceforge.net<mailto:opensaf-devel@lists.sourceforge.net> Subject: [devel] [PATCH 1 of 1] imm: synchronize applier set on all nodes [#1504] osaf/services/saf/immsv/immnd/ImmModel.cc | 44 +++++++++++++++++++++++++++++++ osaf/services/saf/immsv/immnd/immnd_evt.c | 7 +++- 2 files changed, 49 insertions(+), 2 deletions(-) In the first request of OiImplementerSet, from library to IMMND, a pre-check is added for checking if the applier can be set on a class or an object. The pre-check will improve response and skip sending the message over fevs, if the applier cannot be set to the class or the object. If the pre-check is successful, then it might be a short time frame that applier set can still fail until the message sent over fevs reach all nodes. If applier set fails at this point, discard implementer message will be sent to all nodes. diff --git a/osaf/services/saf/immsv/immnd/ImmModel.cc b/osaf/services/saf/immsv/immnd/ImmModel.cc --- a/osaf/services/saf/immsv/immnd/ImmModel.cc +++ b/osaf/services/saf/immsv/immnd/ImmModel.cc @@ -1617,6 +1617,50 @@ immModel_implIsFree(IMMND_CB *cb, const if(impl->mId == 0) {return SA_AIS_OK;} if(impl->mDying) {return SA_AIS_ERR_TRY_AGAIN;} + /* Check if applier can be attached to a class or an object */ + if(impl->mApplier) { + CcbVector::iterator i; + CcbInfo *ccb; + ObjectMutationMap::iterator omit; + ObjectMap::iterator oi; + ObjectInfo *obj; + ImplementerSetMap::iterator ismIter; + + for(i=sCcbVector.begin(); i!=sCcbVector.end(); ++i) { + ccb = (*i); + if(ccb->isActive()) { + for(omit=ccb->mMutations.begin(); omit!=ccb->mMutations.end(); ++omit) { + oi = sObjectMap.find(omit->first); + osafassert(oi != sObjectMap.end()); + obj = oi->second; + + if( ! obj->mClassInfo->mAppliers.empty()) { + ImplementerSet::iterator ii = obj->mClassInfo->mAppliers.begin(); + for(; ii != obj->mClassInfo->mAppliers.end(); ++ii) { + if((*ii) == impl) { + TRACE("TRY_AGAIN: ccb %u is active on object '%s' " + "bound to class applier '%s'. Can not re-attach applier", + ccb->mId, omit->first.c_str(), implName.c_str()); + return SA_AIS_ERR_EXIST; [Hung] Why it's SA_AIS_ERR_EXIST and not SA_AIS_ERR_TRY_AGAIN? [Zoran] I forgot to change the trace to ERR_EXIST. If the function return ERR_TRY_AGAIN, then ERR_TRY_AGAIN will be sent to OM side, and that is not the correct error code in this situation. [Hung_2] The main check (in ImmModel::implementerSet) returns ERR_TRY_AGAIN for CCB interference. Why we return ERR_EXIST here in the pre-check? That's inconsistent. Moreover, according to the SAF AIS, saImmOiImplementerSet() returns ERR_EXIST when: "SA_AIS_ERR_EXIST- An Object Implementer with the same name is already registered with the IMM Service." [Zoran_2] I just realized that the whole block is a mistake and it's useless. It will return ERR_EXIST anyway. I think that we don't need this part of the code at all. + } + } + } + + ismIter = sObjAppliersMap.find(omit->first); + if(ismIter != sObjAppliersMap.end()) { + ImplementerSet *implSet = ismIter->second; + if(implSet->find(impl) != implSet->end()) { + TRACE("TRY_AGAIN: ccb %u is active on object '%s' " + "bound to object applier '%s'. Can not re-attach applier", + ccb->mId, omit->first.c_str(), implName.c_str()); + return SA_AIS_ERR_EXIST; + } + } + } + } + } + } + return SA_AIS_ERR_EXIST; } diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c b/osaf/services/saf/immsv/immnd/immnd_evt.c --- a/osaf/services/saf/immsv/immnd/immnd_evt.c +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c @@ -9543,7 +9543,7 @@ static void immnd_evt_proc_impl_set_rsp( if (originatedAtThisNd) { /*Send reply to client from this ND. */ immnd_client_node_get(cb, clnt_hdl, &cl_node); - if (cl_node == NULL) { + if (cl_node == NULL || (err == SA_AIS_ERR_TRY_AGAIN && !cl_node->mIsStale)) { [Hung] This is for appliers only, not for regular OI. The if statement should include 'isApplier' (or something like that). [Zoran] This is only for appliers. ERR_TRY_AGAIN is returned only for appliers. There is only one case where ERR_TRY_AGAIN is returned and that it's not applier. The check in implementerSet() is: if(immNotWritable() && !protocol43Allowed()) { TRACE_LEAVE(); return SA_AIS_ERR_TRY_AGAIN; } This case will send one extra discard implementer message. [Hung_2] Please check ImmModel::implementerSet again. It also returns ERR_TRY_AGAIN for regular OI. if(obj->mImplementer == info) { osafassert(!isApplier); TRACE("TRY_AGAIN: ccb %u is active on object '%s' bound to OI name " "'%s'. Can not set re-attach implementer", ccb->mId, omit->first.c_str(), implName.c_str()); err = SA_AIS_ERR_TRY_AGAIN; goto done; } [Zoran_2] I'll send the new code to the review. Best regards, Zoran Why we have to check for 'cl_node->mIsStale' here? Stale or not we still have to discard the applier on remote nodes. [Zoran] Correct. I'll fix this in the next patch /* Client was down */ TRACE_5("Failed to get client node, discarding implementer id:%u for connection: %u", implId, conn); [Hung] The trace is misleading in case of discarding applier. [Zoran] I'll fix this too. memset(&send_evt, '\0', sizeof(IMMSV_EVT)); @@ -9556,7 +9556,10 @@ static void immnd_evt_proc_impl_set_rsp( /* Mark the implementer as dying to make sure no upcall is sent to the client. The implementer will be really discarded when global-discard message comes */ immModel_discardImplementer(cb, implId, SA_FALSE, NULL, NULL); - return; + + if(cl_node == NULL) { + return; + } } else if (cl_node->mIsStale) { [Hung] Should be a new if statement instead of 'else if'. Just to make sure IMMND don't send response message when ERR_TRY_AGAIN and mIsStale coincide. [Zoran] I will fix this. Best regards, Zoran LOG_WA("IMMND - Client went down so no response"); return; ------------------------------------------------------------------------------ _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net<mailto:Opensaf-devel@lists.sourceforge.net> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Full-scale, agent-less Infrastructure Monitoring from a single dashboard Integrate with 40+ ManageEngine ITSM Solutions for complete visibility Physical-Virtual-Cloud Infrastructure monitoring from one console Real user monitoring with APM Insights and performance trend reports Learn More http://pubads.g.doubleclick.net/gampad/clk?id=247754911&iu=/4140 _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
