- **status**: unassigned --> fixed
- **Milestone**: future --> never
- **Comment**:
I am closing this ticket, please create a new ticket when refactoring ideas
need to be implemented.
---
** [tickets:#84] AMF: refactor SI dependency code**
**Status:** fixed
**Milestone:** never
**Created:** Mon May 13, 2013 04:42 AM UTC by Nagendra Kumar
**Last Updated:** Mon May 13, 2013 04:43 AM UTC
**Owner:** nobody
Migrated from http://devel.opensaf.org/ticket/2470
The code is hard to maintain and needs to be improved.
Initial requirements identified for this ticket:
1) Initial screening logic needs to be modified. In the current logic we see
unnecessary looping and si_dep_state updations happening for a particular
operation.
2) Screening for dependency is also performed while choosing assignments in
avd_sg_2n_su_chose_asgn(). Ideally si_dep_state should be updated prior to
calling assignment algorithm.
3) In avd_siDep.c the code has been changed while addressing issues and
performing enhancements for 2N red model. Although these changes handles
assignments per susi levels, this needs to be verified for other redundancy
models.
4) Previously SI-SI dependency used to be created with both the sponsor SI and
dependent SI in assigned states. But at present dependent SI should be in
unassigned state before configuring si-si dependency between them. This
condition should be considered while adding new si-si dependency dynamically.
5) For better understanding and debugging more comments and log messages need
to be added appropriately.
----------------------------------------------
Changed 11 months ago by hafe ¶
I haven't thought much about any "any design". All I know is that AMF needs
constant refactoring to become more functionable, understandable and
maintainable.
In context of #1796 I removed some old unused code. There's probably more but
that is a good first step.
Another idea I started with was using function pointers between avd_sgproc.c
and the respective redundancy model. It is perfectly suited for that (when not
using C++). Will provide patches for this. That change drastically reduces the
amount of code in avd_sgproc.c that really just blurs the real logic.
Changed 11 months ago by hafe ¶
I think any piece of code (not just AMF/OpenSAF) needs to be constantly
refactored to the requirements of the current designers/maintainers. A piece of
code can be "perfect" in the eyes of the original designer but still not
understandable by others. The alternative to rewrite it from scratch is perhaps
an option but not realistic...
Changed 11 months ago by praveenmalviya ¶
■description modified (diff)
Changed 11 months ago by praveenmalviya ¶
■status changed from assigned to accepted
Changed 10 months ago by hafe ¶
Some things to consider/discuss:
- create avsv design rules
- 2n & n+m should be able to share most of its code, isn't 2n a special case of
n+m?
- Remove optimization when sending SU_SI_ASSIGN_MSG so that a named SI is
always specified. Simplifies logic, no special case...
Changed 10 months ago by ravisekhar ¶
Here is the meeting minutes of si-si dep refactoring conferance discussion:
1. Praveen and Ravi went through the items listed in the ticket.
2. Hans asked whether si dep state is required or not. Whether it can be
eliminated by using si assignment state. Ravi explained that few enums of si
dep state can be avoided using si assignment state, but not all like
failover_under_progress, tol_timer_running. So, it is necessary to have si dep
state.
3. We discussed “si dep state oscillates multiple times while si dep screening,
this needs to avoid with and without si-si dep configuration.”
4. Hans queried whether susi update and notification are updated before amfd
sends susi assignment to amfnd or after receiving susi assignment response from
amfnd. Ravi and Praveen answered the implementation is the same as before that
both are updated beforehand. But this is not in the scope of 2470.
5. Hans keep forward “create avsv design rules”, which all developer in AMF
should follow. That includes variable names, function name size, function
length size, etc. Hans to float initial draft for the same.
6. Hans comments on 2470 ticket : “2n & n+m should be able to share most of its
code, isn't 2n a special case of n+m?”: Right nowscope is not very clea, need
to work on that. This is not in the 2470 scope as of now.
7. Another comments from Hans “Remove optimization when sending
SU_SI_ASSIGN_MSG so that a named SI is always specified. Simplifies logic, no
special case”: This logic exists in the code, need to improve it, not in scope
right now.
8. We discussed few patches floated by Ravi and Hans(like 2636, 2669, etc).
So, this freezes requirement for ticket 2470 and we are going to work on this.
We can share the repo on devel server for patch review.
Thanks
-Nagu
Changed 10 months ago by ravisekhar ¶
Here are the meeting minutes of the si-si dep refactoring discussion
1) If dependency is configured across SGs (SG1 having sponsor sis and SG2
having dependent sis), when screening is performed in SG1, As per the latest
assignment status of sponsor SI's updation(si_dep_state) and action has to be
taken on the dependent SI's which are part of SG2. But this updation and action
will be done only if SG corresponding to dependant SI is Stable.
2)Use si_dep_state_evt() for posting the unassignment and assignment events
only for dependents.
3)Introduction of new si_dep_state AVD_SI_READY_TO_ASSIGN . For this dep state,
previous states are AVD_SI_NO_DEPENDENCY and AVD_SI_SPONSOR_UNASSIGNED.
Destination states are AVD_SI_ASSIGNED and AVD_SI_SPONSOR_UNASSIGNED. The state
will be check pointed except for old versions of amfd.
4)Enum AVD_SI_DEP_SPONSOR_SI_STATE is not required, so has to be removed
5)Discontinue the use of AVD_SI_UNASSIGNING_DUE_TO_DEP.
6)While deducing si_dep_state for any dependent from its sponsors, consider the
assignment state of all its sponsors from sponsor's list. For any sponsor si,
active or quiescing assignment states means sponsor is active.
7)Our approach will be first an si will update its own si_dep_state based upon
its sponsors's assignment state and then it will update si_dep_state
of its dependent sis following the point mentioned in 1). During screening, if
an si is found assigned or unassigned then only update the si_dep_state of
dependent. And there wil be separate routine for taking action based on updated
states following 1).
8)As a part of the documentation, description of each value of AVD_SI_DEP_STATE
needs to be included in avd_siDep.c. The description should include which
values are applicable to an isolated si, only sponsor si, only dependent si or
an si which is both sponsor and dependent. Transition diagrams for
si_dep_states can also be prepared.
9)Before calling avd_sg_2n_su_chose_asgn(), screening will be done. If this
function call returns NULL then action will be taken after making sg fsm stable
without doing further screening.
10)In tolerance timer expiry event and in the processing of si_dep_state_evt(),
first check the sg fsm state. if sg fsm state is stable then only check its
sponsors assignment state and take action. If sg is unstable then mark the
state to READY_TO_UNASSIGN or READY_TO_ASSIGN whichever applicable.
Deviations from the existing behaviour
--------------------------------------------------------------------------------
When taking action(assignment or unassignment) on the dependants based on
sponsor assignment or unassignment, we are not checking whether the SG
corresponding to dependant SI is in Stable state or not. This will lead to
inconsistencies. So as part of refactoring, action will be taken on the
dependant SI only when the corresponding SG is in stable state.
Thanks,
Praveen
Changed 9 months ago by praveenmalviya ¶
■patch_waiting changed from no to yes
Changed 8 months ago by praveenmalviya ¶
■status changed from accepted to closed
■resolution set to fixed
changeset: 3758:382484bb02ab
tag: tip
user: praveen.malviya@…
date: Wed Sep 26 15:34:54 2012 +0530
summary: avsv: refactored si-si dep code for better maintainability (#2470)
remote: added 1 changesets with 16 changes to 16 files
remote: rev 382484bb02ab26809d86d648e985cff435faf257 sent
Changed 8 months ago by hafe ¶
■status changed from closed to reopened
■resolution fixed deleted
■patch_waiting changed from yes to no
http://list.opensaf.org/pipermail/devel/2012-September/027529.html
I also have a number of non-functional comments and cleanup ideas.
Changed 8 months ago by praveenmalviya ¶
Corrected compilation problem:
remote: added 1 changesets with 1 changes to 1 files
remote: rev 2f41e2537b46feca8740e160e752c93b489c794c sent
Changed 6 months ago by praveenmalviya ¶
■patch_waiting changed from no to yes
Changed 6 months ago by praveenmalviya ¶
■patch_waiting changed from yes to no
changeset: 3837:ef259bc8a21d
tag: tip
user: praveen.malviya@…
date: Thu Nov 08 16:27:01 2012 +0530
summary: avsv: added version check for READY_TO_ASSIGN dep state(#2470)
remote: rev ef259bc8a21dbb521746b9e80aaf87068b0d5824 sent
Changed 2 months ago by hafe ¶
■milestone changed from 4.3.GA to future_releases
TLC decision/preparation for 4.3 release
---
Sent from sourceforge.net because [email protected] is
subscribed to https://sourceforge.net/p/opensaf/tickets/
To unsubscribe from further messages, a project admin can change settings at
https://sourceforge.net/p/opensaf/admin/tickets/options. Or, if this is a
mailing list, you can unsubscribe from the mailing list.
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
Opensaf-tickets mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-tickets
