Thanks Neel for the early response on the prototype patch.
Neelakanta Reddy wrote:
> Hi AndersBj,
>
> Reviewed the first version of 2PBE patch. following are the comments:
>
> 1. while applying the patch is failed with one rejection:
>
> 1 out of 13 hunks FAILED -- saving rejects to file
> osaf/services/saf/immsv/immnd/immnd_evt.c.rej
Yes, the purpose of this patch was to provide an early sharing of the
loading arbitration mechanism.
This patch will not be applied as is.
When the 2PBE enhancement [21] is ready for serious review, new patches
will be sent out.
>
>
> The #355 LOG_NO anf the if condition is changed in the #355. The
> following needs to be removed from the patch:
>
> @@ -3301,7 +3304,7 @@ static SaAisErrorT immnd_fevs_local_chec
> immnd_evt_destroy(&frwrd_evt, SA_FALSE, __LINE__);
>
> if ((error != SA_AIS_OK) && (error != SA_AIS_ERR_NO_BINDINGS)) {
> - LOG_NO("Precheck of fevs message failed ERROR:%u",
> error);
> + LOG_NO("Precheck of fevs message type %u failed
> ERROR:%u", frwrd_evt.info.immnd.type, error);
> }
>
> TRACE_LEAVE();
> 2. Typo, ctgrl would have be ctrl:
>
> + node accpet message for each SC. Subsequent may
> have ctgrl->canBeCoord
Yes and accpet -> accept.
>
> 3. General comments:
>
> a. with this patch both controllers must have any one of the files
> like imm.db/immdb.nodeID/imm.xml for 2PBE configuration.
Correct.
If one of the nodes has no files, or no loadable files, then this will
result in the IMMD 2PBE timeout expiring
and loading proceeding with only one SC. The falure case of 2PBE loading
after timeout and thus without
arbitration is dangerous and the greatest drawback with 2PBE. Same thing
can in principle also happen with
DRBD (default timeout there is 20 seconds last I checked). But the risk
is lower for DRBD since it is much
earlier in the start sequence.
The issue of how to handle the straggler SC when/if it *does* attach,
after the timeout, has not been addressed yet.
In principle it should be forced to preload and the IMMD should check
that it is equal to or older than the
file base used for loading the first SC. If the straggler turns out to
be newer then then we have a catstrophy
since the system has reverted to an older version and has already been
running with that for some time.
Depending on how large the difference is, probably different
actions/escalations are in order.
In principle the IMMSV_2PBE_PEER_SC_MAX_WAIT needs to be calibrated per
deployment platform,
based on statistics of the average and standard deviation of the time
taken to reach start of IMMD.
>
> b. If all the parameters are same why is the remote IMMND is
> choosen as co-ordinator?
I could just say: "why not ?".
But there where two reasons why I chose to do that.
First, it turns out that the chances of both SCs being ready to
participate in the loading is greater if
the IMMD asks the remote IMMND SC to become coord, instead of the local one.
Second, since the coord does a bit more execution than the non coord, we
may as well place it
at the standby SC when this is possible. The active SC will in general
be more heavily loaded
due to all the active directors executing there, so anything asymmetric
that can be offloaded to
the standby, should be.
>
> 4. SC-1 has imm.db and SC-2 has imm.xml. Both the nodes 2PBE is
> configured.
>
> case 1:
> a. SC-1(slot-3) is LOADING_SERVER and SC-2(slot-4) is LOADING_CLIENT
But here the states in (a) imply that loading arbitration has already
been done.
Loading will only start *after* coord is chosen, i.e. *after* loading
arbitratin.
>
> b. SC-2 went for preload state.
But SC-2 can not go for preloading *after* entering LOADING_CLIENT, that
is impossible.
So I am assuming you did not see that. That you are contemplating some
hypothetical case,
but that case I claim can not happen.
>
> c. SC-2 after sending parameters to IMMD called AdminownerFinalize.
> Adminownerfinalize got timeout
>
> Jun 13 17:51:54 Slot-4 osafimmloadd: WA Failed on
> saImmOmAdminOwnerFinalize (5)
>
> Jun 13 17:51:52 Slot-3 osafimmnd[29960]: NO ERR_BAD_HANDLE: Admin
> owner 2 does not exist
Yes the finalization of admin-owner in the perloading state needs cleanup.
>
> d. The adminownerFinalize event is discarded because the SC-2 IMMND
> is not in maccepted state.
>
>
> Jun 13 17:51:44.419650 osafimmnd [29784:immnd_evt.c:8029] T2
> DISCARDING msg no:6
> Jun 13 17:51:44.419670 osafimmnd [29784:immnd_evt.c:8035] TR ABT
> cb->highestProcessed:5
> Jun 13 17:51:44.419690 osafimmnd [29784:immnd_evt.c:8037] TR ABT
> cb->highestProcessed INCREMENTED TO:6
Some APIs that are using fevs are actually being used here before
loading has started. This can cause discrepancy
in the fevs count. This also needs to be cleaned up. Bottom line is that
once loading gets going, everything
including fevs should be normalized to working exactly as without 2PBE.
>
> case 2:
>
> If SC-1 is started first and SC-2 is started next
>
> a. starting of SC-1 immediatly crestes adminowner at that time
> sc-2 is not there.
>
> b. when AdminOwnerfinalize is called for the preload in SC-1, at
> the SC-2 will give error as bad_handle.
>
> Jun 13 18:40:44 Slot-4 osafimmd[1655]: NO IMMND coord at 2010f
> Jun 13 18:40:44 Slot-4 osafimmnd[1671]: NO ERR_BAD_HANDLE: Admin
> owner 1 does not exist
>
>
> e. ERR_BAD_HANDLE is because, in the when the node comes up,
> immediatly admin
Again this will be handled more cleanly.
There is actually nothing catastrophic happening here.
The imma code for saImmOmAdminOwnerFinalize and for most finalize calls,
will convert
ERR_BAD_HANDLE to SA_AIS_OK. Since the methid is trying to close a
handle, there
is no point in disturbing the application by telling it the handle is
already closed.
So the above is just an ugly prinout in the syslog.
But I agree that the sylog message needs to be supressed in preloading
since it could be misleading.
>
> 5. SC-1 has imm.db and SC-2 has imm.db with 2PBE configured.
>
> In this case also the message is getting discarded, but the print
> is not there to syslog because in this case the SC-2 reads from imm.db.
Dont really see why there would be a difference in messaging if imm-xml
is used or imm.db. In both cases an immloader process
is forked and it takes care of the details of loading from either
imm.xml or imm.db.
So if you did see different syslog prinotuts then it must be due to
differences in timing, possibly random.
>
> 6. In the 2PBE case the checking of immnd cofiguration file must be
> bypassed. Having different imm.xml, imm.db and directory names are
> valid in 2PBE.
It is not bypassed, but unequal file or dir is actually accepted in the
2PBE case.
A warning is printed out though as I regard this case as unusual.
I would expect most configurations to have the same directory and root
file name even if they run 2PBE.
If you check the code in immd_evt.c for immd_evt_proc_immnd_intro() you
will see that when file or dir
is checked a warning is printed, not an error, and the intro is accepted.
if(strcmp(cb->mPbeFile, evt->info.ctrl_msg.pbeFile.buf)) {
/* Missmatch on Pbe file */
if(cb->mIs2Pbe && node_info->pbeConfigured) {
/* As long as the pbe-file is not empty 2pbe accepts
unequal names.*/
LOG_WA("Pbe file name differ (%s) != (%s). Allowed for
2PBE.",
cb->mPbeFile, evt->info.ctrl_msg.pbeFile.buf);
if(strcmp(cb->mDir, evt->info.ctrl_msg.dir.buf)) {
/* Missmatch on directory */
if(cb->mIs2Pbe && node_info->pbeConfigured) {
LOG_WA("Pbe directory name differ on SCs (%s) != (%s)."
"Allowed for 2PBE.", cb->mDir,
evt->info.ctrl_msg.dir.buf);
if(strcmp(cb->mFile, evt->info.ctrl_msg.xmlFile.buf)) {
/* Missmatch on Xml file */
if(cb->mIs2Pbe && evt->info.ctrl_msg.xmlFile.buf) {
LOG_WA("Xml file name differ on nodess (%s) != (%s)."
"Allowed for 2PBE.",
cb->mFile, evt->info.ctrl_msg.xmlFile.buf);
Thanks for taking a serious look at the code.
/AndersBj
>
>
> /Neel.
>
>
> On Thursday 30 May 2013 06:26 PM, Anders Bjornerstedt wrote:
>> Summary: IMM: Prototype for 2PBE loading (test patch) [#21]
>> Review request for Trac Ticket(s): 21
>> Peer Reviewer(s): Neel, Zoran
>> Pull request to:
>> Affected branch(es): none
>> Development branch:
>>
>> --------------------------------
>> Impacted area Impact y/n
>> --------------------------------
>> Docs n
>> Build system n
>> RPM/packaging n
>> Configuration files n
>> Startup scripts n
>> SAF services y
>> OpenSAF services n
>> Core libraries n
>> Samples n
>> Tests n
>> Other n
>>
>>
>> Comments (indicate scope for each "y" above):
>> ---------------------------------------------
>>
>> changeset 617845e0e935da9e5968e4b85673902aedd06642
>> Author: Anders Bjornerstedt <[email protected]>
>> Date: Thu, 30 May 2013 14:08:10 +0200
>>
>> IMM: Prototype for 2PBE loading (test patch) [#21]
>>
>> This is a testpatch containing a first version of the 2PBE loading
>> mechanism. This patch goes on top of the patch for [#17] the file
>> configuration consistency checking. See separate review request
>> mail for
>> instructions. This patch will not be pushed. The intent with this
>> patch is
>> to allow testing and obtaining feedback on the 2PBE loading.
>>
>> This is the "separate review request mail".
>> To test 2PBE loading do as folows:
>>
>> 1) Ensure that the patch for [#17] and this patch apply successfully
>> on the default branch.
>> 2) Ensure that opensaf builds with configure for --enable-imm-pbe.
>> 3) Verify that opensaf starts and loads without pbe configured in
>> immnd.conf.
>> 4) Configure immnd.conf for regular PBE.
>> 5) Verify that opensaf starts and works normally for regular PBE.
>> Stop opensaf.
>> 6) Configure immd.conf (not immnd.conf) for 2PBE by commenting in the
>> 'export IMMSV_PEER_SC_MAX_WAIT=30' environment variable.
>> 7) Start opensaf.
>>
>> You should now see loading arbitration being done by the active IMMD.
>> This consists of ordering a "preload" at both SC1 and SC2 concurrently.
>> That preload will probe the filesystem; determine which file to be
>> loaded
>> from locally; scan that file (xml or sqlite) for the parameters used in
>> loading arbitration and fially send the paramters to the IMMD via an
>> admin-operation.
>> When the IMMD has received the loading arbitration parameters from both
>> SCs, it will compare them and decide which SC to perform the loading.
>> The IMMD then elects the IMMND coord to be at the SC that "won" the
>> arbitration contest. Loading is then done in the normal way.
>>
>> After loading has completed, a PBE will be started only at the coord
>> IMMND. Thus this patch does not provide the logic for full 2PBE and
>> in particular not the 2 part of the PBE dumping.
>> So after loading, the system works almost as normal 1PBE, with one
>> difference. The file name used by the immpbedd process is not just
>> the regular "imm.db" file. The file name used is the configued
>> file name (imm.db) with the processor id appended as a suffix.
>> Thus from SC1 you will see 'imm.db.2010f' and from SC2 you will see
>> 'imm.db.2020f'.
>> Of course if you run this on a shared file system then you may see both
>> files. One reason for appending the processor id to the pbe file is
>> to allow
>> testing of 2PBE on systems currently prepared only for 1PBE. Thus 2PBE
>> should work also when the PBE files are placed on a shared file system.
>> It would of course be silly to have such a configuration in production.
>> But for testing it may be convenient.
>>
>> One danger has to be pointed out with testing of 2PBE loading when not
>> having the 2PBE dumping in place, is that you have to be very carefull
>> with failover/switchover of SC or failover of immnd-coord.
>> When the immnd-coord moves from one SC ot the other in 1PBE, it tries to
>> avoid regenerating the imm.db file and instead re-attaches to the pre
>> existing imm.db file. With this incomplete 2PBE setup however, the PBE
>> will indeed failover, but if it re-attaches it will re-attach to the
>> imm.db.suffix for that matches the SC it is executing on.
>> The imm.db.suffix file it re-attaches to may be completely out of sync
>> since any CCBS or other persistent writes done prior to the failover
>> would
>> only have reached the other imm.db.suffix file.
>> So *if* you are going to perform any test that implies a failover of
>> immnd
>> coord, then you must remover or rename the imm.db.suffix file at SC
>> where
>> the new immnd coord (and immpbedd) will start. This will force the
>> immpbedd
>> to regenerate the file.
>>
>> When testing loading you may also experiment with updating the epoch or
>> ccb-id in the sqlite file or the xml file.
>>
>> The file chosen for pre-loading and loading is in order of precedence:
>> 1) imm.db.<suffix>
>> 2) imm.db
>> 3) imm.xml
>>
>> If it cant find opne file or fails to open it, it tries the next.
>> But if it succeeds in opening it and preloading starts but fails,
>> then there is no attempt to look for the next file. Instead such
>> a failure is regarded as a corrupt file setup.
>>
>> The loading arbitration done at the IMMD is based on:
>> 1) epoch
>> 2) regular ccb-id.
>> 3) weak-ccb-id (a fake ccb-id for PRT updates and class updates).
>> 4) regular commit-time.
>> 5) weak commit-time.
>>
>> The first level with a difference will decide.
>> If there is no difference (would be the normal case in a well
>> functioning system),
>> then it arbitrarily chooses the coord at the standby SC (offloads the
>> active SC
>> a tiny bit).
>>
>> Please do not hesitate to ask questions.
>> The loading paramters may be extended a bit later.
>>
>>
>> Complete diffstat:
>> ------------------
>> osaf/libs/agents/saf/imma/imma_proc.c | 33 ++++++-
>> osaf/libs/common/immsv/immsv_evt.c | 55 ++++++++++++-
>> osaf/libs/common/immsv/include/immsv_api.h | 2 +-
>> osaf/libs/common/immsv/include/immsv_evt.h | 13 +++-
>> osaf/services/saf/immsv/immd/immd_amf.c | 5 +-
>> osaf/services/saf/immsv/immd/immd_cb.h | 6 +-
>> osaf/services/saf/immsv/immd/immd_db.c | 2 +
>> osaf/services/saf/immsv/immd/immd_evt.c | 82
>> +++++++++++++++++++-
>> osaf/services/saf/immsv/immd/immd_main.c | 40 ++++++++++-
>> osaf/services/saf/immsv/immd/immd_proc.c | 229
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> osaf/services/saf/immsv/immd/immd_proc.h | 3 +-
>> osaf/services/saf/immsv/immd/immd_sbevt.c | 23 +++++-
>> osaf/services/saf/immsv/immloadd/imm_loader.cc | 260
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
>> osaf/services/saf/immsv/immloadd/imm_loader.hh | 6 +-
>> osaf/services/saf/immsv/immloadd/imm_pbe_load.cc | 190
>> ++++++++++++++++++++++++++++++++++++++++++++++--
>> osaf/services/saf/immsv/immnd/ImmModel.cc | 53 ++++++++++++-
>> osaf/services/saf/immsv/immnd/immnd_cb.h | 2 +
>> osaf/services/saf/immsv/immnd/immnd_evt.c | 121
>> ++++++++++++++++++++++++++++++-
>> osaf/services/saf/immsv/immnd/immnd_main.c | 2 +-
>> osaf/services/saf/immsv/immnd/immnd_proc.c | 91
>> +++++++++++++++++++---
>> osaf/services/saf/immsv/immpbed/immpbe_daemon.cc | 19 +++-
>> 21 files changed, 1116 insertions(+), 121 deletions(-)
>>
>>
>> Testing Commands:
>> -----------------
>> <<LIST THE COMMAND LINE TOOLS/STEPS TO TEST YOUR CHANGES>>
>>
>>
>> Testing, Expected Results:
>> --------------------------
>> <<PASTE COMMAND OUTPUTS / TEST RESULTS>>
>>
>>
>> Conditions of Submission:
>> -------------------------
>> <<HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC>>
>>
>>
>> Arch Built Started Linux distro
>> -------------------------------------------
>> mips n n
>> mips64 n n
>> x86 n n
>> x86_64 n n
>> powerpc n n
>> powerpc64 n n
>>
>>
>> Reviewer Checklist:
>> -------------------
>> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>>
>>
>> Your checkin has not passed review because (see checked entries):
>>
>> ___ Your RR template is generally incomplete; it has too many blank
>> entries
>> that need proper data filled in.
>>
>> ___ You have failed to nominate the proper persons for review and push.
>>
>> ___ Your patches do not have proper short+long header
>>
>> ___ You have grammar/spelling in your header that is unacceptable.
>>
>> ___ You have exceeded a sensible line length in your
>> headers/comments/text.
>>
>> ___ You have failed to put in a proper Trac Ticket # into your commits.
>>
>> ___ You have incorrectly put/left internal data in your comments/files
>> (i.e. internal bug tracking tool IDs, product names etc)
>>
>> ___ You have not given any evidence of testing beyond basic build tests.
>> Demonstrate some level of runtime or other sanity testing.
>>
>> ___ You have ^M present in some of your files. These have to be removed.
>>
>> ___ You have needlessly changed whitespace or added whitespace crimes
>> like trailing spaces, or spaces before tabs.
>>
>> ___ You have mixed real technical changes with whitespace and other
>> cosmetic code cleanup changes. These have to be separate commits.
>>
>> ___ You need to refactor your submission into logical chunks; there is
>> too much content into a single commit.
>>
>> ___ You have extraneous garbage in your review (merge commits etc)
>>
>> ___ You have giant attachments which should never have been sent;
>> Instead you should place your content in a public tree to be
>> pulled.
>>
>> ___ You have too many commits attached to an e-mail; resend as threaded
>> commits, or place in a public tree for a pull.
>>
>> ___ You have resent this content multiple times without a clear
>> indication
>> of what has changed between each re-send.
>>
>> ___ You have failed to adequately and individually address all of the
>> comments and change requests that were proposed in the initial
>> review.
>>
>> ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)
>>
>> ___ Your computer have a badly configured date and time; confusing the
>> the threaded patch review.
>>
>> ___ Your changes affect IPC mechanism, and you don't present any results
>> for in-service upgradability test.
>>
>> ___ Your changes affect user manual and documentation, your patch series
>> do not contain the patch that updates the Doxygen manual.
>>
>
------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:
Build for Windows Store.
http://p.sf.net/sfu/windows-dev2dev
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
