Hi Anders, The sync not being aborted correctly is the main problem in this ticket. In order to trigger the sync abort (in immnd_proc_server), the node sate has to be R_AVAILABLE.
In the current code, the state is changed from R_AVAILABLE to FULLY_AVAILABLE before sending. The patch postpones that (changing node-state) until successfully sending. If it fails to send, the node-sate remains R_AVAILABLE and sync abort will be triggered. Removing the dying admo doesn't affect the sync abort. I think it's safer to remove them when sync-abort message arrives from IMMD. There's a gap between ::finalizeSync() and sending sync-abort message. I'm afraid that there may be inconsistency between nodes if coord IMMND remove the dying admo "before" the other IMMNDs. BR, Hung Nguyen - DEK Technologies -------------------------------------------------------------------------------- From: Anders Bjornerstedt [email protected] Sent: Wednesday, April 20, 2016 2:48PM To: Hung Nguyen, Neelakanta Reddy, Zoran Milinkovic [email protected], [email protected], [email protected] Cc: Opensaf-devel [email protected] Subject: Re: [devel] [PATCH 1 of 1] imm: Handle error when sending sync-finalize message over fevs [#1735] Hi, I must confes that I dont fully understand the ticket or the fix here. But to me it seems that the problem pointed to by #1735 is that the sync does not get aborted in he coord correctly when it should be aborted. I understand the reason is the is he lack of checking on error for the sending of a message. My simpple (naiive ?) question is: Why not just fix the check of that error and make the abort of the sync simply work. If the sync gets aborted then after a while a new sync will be attempted. Hopefully the next attempt will work. If sync repeatedly fails due to an "mds send problem" then that MDS send problem needs to be looked into. Thanks AndersBj > ----Ursprungligt meddelande---- > Från : [email protected] > Datum : 2016-04-20 - 05:12 (GMT) > Till : [email protected], [email protected] > Kopia : [email protected] > Ämne : Re: [devel] [PATCH 1 of 1] imm: Handle error when sending > sync-finalize message over fevs [#1735] > > Hi Zoran, > > Please find my answer inline. > > BR, > > Hung Nguyen - DEK Technologies > > > -------------------------------------------------------------------------------- > From: Zoran Milinkovic [email protected] > Sent: Tuesday, April 19, 2016 8:22PM > To: Hung Nguyen, Neelakanta Reddy > [email protected], [email protected] > Cc: Opensaf-devel > [email protected] > Subject: RE: [PATCH 1 of 1] imm: Handle error when sending sync-finalize > message over fevs [#1735] > > > Hi Hung, > > Find my comment inline. > > -----Original Message----- > From: Hung Nguyen [mailto:[email protected]] > Sent: Thursday, April 14, 2016 1:45 PM > To: Zoran Milinkovic; [email protected] > Cc: [email protected] > Subject: [PATCH 1 of 1] imm: Handle error when sending sync-finalize message > over fevs [#1735] > > osaf/services/saf/immsv/immnd/ImmModel.cc | 120 > ++++++++++++++++------------- > osaf/services/saf/immsv/immnd/immnd_evt.c | 6 +- > 2 files changed, 72 insertions(+), 54 deletions(-) > > > This patch delays setting node state and deleting dead admin-owners until > sync-finalize message is successfully sent. > If sync server fails to send, sync will be aborted (broadcasting sync-abort > message). > > In sync server and veteran nodes, admin-owners that are in demise will also > be deleted when sync is aborted. > This will prevent IMM from having zombie admin-owners when sync is aborted. > > 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 > @@ -2731,6 +2731,7 @@ ImmModel::pbePrtoPurgeMutations(unsigned > void > ImmModel::abortSync() > { > + AdminOwnerVector::iterator i; > switch(sImmNodeState){ > > case IMM_NODE_R_AVAILABLE: > @@ -2738,6 +2739,23 @@ ImmModel::abortSync() > sNodesDeadDuringSync.clear(); > sImplsDeadDuringSync.clear(); > > + /* Normally, admin-owners which are dead during sync > + * will be cleared in finalizeSync(). > + * When sync is aborted, we don't enter finalizeSync(). > + * So we have to get rid of them here. > + * This is done in sync server and veteran nodes. */ > + for (i = sOwnerVector.begin(); i != sOwnerVector.end();) { > + if ((*i)->mDying) { > + LOG_WA("Removing admin owner %u %s which is in demise ", > + (*i)->mId, (*i)->mAdminOwnerName.c_str()); > + osafassert(adminOwnerDelete((*i)->mId, true) == > SA_AIS_OK); > + /* Above does a lookup of admin owner again. */ > + i = sOwnerVector.begin(); /* Restart of iteration */ > + } else { > + ++i; > + } > + } > + > LOG_NO("NODE STATE-> IMM_NODE_FULLY_AVAILABLE (%u)", > __LINE__); > break; > @@ -18066,48 +18084,58 @@ ImmModel::finalizeSync(ImmsvOmFinalizeSy > > if(isCoord) {//Produce the checkpoint > CcbVector::iterator ccbItr; > - > - sImmNodeState = IMM_NODE_FULLY_AVAILABLE; > - LOG_NO("NODE STATE-> IMM_NODE_FULLY_AVAILABLE %u", __LINE__); > - /*WARNING the controller node here goes to writable state > - based on a NON FEVS message, directly from the sync-process. > - Other nodes go to writable based on the reception of the > - finalizeSync message over FEVS. This means that the controller > - actually opens up for writes before having received the > - finalize sync message. > - > - This should still be safe based on the following reasoning: > - Any writable operation now allowed at this node will be > - sent over fevs AFTER the finalizeSync message that this node > - just sent. This at least is the case for messages arriving at > - this node. So we know that ALL nodes, including this coord node, > - will receive any mutating messages AFTER ALL nodes, including this > - coord node, have received the finalizeSync message. This at least > - for messages arriving from clients at this node. > - > - At the other nodes. Any mutating message does a local check on > writability. > - If currently not writable, the request is rejected locally. > - Shift to writability is only by finalize sync or abortSync. > - */ > + AdminOwnerVector::iterator i; > + > + /* Use 'req' to check if we enter here after sending sync-finalize > message successfully. */ > + if (!req) { > + /* Now we can change the node sate to FULLY_AVAILABLE, > + * and clear the admin-owners which are in demise */ > + > + sImmNodeState = IMM_NODE_FULLY_AVAILABLE; > + LOG_NO("NODE STATE-> IMM_NODE_FULLY_AVAILABLE %u", __LINE__); > + /*WARNING the controller node here goes to writable state > + based on a NON FEVS message, directly from the sync-process. > + Other nodes go to writable based on the reception of the > + finalizeSync message over FEVS. This means that the controller > + actually opens up for writes before having received the > + finalize sync message. > + > + This should still be safe based on the following reasoning: > + Any writable operation now allowed at this node will be > + sent over fevs AFTER the finalizeSync message that this node > + just sent. This at least is the case for messages arriving at > + this node. So we know that ALL nodes, including this coord > node, > + will receive any mutating messages AFTER ALL nodes, including > this > + coord node, have received the finalizeSync message. This at > least > + for messages arriving from clients at this node. > + > + At the other nodes. Any mutating message does a local check on > writability. > + If currently not writable, the request is rejected locally. > + Shift to writability is only by finalize sync or abortSync. > + */ > + > + for (i = sOwnerVector.begin(); i != sOwnerVector.end();) { > + if ((*i)->mDying) { > + LOG_WA("Removing admin owner %u %s which is in demise ", > + (*i)->mId, (*i)->mAdminOwnerName.c_str()); > + osafassert(adminOwnerDelete((*i)->mId, true) == > SA_AIS_OK); > + /* Above does a lookup of admin owner again. */ > + i = sOwnerVector.begin(); /* Restart of iteration */ > + } else { > + ++i; > + } > + } > + > + /* We are not generating sync-finalize message */ > + goto done; > + } > > req->lastContinuationId = sLastContinuationId; > req->adminOwners = NULL; > - AdminOwnerVector::iterator i; > - > - for(i=sOwnerVector.begin(); i!=sOwnerVector.end();) { > - if((*i)->mDying && !((*i)->mReleaseOnFinalize)) { > - LOG_WA("Removing admin owner %u %s (ROF==FALSE) which is in > demise, " > - "BEFORE generating finalize sync message", (*i)->mId, > - (*i)->mAdminOwnerName.c_str()); > - osafassert(adminOwnerDelete((*i)->mId, true) == SA_AIS_OK); > - //Above does a lookup of admin owner again. > - i=sOwnerVector.begin();//Restart of iteration. > - } else { > - ++i; > - } > - } > - > - for(i=sOwnerVector.begin(); i!=sOwnerVector.end(); ++i) { > + > + for (i = sOwnerVector.begin(); > + i != sOwnerVector.end() && (!(*i)->mDying || > ((*i)->mReleaseOnFinalize)); /* Exclude mDying with ROF==FALSE */ > > [Zoran] The upper code does not exclude ROF == FALSE. It actually stops the > loop on the first admin owner who is dying and ROF == true; > If the code above 'for' loop is removed, then it's very likely that the 'for' > loop may exit before the end of sOwnerVector. Without the upper removed code, > there might be a dying admin owner in sOwnerVector. > > [Hung] Yes, that's loop is wrong. The condition used to exclude the > "dying+ROF=TRUE" should be put inside the for loop. Something like this: > for (i = sOwnerVector.begin(); i != sOwnerVector.end(); i++) { if > ((*i)->mDying && !(*i)->mReleaseOnFinalize) continue; /* Exclude mDying > with ROF==FALSE */ ... } I also want to explain more what I did in the > patch. Here's what coord IMMND does in the original code: 1. Set node > state to FULLY_AVAILABLE 2. Loop through sOwnerVector and remove all > "dying+ROF=FALSE" admo. 3. Loop through sOwnerVector and add all to the > message (req->adminOwners). That means both "healthy" admo and > "dying+ROF=TRUE" admo will be synced to other nodes. 4. Loop through > sOwnerVector and remove all "dying+ROF=TRUE" admo. 5. Add implementers, > classes, ccbs to messsage. 6. Send message over fevs. I changed the code > to: 1. Loop through sOwnerVector and only add "healthy" admo and > "dying+ROF=TRUE" admo to the message. All "dying+ROF=FALSE" admo will be > excluded from the message. We don't actually delete any admo at this > moment. 2. Add implementers, classes, ccbs to messsage. 3. Send message > over fevs. 4. If it succeed to send, set node state to FULLY_AVAILABLE > and remove all "dying" admo. If not, the state will remain R_AVAILABLE > to trigger a sync abort broadcasted to all IMMND. The "dying" admo will > be cleared when abort sync message arrives. The other IMMNDs > (sync-clients and veterans) will receive the same sync-finalize message > (containing only "healthy" admo and "dying+ROF=TRUE" admo). No code > change is needed for them. The main purpose of the patch is to postpone > setting node-sate and removing dying admo until successfully sending > sync-finalize message. Cleaning dying admo in ::abortSync() also fixes > resource leak problem. In the current code dying admo only are cleaned > up in ::finalizeSync(). If the sync is aborted, the admo marked as dying > will not be removed until there's an successful sync. Sync abort can > happen even before finalizing sync (e.g sync process crashes). Even if > this ticket doesn't exist, we still need to fix that resource leak > problem. I don't want to create another ticket for it. I think it's fine > to fix it here in this ticket. > > > With this patch, this function is called from 4 places, and from only one > place where 'req' is NULL. > > [Hung] I don't get your point here. About 'req' being NULL. Since > setting node-state and removing dying admo are postponed, > ::finalizeSync() needed to be called twice. The 1st time is for > generating message, the 2nd time (after sending) is for setting > node-state and removing admo. I don't want to add another argument for > ::finalizeSync() or to create new function. That's why I used 'req' to > check. When req is NULL, it will just set node-state and remove dying > admo and then "goto done". I will resend the patch for review when you > and Neel are done reviewing. > Thanks, > Zoran > > + ++i) { > ImmsvAdmoList* ai = (ImmsvAdmoList *) > calloc(1, sizeof(ImmsvAdmoList)); > ai->id = (*i)->mId; > @@ -18156,20 +18184,6 @@ ImmModel::finalizeSync(ImmsvOmFinalizeSy > LOG_IN("finalizeSync message contains %u admin-owners", > (unsigned int) sOwnerVector.size()); > > - for(i=sOwnerVector.begin(); i!=sOwnerVector.end();) { > - if((*i)->mDying) { > - osafassert((*i)->mReleaseOnFinalize); > - LOG_WA("Removing admin owner %u %s (ROF==TRUE) which is in > demise, " > - "AFTER generating finalize sync message", (*i)->mId, > - (*i)->mAdminOwnerName.c_str()); > - osafassert(adminOwnerDelete((*i)->mId, true) == SA_AIS_OK); > - //Above does a lookup of admin owner again. > - i=sOwnerVector.begin();//Restart of iteration. > - } else { > - ++i; > - } > - } > - > /* Done with generate Admo */ > > req->implementers = NULL; > 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 > @@ -5763,10 +5763,14 @@ static uint32_t immnd_evt_proc_sync_fina > if (proc_rc != NCSCC_RC_SUCCESS) { > TRACE_2("Failed send fevs message"); /*Error > already logged in fevs_fo */ > err = SA_AIS_ERR_NO_RESOURCES; > + /* Sync will be aborted in immnd_proc_server() > */ > + } else { > + /* Change the node sate to FULLY_AVAILABLE and > clear the admin-owners which are in demise */ > + immModel_finalizeSync(cb, NULL, SA_TRUE, > SA_FALSE); > + cb->mSyncFinalizing = 0x1; > } > > free(tmpData); > - cb->mSyncFinalizing = 0x1; > } > } else { > LOG_ER("Will not allow sync messages from any process except > sync process"); > > > ------------------------------------------------------------------------------ > Find and fix application performance issues faster with Applications Manager > Applications Manager provides deep performance insights into multiple tiers of > your business applications. It resolves application problems quickly and > reduces your MTTR. Get your free trial! > https://ad.doubleclick.net/ddm/clk/302982198;130105516;z > _______________________________________________ > Opensaf-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/opensaf-devel > ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
