ack, code review only. I attach a patch with some suggested
improvements. /Thanks HansN
On 10/14/2015 10:47 AM, [email protected] wrote:
osaf/services/saf/amf/amfd/include/si.h | 1 +
osaf/services/saf/amf/amfd/sg_2n_fsm.cc | 67 +----------
osaf/services/saf/amf/amfd/sg_nway_fsm.cc | 31 +----
osaf/services/saf/amf/amfd/si.cc | 191
++++++++++++++++++++++++++++++
4 files changed, 194 insertions(+), 96 deletions(-)
Return SA_AIS_ERR_BAD_OPERATION for SI SWAP admin op when
1. SI is locked.
2. Only one SU is configured.
3. None of other SUs or its hosting nodes are in unlocked state.
4. All other nodes hosting SUs are down.
5. Other SUs are in inst/term failed state or Out of service.
Return SA_AIS_ERR_TRY_AGAIN for SI SWAP admin op when
1. Only one assignment for M/w SI. This is in assumption that
the another controller will come and join shortly.
2. If nodes hosting other unlocked SUs are joining.
3. If other SUs are in INSTANTIATING/TERMINATING/RESTARTING state.
diff --git a/osaf/services/saf/amf/amfd/include/si.h
b/osaf/services/saf/amf/amfd/include/si.h
--- a/osaf/services/saf/amf/amfd/include/si.h
+++ b/osaf/services/saf/amf/amfd/include/si.h
@@ -141,6 +141,7 @@ public:
bool is_sirank_valid(uint32_t newSiRank) const;
void update_sirank(uint32_t newSiRank);
bool si_dep_states_check();
+ SaAisErrorT si_swap_validate();
private:
AVD_SI(const AVD_SI&);
AVD_SI& operator=(const AVD_SI&);
diff --git a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
--- a/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
+++ b/osaf/services/saf/amf/amfd/sg_2n_fsm.cc
@@ -736,67 +736,11 @@ done:
SaAisErrorT SG_2N::si_swap(AVD_SI *si, SaInvocationT invocation) {
AVD_SU_SI_REL *susi;
SaAisErrorT rc = SA_AIS_OK;
- AVD_AVND *node;
TRACE_ENTER2("'%s' sg_fsm_state=%u", si->name.value, si->sg_of_si->sg_fsm_state);
- if (si->saAmfSIAdminState != SA_AMF_ADMIN_UNLOCKED) {
- LOG_ER("%s SWAP failed - wrong admin state=%u", si->name.value,
- si->saAmfSIAdminState);
- rc = SA_AIS_ERR_TRY_AGAIN;
+ if ((rc = si->si_swap_validate()) != SA_AIS_OK)
goto done;
- }
-
- if (avd_cb->init_state != AVD_APP_STATE) {
- LOG_ER("%s SWAP failed - not in app state (%u)", si->name.value,
- avd_cb->init_state);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
-
- if (si->sg_of_si->sg_fsm_state != AVD_SG_FSM_STABLE) {
- LOG_ER("%s SWAP failed - SG not stable (%u)", si->name.value,
- si->sg_of_si->sg_fsm_state);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
-
- if (si->list_of_sisu == NULL) {
- LOG_ER("%s SWAP failed - no assignments to swap",
si->name.value);
- rc = SA_AIS_ERR_BAD_OPERATION;
- goto done;
- }
-
- if (si->sg_of_si->sg_ncs_spec) {
- if (avd_cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC) {
- LOG_ER("%s SWAP failed - Cold sync in progress",
si->name.value);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
- }
-
- if (si->list_of_sisu->si_next == NULL) {
- LOG_ER("%s SWAP failed - only one assignment", si->name.value);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
-
- /* Since middleware components can still have si->list_of_sisu->si_next
as not NULL, but we need to check
- whether it is unlocked. We need to reject si_swap on controllers when
stdby controller is locked. */
- if (si->sg_of_si->sg_ncs_spec) {
- /* Check if the Standby is there in unlocked state. */
- node = avd_node_find_nodeid(avd_cb->node_id_avd_other);
- if (node == NULL) {
- LOG_NO("SI Swap not possible, node %x is not available",
avd_cb->node_id_avd_other);
- rc = SA_AIS_ERR_BAD_OPERATION;
- goto done;
- }
- if (SA_FALSE == node->node_info.member) {
- LOG_NO("SI Swap not possible, node %x is locked",
avd_cb->node_id_avd_other);
- rc = SA_AIS_ERR_BAD_OPERATION;
- goto done;
- }
- }
/* Identify the active susi rel */
if (si->list_of_sisu->state == SA_AMF_HA_ACTIVE) {
@@ -809,15 +753,6 @@ SaAisErrorT SG_2N::si_swap(AVD_SI *si, S
goto done;
}
- /* If the swap is on m/w si, then check whether any ccb was going on. */
- if (si->sg_of_si->sg_ncs_spec) {
- if (ccbutil_EmptyCcbExists() == false) {
- rc = SA_AIS_ERR_TRY_AGAIN;
- LOG_NO("%s SWAP failed - Ccb going on", si->name.value);
- goto done;
- }
- }
-
/* Check if there is dependency between SI's within SU */
if (avd_sidep_si_dependency_exists_within_su(susi->su)) {
if (avd_sg_susi_mod_snd_honouring_si_dependency(susi->su,
SA_AMF_HA_QUIESCED) == NCSCC_RC_FAILURE) {
diff --git a/osaf/services/saf/amf/amfd/sg_nway_fsm.cc
b/osaf/services/saf/amf/amfd/sg_nway_fsm.cc
--- a/osaf/services/saf/amf/amfd/sg_nway_fsm.cc
+++ b/osaf/services/saf/amf/amfd/sg_nway_fsm.cc
@@ -3532,32 +3532,8 @@ SaAisErrorT SG_NWAY::si_swap(AVD_SI *si,
TRACE_ENTER2("'%s' sg_fsm_state=%u", si->name.value, si->sg_of_si->sg_fsm_state);
- if (si->saAmfSIAdminState != SA_AMF_ADMIN_UNLOCKED) {
- LOG_NO("%s SWAP failed - wrong admin state=%u", si->name.value,
- si->saAmfSIAdminState);
- rc = SA_AIS_ERR_TRY_AGAIN;
+ if ((rc = si->si_swap_validate()) != SA_AIS_OK)
goto done;
- }
-
- if (avd_cb->init_state != AVD_APP_STATE) {
- LOG_NO("%s SWAP failed - not in app state (%u)", si->name.value,
- avd_cb->init_state);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
-
- if (si->sg_of_si->sg_fsm_state != AVD_SG_FSM_STABLE) {
- LOG_NO("%s SWAP failed - SG not stable (%u)", si->name.value,
- si->sg_of_si->sg_fsm_state);
- rc = SA_AIS_ERR_TRY_AGAIN;
- goto done;
- }
-
- if (si->list_of_sisu == NULL) {
- LOG_NO("%s SWAP failed - no assignments to swap",
si->name.value);
- rc = SA_AIS_ERR_BAD_OPERATION;
- goto done;
- }
if ((sg->equal_ranked_su == true) && (sg->saAmfSGAutoAdjust == SA_TRUE)) {
LOG_NO("%s Equal distribution is enabled, si-swap not allowed",
si->name.value);
@@ -3569,11 +3545,6 @@ SaAisErrorT SG_NWAY::si_swap(AVD_SI *si,
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
}
- if (si->list_of_sisu->si_next == NULL) {
- LOG_NO("%s SWAP failed - only one assignment", si->name.value);
- rc = SA_AIS_ERR_BAD_OPERATION;
- goto done;
- }
/*
After SI swap, there should not be violation of
saAmfSGMaxActiveSIsperSU.
diff --git a/osaf/services/saf/amf/amfd/si.cc b/osaf/services/saf/amf/amfd/si.cc
--- a/osaf/services/saf/amf/amfd/si.cc
+++ b/osaf/services/saf/amf/amfd/si.cc
@@ -1500,3 +1500,194 @@ void AVD_SI::update_sirank(uint32_t newS
saAmfSIRank = (newSiRank == 0) ? ~0U : newSiRank;
sg->add_si(this);
}
+
+/*
+ * @brief This function does generic validation for admin op SI_SWAP.
+ * @return int
+ */
+SaAisErrorT AVD_SI::si_swap_validate()
+{
+ AVD_AVND *node;
+ SaAisErrorT rc = SA_AIS_OK;
+ if (saAmfSIAdminState != SA_AMF_ADMIN_UNLOCKED) {
+ LOG_ER("%s SWAP failed - wrong admin state=%u", name.value,
+ saAmfSIAdminState);
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+
+ if (avd_cb->init_state != AVD_APP_STATE) {
+ LOG_ER("%s SWAP failed - not in app state (%u)", name.value,
+ avd_cb->init_state);
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ }
+
+ if (sg_of_si->sg_fsm_state != AVD_SG_FSM_STABLE) {
+ LOG_ER("%s SWAP failed - SG not stable (%u)", name.value,
+ sg_of_si->sg_fsm_state);
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ }
+
+ if (list_of_sisu == NULL) {
+ LOG_ER("%s SWAP failed - no assignments to swap", name.value);
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+
+ /* First check for another node for M/w. */
+ /* Since middleware components can still have si->list_of_sisu->si_next
as not NULL, but we need to check
+ whether it is unlocked. We need to reject si_swap on controllers
when stdby controller is locked. */
+ if (sg_of_si->sg_ncs_spec) {
+ /* Check if the Standby is there in unlocked state. */
+ node = avd_node_find_nodeid(avd_cb->node_id_avd_other);
+ if (node == NULL) {
+ LOG_NO("SI Swap not possible, node %x is not available",
avd_cb->node_id_avd_other);
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+ if (SA_FALSE == node->node_info.member) {
+ LOG_NO("SI Swap not possible, node %x is locked",
avd_cb->node_id_avd_other);
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+ }
+
+ /* Check whether Amfd is in sync if node is illigible to join. */
+ if (sg_of_si->sg_ncs_spec) {
+ if (avd_cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC) {
+ LOG_ER("%s SWAP failed - Cold sync in progress",
name.value);
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ }
+ }
+
+ if (list_of_sisu->si_next == NULL) {
+ LOG_ER("%s SWAP failed - only one assignment", name.value);
+ if (sg_of_si->sg_ncs_spec) {
+ /* Another M/w SU will come anyway because of two
controller,
+ so wait to come back.*/
+ TRACE("M/w SI");
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ }
+
+ /* Check whether one assignment is because of configuration. */
+ if (sg_of_si->list_of_su.size() == 1) {
+ LOG_ER("Only once SU configured");
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ } else {
+ /* The validation logic has been written into many
sub-logics of 'for loop',
+ because of
+ a. keeping the code simple. If we combine them, then
the code
+ becomes complex to understand and dificult to fix
any further bug.
+ b. these code not being executed frequently as these
scenarios are rare.*/
+ bool any_su_unlocked = false;
+ /* 1. If more than one SU configured, check if any
other are unlocked.*/
+ for (const auto& su : sg_of_si->list_of_su) {
+ if ((su->saAmfSUAdminState == SA_AMF_ADMIN_UNLOCKED)
&&
+ (su->su_on_node->saAmfNodeAdminState ==
SA_AMF_ADMIN_UNLOCKED) &&
+ (list_of_sisu->su != su)) {
+ any_su_unlocked = true;
+ break;
+ }
+ }
+ /* All other SUs are unlocked, so return BAD OP. */
+ if (any_su_unlocked == false) {
+ LOG_ER("All other SUs and their hosting nodes are
not in UNLOCKED state");
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+
+ /* 2. If the containing nodes (hosting all other SUs)
absent:
+ return BAD OP.*/
+ bool all_nodes_absent = true;
+ for (const auto& su : sg_of_si->list_of_su) {
+ if ((su->su_on_node->node_state !=
AVD_AVND_STATE_ABSENT) &&
+ (list_of_sisu->su != su)) {
+ all_nodes_absent = false;
+ break;
+ }
+ }
+ /* all other nodes absent, so return BAD OP. */
+ if (all_nodes_absent == true) {
+ LOG_ER("All other nodes hosting SUs are down");
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+
+ /* 3. If node (hosting any other SUs) is joining(A case
of Temporary degeneration):
+ return TRY_AGAIN. */
+ bool any_nodes_joining = false;
+ for (const auto& su : sg_of_si->list_of_su) {
+ if ((su->saAmfSUAdminState == SA_AMF_ADMIN_UNLOCKED)
&&
+ ((su->su_on_node->node_state ==
AVD_AVND_STATE_NO_CONFIG) ||
+ (su->su_on_node->node_state ==
AVD_AVND_STATE_NCS_INIT)) &&
+ (list_of_sisu->su != su)) {
+ any_nodes_joining = true;
+ break;
+ }
+ }
+ /* Any other node hosting unlocked SUs are joining, so
return TRY AGAIN. */
+ if (any_nodes_joining == true) {
+ LOG_ER("All other nodes hosting SUs are
joining.");
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ }
+
+ /* If the containing nodes have joined: If any one of
the SUs is in
+ instantiating/restarting/terminating state(A case of
Temporary degeneration):
+ return TRY_AGAIN.*/
+ bool any_su_on_nodes_in_trans = false,
any_su_on_nodes_failed = false;
+ for (const auto& su : sg_of_si->list_of_su) {
+ if ((su->su_on_node->node_state ==
AVD_AVND_STATE_PRESENT) &&
+ (list_of_sisu->su != su)) {
+ /* */
+ if ((su->saAmfSUPresenceState ==
SA_AMF_PRESENCE_INSTANTIATING) ||
+
(su->saAmfSUPresenceState == SA_AMF_PRESENCE_TERMINATING) ||
+
(su->saAmfSUPresenceState == SA_AMF_PRESENCE_RESTARTING)) {
+ any_su_on_nodes_in_trans = true;
+ } else if ((su->saAmfSUPresenceState ==
SA_AMF_PRESENCE_INSTANTIATION_FAILED) ||
+
(su->saAmfSUPresenceState == SA_AMF_PRESENCE_TERMINATION_FAILED) ||
+
(su->saAmfSuReadinessState == SA_AMF_READINESS_OUT_OF_SERVICE)) {
+ /* If any one of the SUs is in
Disabled/OutOfService/
+ Instantiation
failed/Termination failed:
+ return BAD OP.*/
+ any_su_on_nodes_failed = true;
+ }
+ }
+ }
+ /* Any other SUs are in
instantiating/restarting/terminating, so return TRY AGAIN. */
+ if (any_su_on_nodes_in_trans == true) {
+ LOG_ER("Other SUs are in
INSTANTIATING/TERMINATING/RESTARTING state.");
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ goto done;
+ } else if (any_su_on_nodes_failed == true) {
+ /* Nothing in inst/rest/term and others are in
failed state, so reutrn BAD OP.*/
+ LOG_ER("Other SUs are in INSTANTIATION/TERMINATION
failed state/ Out of Service.");
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+ }
+ /* Still the SUs may be in Uninstantiated or Instantiated, but
might not have got
+ assignment because of other configurations like
PrefInServiceSUs, etc, so return
+ BAD_OP for them. */
+ LOG_ER("Other SUs are not instantiated or asigned because of
configurations.");
+ rc = SA_AIS_ERR_BAD_OPERATION;
+ goto done;
+ }
+
+ /* If the swap is on m/w si, then check whether any ccb was going on. */
+ if (sg_of_si->sg_ncs_spec) {
+ if (ccbutil_EmptyCcbExists() == false) {
+ rc = SA_AIS_ERR_TRY_AGAIN;
+ LOG_NO("%s SWAP failed - Ccb going on", name.value);
+ goto done;
+ }
+ }
+
+done:
+ return rc;
+}
# HG changeset patch
# User Hans Nordeback <[email protected]>
# Date 1447142501 -3600
# Tue Nov 10 09:01:41 2015 +0100
# Node ID 421fdf3633e224a6c0406ed82117ecdd201706b0
# Parent 561c68aeed193353aec5b1761ef33f84d33f6bc3
[mq]: 1294_review_comments
diff --git a/osaf/services/saf/amf/amfd/include/sg.h
b/osaf/services/saf/amf/amfd/include/sg.h
--- a/osaf/services/saf/amf/amfd/include/sg.h
+++ b/osaf/services/saf/amf/amfd/include/sg.h
@@ -417,6 +417,8 @@ public:
bool ng_using_saAmfSGAdminState;
uint32_t term_su_list_in_reverse();
+
+ bool is_middleware() const {return sg_ncs_spec ? true : false;}
private:
// disallow copy and assign, TODO(hafe) add common macro for this
AVD_SG(const AVD_SG&);
diff --git a/osaf/services/saf/amf/amfd/include/si.h
b/osaf/services/saf/amf/amfd/include/si.h
--- a/osaf/services/saf/amf/amfd/include/si.h
+++ b/osaf/services/saf/amf/amfd/include/si.h
@@ -143,6 +143,7 @@ public:
bool si_dep_states_check();
SaAisErrorT si_swap_validate();
private:
+ bool is_assigned() const {return list_of_sisu ? true : false;}
AVD_SI(const AVD_SI&);
AVD_SI& operator=(const AVD_SI&);
diff --git a/osaf/services/saf/amf/amfd/si.cc b/osaf/services/saf/amf/amfd/si.cc
--- a/osaf/services/saf/amf/amfd/si.cc
+++ b/osaf/services/saf/amf/amfd/si.cc
@@ -1530,7 +1530,7 @@ SaAisErrorT AVD_SI::si_swap_validate()
goto done;
}
- if (list_of_sisu == NULL) {
+ if (is_assigned() == false) {
LOG_ER("%s SWAP failed - no assignments to swap", name.value);
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
@@ -1539,10 +1539,10 @@ SaAisErrorT AVD_SI::si_swap_validate()
/* First check for another node for M/w. */
/* Since middleware components can still have si->list_of_sisu->si_next
as not NULL, but we need to check
whether it is unlocked. We need to reject si_swap on controllers
when stdby controller is locked. */
- if (sg_of_si->sg_ncs_spec) {
+ if (sg_of_si->is_middleware() == true) {
/* Check if the Standby is there in unlocked state. */
node = avd_node_find_nodeid(avd_cb->node_id_avd_other);
- if (node == NULL) {
+ if (node == nullptr) {
LOG_NO("SI Swap not possible, node %x is not
available", avd_cb->node_id_avd_other);
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
@@ -1552,10 +1552,9 @@ SaAisErrorT AVD_SI::si_swap_validate()
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
}
- }
- /* Check whether Amfd is in sync if node is illigible to join. */
- if (sg_of_si->sg_ncs_spec) {
+ /* Check whether Amfd is in sync if node is illigible to join.
*/
+
if (avd_cb->stby_sync_state == AVD_STBY_OUT_OF_SYNC) {
LOG_ER("%s SWAP failed - Cold sync in progress",
name.value);
rc = SA_AIS_ERR_TRY_AGAIN;
@@ -1563,9 +1562,9 @@ SaAisErrorT AVD_SI::si_swap_validate()
}
}
- if (list_of_sisu->si_next == NULL) {
+ if (list_of_sisu->si_next == nullptr) {
LOG_ER("%s SWAP failed - only one assignment", name.value);
- if (sg_of_si->sg_ncs_spec) {
+ if (sg_of_si->is_middleware() == true) {
/* Another M/w SU will come anyway because of two
controller,
so wait to come back.*/
TRACE("M/w SI");
@@ -1575,7 +1574,7 @@ SaAisErrorT AVD_SI::si_swap_validate()
/* Check whether one assignment is because of configuration. */
if (sg_of_si->list_of_su.size() == 1) {
- LOG_ER("Only once SU configured");
+ LOG_ER("Only one SU configured");
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
} else {
@@ -1674,13 +1673,13 @@ SaAisErrorT AVD_SI::si_swap_validate()
/* Still the SUs may be in Uninstantiated or Instantiated, but
might not have got
assignment because of other configurations like
PrefInServiceSUs, etc, so return
BAD_OP for them. */
- LOG_ER("Other SUs are not instantiated or asigned because of
configurations.");
+ LOG_ER("Other SUs are not instantiated or assigned because of
configurations.");
rc = SA_AIS_ERR_BAD_OPERATION;
goto done;
}
/* If the swap is on m/w si, then check whether any ccb was going on. */
- if (sg_of_si->sg_ncs_spec) {
+ if (sg_of_si->is_middleware() == true) {
if (ccbutil_EmptyCcbExists() == false) {
rc = SA_AIS_ERR_TRY_AGAIN;
LOG_NO("%s SWAP failed - Ccb going on", name.value);
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
