Good catch. Yes. This makes sense. Alex
On 05/07/2014 01:47 AM, Hans Feldt wrote: > The way you have changed the macro has made useable only for pre-instantiable > SUs since the table you point to is only valid for pre-inst SUs. > > We already have some tech debt with this macro: > // TODO(nagu) remove saAmfSUPreInstantiable check > and move into m_AVD_APP_SU_IS_INSVC > > So maybe it is best that to clean this up properly now? > > Proposal: > * change the macro to a function > * make it aware of SU type (pre/non-pre instantiable) > * change use of the macro where checks for INSTANTIATED has been added. > > What do you think? > > Thanks, > Hans > > >> -----Original Message----- >> From: Alex Jones [mailto:ajo...@genband.com] >> Sent: den 6 maj 2014 18:07 >> To: Hans Feldt; praveen.malv...@oracle.com; nagendr...@oracle.com >> Cc: opensaf-devel@lists.sourceforge.net >> Subject: Re: [PATCH 1 of 1] amfd: fix SU in-service macro [#493] >> >> Comments inline... >> >> On 05/06/2014 09:08 AM, Hans Feldt wrote: >>> First I don't think we should bring in cluster and application state just >>> like this. It is not relevant for this case and we anyway don't >> support such ops. >> >> [Alex] It is relevant in that this macro is a test for SU in-service. >> And, according to 3.2.1.4 of the AMF spec (B.04.01) cluster and >> application admin state need to be checked. I understand that we don't >> support admin operations for these two, but having these checks in here >> doesn't hurt because they are always initialized to UNLOCKED right now. > [Hans] I tried todo the same thing before and got >> If you feel strongly that these should not be included, I can remove >> them, but you know that when someone adds support for admin operations >> for cluster and application, that they will forget to update this >> function. :) >>> Second this macro is out of control and should be changed into a (inline) >>> function instead. >>> Haven't I already done that in my refactoring series? >> [Alex] In the latest default branch this is a macro. Do you want me to >> make this an inline function, or should I let you change all the macros >> to inline functions first? >>> I don't see in the analysis what the problem really is. Why the 7 seconds? >> [Alex] The problem is that the SU is being assigned an SI, even though >> the SU is not instantiated, yet. One of the components in the SU takes >> 7 seconds to instantiate. We can't be assigning an SI to an SU which is >> in INSTANTIATING state. Otherwise, no CSIs get assigned here because >> the component instantiates after the SI assignment. >> >> This is because the macro is only following B.01.01 for determining >> in-service. This is a bug in the B.01.01 spec, which was addressed in >> B.04.01. In B.04.01, section 3.2.1.4, in-service must also check >> cluster admin state, application admin state, SU operational state, and >> SU presence state. >> >>> Thanks, >>> Hans >>> >>>> -----Original Message----- >>>> From: Alex Jones [mailto:ajo...@genband.com] >>>> Sent: den 2 maj 2014 21:27 >>>> To: Hans Feldt; praveen.malv...@oracle.com; nagendr...@oracle.com >>>> Cc: opensaf-devel@lists.sourceforge.net >>>> Subject: [PATCH 1 of 1] amfd: fix SU in-service macro [#493] >>>> >>>> osaf/services/saf/amf/amfd/include/su.h | 13 +++++++++---- >>>> 1 files changed, 9 insertions(+), 4 deletions(-) >>>> >>>> >>>> May 2 18:56:32 linux osafamfnd[12420]: NO Assigned >>>> 'safSi=Dataplane-Np1-SI-1,safApp=DataplaneApp' STANDBY to >>>> 'safSu=Dataplane-SU1,safSg=Dataplane-Np1,safApp=DataplaneApp' >>>> May 2 18:56:39 linux osafamfnd[12420]: NO >>>> 'safSu=Dataplane-SU1,safSg=Dataplane-Np1,safApp=DataplaneApp' Presence >>>> State >>>> INSTANTIATING => INSTANTIATED >>>> >>>> You can see that amfnd has assigned the SI to the SU, but the SU doesn't >>>> transition to INSTANTIATED until 7 seconds later. The >>>> m_AVD_APP_SU_IS_INSVC >>>> macro is using tests from B.01.01, so it doesn't take into account SU >>>> presence >>>> state as specified in B.04.01. >>>> >>>> The solution is to bring this macro up to date with B.04.01 by adding >>>> tests for >>>> cluster admin state, application admin state, node operational state, and >>>> SU >>>> presence state. >>>> >>>> diff --git a/osaf/services/saf/amf/amfd/include/su.h >>>> b/osaf/services/saf/amf/amfd/include/su.h >>>> --- a/osaf/services/saf/amf/amfd/include/su.h >>>> +++ b/osaf/services/saf/amf/amfd/include/su.h >>>> @@ -33,6 +33,8 @@ >>>> #include <amf_defs.h> >>>> #include <msg.h> >>>> #include <comp.h> >>>> +#include <app.h> >>>> +#include <cluster.h> >>>> #include "include/db_template.h" >>>> >>>> /** >>>> @@ -134,11 +136,14 @@ m_AVSV_SEND_CKPT_UPDT_ASYNC_UPDT(cb, su, >>>> } >>>> >>>> #define m_AVD_APP_SU_IS_INSVC(i_su,su_node_ptr) \ >>>> -((su_node_ptr->saAmfNodeAdminState == SA_AMF_ADMIN_UNLOCKED) && \ >>>> +((su_node_ptr->cluster->saAmfClusterAdminState == SA_AMF_ADMIN_UNLOCKED) >>>> && \ >>>> +(i_su->sg_of_su->app->saAmfApplicationAdminState == >>>> SA_AMF_ADMIN_UNLOCKED) && \ >>>> +(i_su->saAmfSUAdminState == SA_AMF_ADMIN_UNLOCKED) &&\ >>>> +(i_su->sg_of_su->saAmfSGAdminState == SA_AMF_ADMIN_UNLOCKED) &&\ >>>> +(su_node_ptr->saAmfNodeAdminState == SA_AMF_ADMIN_UNLOCKED) && \ >>>> (su_node_ptr->saAmfNodeOperState == SA_AMF_OPERATIONAL_ENABLED) && \ >>>> -(i_su->sg_of_su->saAmfSGAdminState == SA_AMF_ADMIN_UNLOCKED) &&\ >>>> -(i_su->saAmfSUAdminState == SA_AMF_ADMIN_UNLOCKED) &&\ >>>> -(i_su->saAmfSUOperState == SA_AMF_OPERATIONAL_ENABLED)\ >>>> +(i_su->saAmfSUOperState == SA_AMF_OPERATIONAL_ENABLED) && \ >>>> +(i_su->saAmfSUPresenceState == SA_AMF_PRESENCE_INSTANTIATED || >>>> i_su->saAmfSUPresenceState == >>>> SA_AMF_PRESENCE_RESTARTING) \ >>>> ) >>>> >>>> #define m_AVD_GET_SU_NODE_PTR(avd_cb,i_su,su_node_ptr) \ ------------------------------------------------------------------------------ Is your legacy SCM system holding you back? Join Perforce May 7 to find out: • 3 signs your SCM is hindering your productivity • Requirements for releasing software faster • Expert tips and advice for migrating your SCM now http://p.sf.net/sfu/perforce _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
