Menu
▾
▴
Re: [devel] [PATCH 1 of 1] amfd: allow admin commands before cluster timer expiry [#620]
|
From: Hans F. <han...@er...> - 2014-07-04 11:40:49
|
> -----Original Message-----
> From: Nagendra Kumar [mailto:nag...@or...]
> Sent: den 4 juli 2014 13:44
> To: Hans Feldt; Hans Nordebäck; Praveen Malviya
> Cc: ope...@li...
> Subject: RE: [PATCH 1 of 1] amfd: allow admin commands before cluster timer expiry [#620]
>
> Thanks for your review. I will incorporate these changes. Is this an Ack then?
[Hans] Yes
>
> Thanks
> -Nagu
>
> > -----Original Message-----
> > From: Hans Feldt [mailto:han...@er...]
> > Sent: 04 July 2014 16:47
> > To: Nagendra Kumar; Hans Nordebäck; Praveen Malviya
> > Cc: ope...@li...
> > Subject: RE: [PATCH 1 of 1] amfd: allow admin commands before cluster timer
> > expiry [#620]
> >
> > Just review, see inline
> > /Hans
> >
> > > -----Original Message-----
> > > From: nag...@or... [mailto:nag...@or...]
> > > Sent: den 30 juni 2014 12:48
> > > To: Hans Feldt; Hans Nordebäck; pra...@or...
> > > Cc: ope...@li...
> > > Subject: [PATCH 1 of 1] amfd: allow admin commands before cluster timer
> > expiry [#620]
> > >
> > > osaf/services/saf/amf/amfd/imm.cc | 9 ++++-
> > > osaf/services/saf/amf/amfd/include/util.h | 1 +
> > > osaf/services/saf/amf/amfd/node.cc | 26 +++++++++++---
> > > osaf/services/saf/amf/amfd/sg.cc | 16 +++++++++
> > > osaf/services/saf/amf/amfd/si.cc | 10 +++++-
> > > osaf/services/saf/amf/amfd/su.cc | 23 +++++++++----
> > > osaf/services/saf/amf/amfd/util.cc | 52
> > +++++++++++++++++++++++++++++++
> > > 7 files changed, 122 insertions(+), 15 deletions(-)
> > >
> > >
> > > SA_AMF_ADMIN_UNLOCK, SA_AMF_ADMIN_LOCK,
> > SA_AMF_ADMIN_UNLOCK_INSTANTIATION
> > > and SA_AMF_ADMIN_REPAIRED can be issued on amf entities before cluster
> > > timer expiry. The other commands will be returned as try_again
> > > during the period.
> > >
> > > diff --git a/osaf/services/saf/amf/amfd/imm.cc
> > b/osaf/services/saf/amf/amfd/imm.cc
> > > --- a/osaf/services/saf/amf/amfd/imm.cc
> > > +++ b/osaf/services/saf/amf/amfd/imm.cc
> > > @@ -666,7 +666,14 @@ static void admin_operation_cb(SaImmOiHa
> > > admin_op_name(static_cast<SaAmfAdminOperationIdT>(op_id)),
> > object_name->value, invocation);
> > >
> > > if (admin_op_callback[type] != NULL) {
> > > - admin_op_callback[type](immoi_handle, invocation,
> > object_name, op_id, params);
> > > + if (false == is_admin_op_valid(op_id, type)) {
> > [Hans] please revert this statement to: "is_admin_op_valid(op_id, type) ==
> > false"
> >
> > > + report_admin_op_error(immoi_handle, invocation,
> > SA_AIS_ERR_TRY_AGAIN, NULL,
> > > + "AMF (state %u) is not available for
> > admin op'%llu' on '%s'",
> > > + avd_cb->init_state, op_id,
> > object_name->value);
> > > + goto done;
> > > + } else {
> > > + admin_op_callback[type](immoi_handle, invocation,
> > object_name, op_id, params);
> > > + }
> > > } else {
> > > LOG_ER("Admin operation not supported for %s (%u)",
> > object_name->value, type);
> > > report_admin_op_error(immoi_handle, invocation,
> > SA_AIS_ERR_INVALID_PARAM, NULL,
> > > diff --git a/osaf/services/saf/amf/amfd/include/util.h
> > b/osaf/services/saf/amf/amfd/include/util.h
> > > --- a/osaf/services/saf/amf/amfd/include/util.h
> > > +++ b/osaf/services/saf/amf/amfd/include/util.h
> > > @@ -96,6 +96,7 @@ struct avd_comp_csi_rel_tag;
> > > struct avd_csi_tag;
> > >
> > > void avd_d2n_reboot_snd(struct avd_avnd_tag *node);
> > > +bool is_admin_op_valid(SaImmAdminOperationIdT opId,
> > AVSV_AMF_CLASS_ID class_id);
> > > void amflog(int priority, const char *format, ...);
> > > void d2n_msg_free(AVD_DND_MSG *msg);
> > > uint32_t avd_d2n_msg_dequeue(struct cl_cb_tag *cb);
> > > diff --git a/osaf/services/saf/amf/amfd/node.cc
> > b/osaf/services/saf/amf/amfd/node.cc
> > > --- a/osaf/services/saf/amf/amfd/node.cc
> > > +++ b/osaf/services/saf/amf/amfd/node.cc
> > > @@ -1041,12 +1041,6 @@ static void node_admin_op_cb(SaImmOiHand
> > >
> > > TRACE_ENTER2("%llu, '%s', %llu", invocation, objectName->value,
> > operationId);
> > >
> > > - if (avd_cb->init_state != AVD_APP_STATE) {
> > > - report_admin_op_error(immOiHandle, invocation,
> > SA_AIS_ERR_TRY_AGAIN, NULL,
> > > - "AVD not in APP_STATE");
> > > - goto done;
> > > - }
> > > -
> > > node = avd_node_get(objectName);
> > > osafassert(node != AVD_AVND_NULL);
> > >
> > > @@ -1136,6 +1130,17 @@ static void node_admin_op_cb(SaImmOiHand
> > > goto done;
> > > }
> > >
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + node_admin_state_set(node,
> > SA_AMF_ADMIN_UNLOCKED);
> > > + for(su = node->list_of_su; su != NULL; su = su-
> > >avnd_list_su_next) {
> > > + if (su->is_in_service() == true) {
> > > + su-
> > >set_readiness_state(SA_AMF_READINESS_IN_SERVICE);
> > > + }
> > > + }
> > > + avd_saImmOiAdminOperationResult(immOiHandle,
> > invocation, SA_AIS_OK);
> > > + break;
> > > + }
> > > +
> > > avd_node_admin_lock_unlock_shutdown(node, invocation,
> > static_cast<SaAmfAdminOperationIdT>(operationId));
> > > break;
> > >
> > > @@ -1160,6 +1165,15 @@ static void node_admin_op_cb(SaImmOiHand
> > > goto done;
> > > }
> > >
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > [Hans] white space indent problem?
> >
> > > + node_admin_state_set(node, SA_AMF_ADMIN_LOCKED);
> > > + for(su = node->list_of_su; su != NULL; su = su-
> > >avnd_list_su_next) {
> > > + su-
> > >set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE);
> > > + }
> > > + avd_saImmOiAdminOperationResult(immOiHandle,
> > invocation, SA_AIS_OK);
> > > + break;
> > > + }
> > > +
> > > avd_node_admin_lock_unlock_shutdown(node, invocation,
> > static_cast<SaAmfAdminOperationIdT>(operationId));
> > > break;
> > >
> > > diff --git a/osaf/services/saf/amf/amfd/sg.cc
> > b/osaf/services/saf/amf/amfd/sg.cc
> > > --- a/osaf/services/saf/amf/amfd/sg.cc
> > > +++ b/osaf/services/saf/amf/amfd/sg.cc
> > > @@ -1234,6 +1234,14 @@ static void sg_admin_op_cb(SaImmOiHandle
> > >
> > > adm_state = sg->saAmfSGAdminState;
> > > avd_sg_admin_state_set(sg, SA_AMF_ADMIN_UNLOCKED);
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + for (su = sg->list_of_su; su != NULL; su = su-
> > >sg_list_su_next) {
> > > + if (su->is_in_service() == true) {
> > > + su-
> > >set_readiness_state(SA_AMF_READINESS_IN_SERVICE);
> > > + }
> > > + }
> > > + break;
> > > + }
> > > if (avd_sg_app_sg_admin_func(avd_cb, sg) !=
> > NCSCC_RC_SUCCESS) {
> > > avd_sg_admin_state_set(sg, adm_state);
> > > report_admin_op_error(immOiHandle, invocation,
> > SA_AIS_ERR_BAD_OPERATION, NULL,
> > > @@ -1257,6 +1265,14 @@ static void sg_admin_op_cb(SaImmOiHandle
> > >
> > > adm_state = sg->saAmfSGAdminState;
> > > avd_sg_admin_state_set(sg, SA_AMF_ADMIN_LOCKED);
> > > +
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + for (su = sg->list_of_su; su != NULL; su = su-
> > >sg_list_su_next) {
> > > + su-
> > >set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE);
> > > + }
> > > + break;
> > > + }
> > > +
> > > if (avd_sg_app_sg_admin_func(avd_cb, sg) !=
> > NCSCC_RC_SUCCESS) {
> > > avd_sg_admin_state_set(sg, adm_state);
> > > report_admin_op_error(immOiHandle, invocation,
> > SA_AIS_ERR_BAD_OPERATION, NULL,
> > > 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
> > > @@ -809,6 +809,14 @@ static void si_admin_op_cb(SaImmOiHandle
> > >
> > > si->set_admin_state(SA_AMF_ADMIN_UNLOCKED);
> > >
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + /* Mark the admin state to unlocked, assignments will
> > be
> > > + delivered once cluster timer expires. */
> > [Hans] there is no "mark" of admin state here...
> >
> > > + rc = SA_AIS_OK;
> > > + avd_saImmOiAdminOperationResult(immOiHandle,
> > invocation, rc);
> > > + goto done;
> > > + }
> > > +
> > > err = si->sg_of_si->si_assign(avd_cb, si);
> > > if (si->list_of_sisu == NULL)
> > > LOG_NO("'%s' could not be assigned to any SU", si-
> > >name.value);
> > > @@ -849,7 +857,7 @@ static void si_admin_op_cb(SaImmOiHandle
> > > goto done;
> > > }
> > >
> > > - if (si->list_of_sisu == AVD_SU_SI_REL_NULL) {
> > > + if ((si->list_of_sisu == AVD_SU_SI_REL_NULL) || (avd_cb-
> > >init_state == AVD_INIT_DONE)) {
> > > si->set_admin_state(SA_AMF_ADMIN_LOCKED);
> > > /* This may happen when SUs are locked before SI is
> > locked. */
> > > LOG_WA("SI lock of %s, has no assignments",
> > objectName->value);
> > > diff --git a/osaf/services/saf/amf/amfd/su.cc
> > b/osaf/services/saf/amf/amfd/su.cc
> > > --- a/osaf/services/saf/amf/amfd/su.cc
> > > +++ b/osaf/services/saf/amf/amfd/su.cc
> > > @@ -840,6 +840,14 @@ void AVD_SU::unlock(SaImmOiHandleT immoi
> > > TRACE_ENTER2("'%s'", name.value);
> > > set_admin_state(SA_AMF_ADMIN_UNLOCKED);
> > >
> > > + /* Mark the admin state to unlock and return as cluster timer haven't
> > expired.*/
> > [Hans] [Hans] there is no "mark" of admin state here...
> >
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + if (is_in_service() == true)
> > > +
> > set_readiness_state(SA_AMF_READINESS_IN_SERVICE);
> > > + avd_saImmOiAdminOperationResult(immoi_handle,
> > invocation, SA_AIS_OK);
> > > + goto done;
> > > + }
> > > +
> > > if ((is_in_service() == true) || (sg_of_su->sg_ncs_spec == true)) {
> > > /* Reason for checking for MW component is that node oper
> > state and
> > > * SU oper state are marked enabled after they gets
> > assignments.
> > > @@ -866,7 +874,7 @@ void AVD_SU::unlock(SaImmOiHandleT immoi
> > > report_admin_op_error(immoi_handle, invocation,
> > SA_AIS_ERR_FAILED_OPERATION, NULL,
> > > "SG redundancy model specific handler
> > failed");
> > > }
> > > -
> > > +done:
> > > TRACE_LEAVE();
> > > }
> > >
> > > @@ -878,6 +886,13 @@ void AVD_SU::lock(SaImmOiHandleT immoi_h
> > > bool is_oper_successful = true;
> > >
> > > TRACE_ENTER2("'%s'", name.value);
> > > + /* Mark the admin state to lock and return as cluster timer haven't
> > expired.*/
> > [Hans] mark->change ?
> >
> > > + if (avd_cb->init_state == AVD_INIT_DONE) {
> > > + set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE);
> > > + set_admin_state(SA_AMF_ADMIN_LOCKED);
> > > + avd_saImmOiAdminOperationResult(immoi_handle,
> > invocation, SA_AIS_OK);
> > > + goto done;
> > > + }
> > >
> > > if (list_of_susi == NULL) {
> > > set_readiness_state(SA_AMF_READINESS_OUT_OF_SERVICE);
> > > @@ -1098,12 +1113,6 @@ static void su_admin_op_cb(SaImmOiHandle
> > > goto done;
> > > }
> > >
> > > - if (cb->init_state != AVD_APP_STATE ) {
> > > - report_admin_op_error(immoi_handle, invocation,
> > SA_AIS_ERR_TRY_AGAIN, NULL,
> > > - "AMF (state %u) is not available for admin
> > ops", cb->init_state);
> > > - goto done;
> > > - }
> > > -
> > > if (NULL == (su = su_db->find(Amf::to_string(su_name)))) {
> > > LOG_CR("SU '%s' not found", su_name->value);
> > > /* internal error? osafassert instead? */
> > > diff --git a/osaf/services/saf/amf/amfd/util.cc
> > b/osaf/services/saf/amf/amfd/util.cc
> > > --- a/osaf/services/saf/amf/amfd/util.cc
> > > +++ b/osaf/services/saf/amf/amfd/util.cc
> > > @@ -1704,3 +1704,55 @@ void amflog(int priority, const char *fo
> > > saflog(priority, amfSvcUsrName, "%s", str);
> > > }
> > > }
> > > +
> > > +/**
> > > + * Validates whether the initial state is apt to process the admin command.
> > [Hans] "initial state", what is that?
> >
> > > + * @param: admin operation id.
> > > + * @param: class id related to objects on which admin command has been
> > issued.
> > > + */
> > > +bool is_admin_op_valid(SaImmAdminOperationIdT opId,
> > AVSV_AMF_CLASS_ID class_id)
> > [Hans] admin_op_is_valid makes code using this function more readable
> >
> > > +{
> > > + bool valid = false;
> > > + TRACE_ENTER2("%llu, %u", opId, class_id);
> > > +
> > > + if (avd_cb->init_state == AVD_APP_STATE)
> > > + return true;
> > > +
> > > + switch (class_id) {
> > > + case AVSV_SA_AMF_SU:
> > > + {
> > [Hans] wrong placement of brace not even needed, same below
> > > + /* Support for admin op on su before cluster
> > timer expires.*/
> > > + if ((avd_cb->init_state == AVD_INIT_DONE)
> > &&
> > > + ((opId ==
> > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK)
> > > + || (opId ==
> > SA_AMF_ADMIN_REPAIRED) ||
> > [Hans] please put logical or at the end of a line
> > > + (opId ==
> > SA_AMF_ADMIN_UNLOCK_INSTANTIATION)))
> > > + valid = true;
> > > + }
> > > + break;
> > > + case AVSV_SA_AMF_NODE:
> > > + case AVSV_SA_AMF_SG:
> > > + {
> > > + /* Support for admin op on node/sg before
> > cluster timer expires.*/
> > > + if ((avd_cb->init_state == AVD_INIT_DONE)
> > &&
> > > + ((opId ==
> > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK)
> > > + || (opId ==
> > SA_AMF_ADMIN_UNLOCK_INSTANTIATION)))
> > [Hans] please put logical or at the end of a line
> >
> > > + valid = true;
> > > + }
> > > + break;
> > > +
> > > + case AVSV_SA_AMF_SI:
> > > + {
> > > + /* Support for admin op on si before cluster
> > timer expires. */
> > > + if ((avd_cb->init_state == AVD_INIT_DONE)
> > &&
> > > + ((opId ==
> > SA_AMF_ADMIN_LOCK) || (opId == SA_AMF_ADMIN_UNLOCK)))
> > > + valid = true;
> > > + }
> > > + break;
> > > + case AVSV_SA_AMF_COMP:
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + return valid;
> > > +}
> > > +
|