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
