subject:"Re\: \[devel\] \[PATCH 1 of 1\] smf\: Recreate IMM handles if bad handle when deleting node group \[#2046\]"

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-29 Thread Rafael Odzakow

ACK


On 09/29/2016 02:45 PM, Lennart Lund wrote:
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  93 
> ++---
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.hh |   3 +-
>   2 files changed, 67 insertions(+), 29 deletions(-)
>
>
> Fix the deleteNodeGroup() method must be so that if the delete operation fails
> with bad handles new handles are created so that the node group can be deleted
>
> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc 
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> @@ -2823,7 +2823,7 @@ SmfAdminOperation::SmfAdminOperation(std
>   // and an immediate return is done
>   // The public methods shall return fail if m_creation_fail is true
>   
> - bool rc = getAllImmHandles();
> + bool rc = initAllImmHandles();
>   if (rc == false) {
>   LOG_NO("%s: getAllImmHandles Fail", __FUNCTION__);
>   m_creation_fail = true;
> @@ -2860,11 +2860,7 @@ SmfAdminOperation::SmfAdminOperation(std
>   
>   SmfAdminOperation::~SmfAdminOperation()
>   {
> - // This frees all IMM handles that's based on the omHandle and
> - // give up admin ownership of Amf Cluster object
> - if (m_omHandle != 0) {
> - (void)immutil_saImmOmFinalize(m_omHandle);
> - }
> +finalizeAllImmHandles();
>   }
>   
>   ///
> @@ -3079,7 +3075,7 @@ bool SmfAdminOperation::restart()
>   /// Get all needed IMM handles. Updates corresponding member variables
>   /// Return false if fail
>   ///
> -bool SmfAdminOperation::getAllImmHandles()
> +bool SmfAdminOperation::initAllImmHandles()
>   {
>   SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
>   int timeout_try_cnt = 6;
> @@ -3156,6 +3152,15 @@ bool SmfAdminOperation::getAllImmHandles
>   return rc;
>   }
>   
> +// Free all IMM handles that's based on the omHandle and
> +// give up admin ownership of Amf Cluster object
> +void SmfAdminOperation::finalizeAllImmHandles() {
> + if (m_omHandle != 0) {
> + (void)immutil_saImmOmFinalize(m_omHandle);
> + }
> +
> +}
> +
>   /// Check if the AIS return is considered a campaign error
>   /// Do not Fail if any of the following AIS codes
>   ///
> @@ -3552,6 +3557,8 @@ bool SmfAdminOperation::createNodeGroup(
>   // A node group with the same name already exist
>   // May happen if a previous delete after usage has
>   // failed
> +LOG_NO("%s: saImmOmCcbObjectCreate_2 Fail %s",
> +   __FUNCTION__, saf_error(m_errno));
>   bool rc = deleteNodeGroup();
>   if (rc == false) {
>   LOG_NO("%s: deleteNodeGroup() Fail",
> @@ -3591,8 +3598,7 @@ bool SmfAdminOperation::createNodeGroup(
>   ///
>   bool SmfAdminOperation::deleteNodeGroup()
>   {
> - bool rc = true;
> - m_errno = SA_AIS_OK;
> + SaAisErrorT ais_rc = SA_AIS_OK;
>   
>   TRACE_ENTER();
>   std::string nodeGroupName_s =
> @@ -3602,25 +3608,53 @@ bool SmfAdminOperation::deleteNodeGroup(
>   );
>   
>   TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> -
> - m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> - );
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> -  __FUNCTION__, nodeGroupName_s.c_str(),
> -  saf_error(m_errno));
> - rc = false;
> - } else {
> - m_errno = saImmOmCcbApply(m_ccbHandle);
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> - __FUNCTION__, saf_error(m_errno));
> - rc = false;
> - }
> - }
> -
> - TRACE_LEAVE();
> - return rc;
> + bool method_rc = false;
> +const uint32_t MAX_NO_RETRIES = 2;
> +uint32_t retry_cnt = 0;
> +while (++retry_cnt <= MAX_NO_RETRIES) {
> +// Handles including ccb handle may have been invalidated by
> +// IMM resulting in SA_AIS_ERR_BAD_HANDLE response on the
> +// delete object request.
> +// If that's the case: Try to create new handles and try 
> again
> +ais_rc = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> +);
> +TRACE("%s: immutil_saImmOmCcbObjectDelete %s",
> +__FUNCTION__, saf_error(m_errno));
> +
> +if (ais_rc == SA_AIS_ERR_BAD_HANDLE) {
> +LOG_NO("%s: saImmOmCcbObjectDelete Fail %s",
> +   __FUNCTION__, saf_error(m_errno));
> +finalizeAllImmHandles();
> +bool rc = initAllImmHandles();
> +if

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-27 Thread Neelakanta Reddy

Hi Lennart,

comments inline.

On 2016/09/27 04:58 PM, Lennart Lund wrote:
> Hi Neel,
>
> See my comments/answers inline [Lennart]
>
> Thanks
> Lennart
>
>> -Original Message-
>> From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com]
>> Sent: den 27 september 2016 11:04
>> To: Lennart Lund ; Rafael Odzakow
>> 
>> Cc: opensaf-devel@lists.sourceforge.net
>> Subject: Re: [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when
>> deleting node group [#2046]
>>
>> Hi Lennart,
>>
>> Reviewed the patch, I have to NACK the patch because of the comments.
>>
>> Following are the comments:
>>
>> 1. The retries can not be infinite, while(1) has to be changed to some
>> finite number of retries.
> [Lennart] Since immutil_saImmOmCcbObjectDelete() may have to be called more 
> than once I have chosen to use a loop.
> The loop is not infinite but the loop is not ended because of a loop counter 
> and the exit condition is not tested within the while parenthesis. The loop 
> continues only if immutil_saImmOmCcbObjectDelete returns 
> SA_AIS_ERR_BAD_HANDLE. Before the loop continues new handles are requested 
> but if this fails the loop will end.
> The loop will be infinite only if it is possible to create handles (no Fail) 
> and saImmOmCcbObjectDelete always returns SA_AIS_ERR_BAD_HANDLE even if new 
> handles just have been successfully created. If you think this could happen I 
> can insert a loop counter to end the loop in case?
[Neel]
BAD_HANDLE will not be returned always.  But the loop should not be 
infinite.
>> 2.  make saImmOiFinalize as part of getAllImmHandles. (for more
>> information look into avd_imm_reinit_bg_thread in
>> osaf/services/saf/amf/amfd/imm.cc)
> [Lennart] This is not an OI it's an object manager. In this case 
> immutil_saImmOmFinalize() is called before the attempt to create new handles. 
> However it is not done in the getAllImmHandles() function. This function is 
> also used to create the handles in the constructor and if this fails none of 
> the admin operations will be executed (campaign will fail)
[Neel]
you can guard this with immhandle, if it is zero or NULL do not 
finalize, this is to generalize and not a mandatory change.


>
>> 3.  for each retry there must be sleep.
> [Lennart] Note that this is not a loop for handling SA_AIS_ERR_TRY_AGAIN this 
> is handled within the immutil_xxx functions. A sleep is not needed here since 
> there is no attempt made to call immutil_saImmOmCcbObjectDelete() again until 
> after both OM Finalize and creation of new handles is done
[Neel]
even though it is not TRY_AGAIN, for each retry there must be a sleep,  
for any type of error, because this will  give system/openSAF to do some 
initializations, instead of returning the same error again..
This has been done in AMF (AMF is well tested with BAD_HANDLE from IMM 
perspective):

  if (rc == SA_AIS_ERR_BAD_HANDLE) {
 osaf_nanosleep();


Thanks,
Neel.
>> Regards,
>> Neel.
>>
>>
>> On 2016/09/22 07:03 PM, Lennart Lund wrote:
>>>osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49
>> +
>>>1 files changed, 33 insertions(+), 16 deletions(-)
>>>
>>>
>>> Fix the deleteNodeGroup() method must be so that if the delete operation
>> fails
>>> with bad handles new handles are created so that the node group can be
>> deleted
>>> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>>> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>>> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>>> @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
>>> );
>>>
>>> TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
>>> -
>>> -   m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
>>> -   );
>>> -   if (m_errno != SA_AIS_OK) {
>>> -   LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
>>> -__FUNCTION__, nodeGroupName_s.c_str(),
>>> -saf_error(m_errno));
>>> -   rc = false;
>>> -   } else {
>>> -   m_errno = saImmOmCcbApply(m_ccbHandle);
>>> -   if (m_errno != SA_AIS_OK) {
>>> -   LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
>>> -   __FUNCTION__, saf_error(m_errno));
>>> -   rc = false;
>>> -   }
>>> -   }
>>> +while (true) {
>>> +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
>>> +);
>>> +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
>>> +// Handles may have been invalidated/timed out
>>> +(void)immutil_saImmOmFinalize(m_omHandle);
>>> +bool rc = getAllImmHandles();
>>> +if (rc == false) {
>>> +LOG_NO("%s: getAllImmHandles Fail",
>>> +   __FUNCTION__);
>>> +

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-27 Thread Lennart Lund

Hi Neel,

See my comments/answers inline [Lennart]

Thanks
Lennart

> -Original Message-
> From: Neelakanta Reddy [mailto:reddy.neelaka...@oracle.com]
> Sent: den 27 september 2016 11:04
> To: Lennart Lund ; Rafael Odzakow
> 
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when
> deleting node group [#2046]
> 
> Hi Lennart,
> 
> Reviewed the patch, I have to NACK the patch because of the comments.
> 
> Following are the comments:
> 
> 1. The retries can not be infinite, while(1) has to be changed to some
> finite number of retries.
[Lennart] Since immutil_saImmOmCcbObjectDelete() may have to be called more 
than once I have chosen to use a loop.
The loop is not infinite but the loop is not ended because of a loop counter 
and the exit condition is not tested within the while parenthesis. The loop 
continues only if immutil_saImmOmCcbObjectDelete returns SA_AIS_ERR_BAD_HANDLE. 
Before the loop continues new handles are requested but if this fails the loop 
will end.
The loop will be infinite only if it is possible to create handles (no Fail) 
and saImmOmCcbObjectDelete always returns SA_AIS_ERR_BAD_HANDLE even if new 
handles just have been successfully created. If you think this could happen I 
can insert a loop counter to end the loop in case?

> 2.  make saImmOiFinalize as part of getAllImmHandles. (for more
> information look into avd_imm_reinit_bg_thread in
> osaf/services/saf/amf/amfd/imm.cc)
[Lennart] This is not an OI it's an object manager. In this case 
immutil_saImmOmFinalize() is called before the attempt to create new handles. 
However it is not done in the getAllImmHandles() function. This function is 
also used to create the handles in the constructor and if this fails none of 
the admin operations will be executed (campaign will fail)

> 3.  for each retry there must be sleep.
[Lennart] Note that this is not a loop for handling SA_AIS_ERR_TRY_AGAIN this 
is handled within the immutil_xxx functions. A sleep is not needed here since 
there is no attempt made to call immutil_saImmOmCcbObjectDelete() again until 
after both OM Finalize and creation of new handles is done
> 
> Regards,
> Neel.
> 
> 
> On 2016/09/22 07:03 PM, Lennart Lund wrote:
> >   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49
> +
> >   1 files changed, 33 insertions(+), 16 deletions(-)
> >
> >
> > Fix the deleteNodeGroup() method must be so that if the delete operation
> fails
> > with bad handles new handles are created so that the node group can be
> deleted
> >
> > diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
> > );
> >
> > TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> > -
> > -   m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> > -   );
> > -   if (m_errno != SA_AIS_OK) {
> > -   LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> > -__FUNCTION__, nodeGroupName_s.c_str(),
> > -saf_error(m_errno));
> > -   rc = false;
> > -   } else {
> > -   m_errno = saImmOmCcbApply(m_ccbHandle);
> > -   if (m_errno != SA_AIS_OK) {
> > -   LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> > -   __FUNCTION__, saf_error(m_errno));
> > -   rc = false;
> > -   }
> > -   }
> > +while (true) {
> > +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> > +);
> > +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
> > +// Handles may have been invalidated/timed out
> > +(void)immutil_saImmOmFinalize(m_omHandle);
> > +bool rc = getAllImmHandles();
> > +if (rc == false) {
> > +LOG_NO("%s: getAllImmHandles Fail",
> > +   __FUNCTION__);
> > +break;
> > +}
> > +continue;
> > +}
> > +if (m_errno != SA_AIS_OK) {
> > +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> > +  __FUNCTION__, nodeGroupName_s.c_str(),
> > +  saf_error(m_errno));
> > +  rc = false;
> > +  break;
> > +} else {
> > +m_errno = saImmOmCcbApply(m_ccbHandle);
> > +if (m_errno != SA_AIS_OK) {
> > +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> > +  __FUNCTION__, saf_error(m_errno));
> > +

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-27 Thread Neelakanta Reddy

Hi Lennart,

Reviewed the patch, I have to NACK the patch because of the comments.

Following are the comments:

1. The retries can not be infinite, while(1) has to be changed to some 
finite number of retries.
2.  make saImmOiFinalize as part of getAllImmHandles. (for more 
information look into avd_imm_reinit_bg_thread in 
osaf/services/saf/amf/amfd/imm.cc)
3.  for each retry there must be sleep.

Regards,
Neel.


On 2016/09/22 07:03 PM, Lennart Lund wrote:
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49 
> +
>   1 files changed, 33 insertions(+), 16 deletions(-)
>
>
> Fix the deleteNodeGroup() method must be so that if the delete operation fails
> with bad handles new handles are created so that the node group can be deleted
>
> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc 
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
>   );
>   
>   TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> -
> - m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> - );
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> -  __FUNCTION__, nodeGroupName_s.c_str(),
> -  saf_error(m_errno));
> - rc = false;
> - } else {
> - m_errno = saImmOmCcbApply(m_ccbHandle);
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> - __FUNCTION__, saf_error(m_errno));
> - rc = false;
> - }
> - }
> +while (true) {
> +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> +);
> +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
> +// Handles may have been invalidated/timed out
> +(void)immutil_saImmOmFinalize(m_omHandle);
> +bool rc = getAllImmHandles();
> +if (rc == false) {
> +LOG_NO("%s: getAllImmHandles Fail",
> +   __FUNCTION__);
> +break;
> +}
> +continue;
> +}
> +if (m_errno != SA_AIS_OK) {
> +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> +  __FUNCTION__, nodeGroupName_s.c_str(),
> +  saf_error(m_errno));
> +  rc = false;
> +  break;
> +} else {
> +m_errno = saImmOmCcbApply(m_ccbHandle);
> +if (m_errno != SA_AIS_OK) {
> +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> +  __FUNCTION__, saf_error(m_errno));
> +  rc = false;
> +}
> +break;
> +}
> +}
>   
>   TRACE_LEAVE();
>   return rc;
> @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp
>   , 0, adminOp, params,
>   _rc, smfd_cb->adminOpTimeout);
>   
> +if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK)
> +break;
> +
>   if (retry <= 0) {
>   LOG_NO("Fail to invoke admin operation, too many OI "
>   "TRY_AGAIN, giving up. %s", __FUNCTION__);


--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-23 Thread Rafael Odzakow

ACK with previous comments


On 09/23/2016 10:48 AM, Lennart Lund wrote:
> My own comments:
>
> 1)
> Do not set m_errno in object delete method. Not used, not needed. Instead use 
> internal ais_rc where needed
>
> See also [Lennart] inline
>
> /Lennart
>
>> -Original Message-
>> From: Lennart Lund [mailto:lennart.l...@ericsson.com]
>> Sent: den 22 september 2016 15:34
>> To: Rafael Odzakow ;
>> reddy.neelaka...@oracle.com
>> Cc: opensaf-devel@lists.sourceforge.net
>> Subject: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle
>> when deleting node group [#2046]
>>
>>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49
>> +
>>   1 files changed, 33 insertions(+), 16 deletions(-)
>>
>>
>> Fix the deleteNodeGroup() method must be so that if the delete operation
>> fails
>> with bad handles new handles are created so that the node group can be
>> deleted
>>
>> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
>> @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
>>  );
>>
>>  TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
>> -
>> -m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
>> -);
>> -if (m_errno != SA_AIS_OK) {
>> -LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
>> - __FUNCTION__, nodeGroupName_s.c_str(),
>> - saf_error(m_errno));
>> -rc = false;
>> -} else {
>> -m_errno = saImmOmCcbApply(m_ccbHandle);
>> -if (m_errno != SA_AIS_OK) {
>> -LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
>> -__FUNCTION__, saf_error(m_errno));
>> -rc = false;
>> -}
>> -}
>> +while (true) {
>> +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
>> +);
>> +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
>> +// Handles may have been invalidated/timed out
>> +(void)immutil_saImmOmFinalize(m_omHandle);
>> +bool rc = getAllImmHandles();
> [Lennart] Incorrect. Is mixed up with merthod return code. Separate method 
> return code and this internal return code. Set method return code if 
> getAllImmHandles() fail.
>
>> +if (rc == false) {
>> +LOG_NO("%s: getAllImmHandles Fail",
>> +   __FUNCTION__);
>> +break;
>> +}
>> +continue;
>> +}
>> +if (m_errno != SA_AIS_OK) {
>> +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
>> +  __FUNCTION__, nodeGroupName_s.c_str(),
>> +  saf_error(m_errno));
>> +  rc = false;
>> +  break;
>> +} else {
>> +m_errno = saImmOmCcbApply(m_ccbHandle);
>> +if (m_errno != SA_AIS_OK) {
>> +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
>> +  __FUNCTION__, saf_error(m_errno));
>> +  rc = false;
>> +}
>> +break;
>> +}
>> +}
>>
>>  TRACE_LEAVE();
>>  return rc;
>> @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp
>>  , 0, adminOp, params,
>>  _rc, smfd_cb->adminOpTimeout);
>>
>> +if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK)
>> +break;
>> +
>>  if (retry <= 0) {
>>  LOG_NO("Fail to invoke admin operation, too many
>> OI "
>>  "TRY_AGAIN, giving up. %s",
>> __FUNCTION__);
>>
>> --
>> ___
>> Opensaf-devel mailing list
>> Opensaf-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel


--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-23 Thread Lennart Lund

Hi,

See my comment/reply inline [Lennart]

Thanks
Lennart

> -Original Message-
> From: Rafael Odzakow
> Sent: den 23 september 2016 10:34
> To: Lennart Lund ;
> reddy.neelaka...@oracle.com
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when
> deleting node group [#2046]
> 
> comments one comment under [rafael] tag.
> 
> 
> On 09/22/2016 03:33 PM, Lennart Lund wrote:
> >   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49
> +
> >   1 files changed, 33 insertions(+), 16 deletions(-)
> >
> >
> > Fix the deleteNodeGroup() method must be so that if the delete operation
> fails
> > with bad handles new handles are created so that the node group can be
> deleted
> >
> > diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> > @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
> > );
> >
> > TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> > -
> > -   m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> > -   );
> > -   if (m_errno != SA_AIS_OK) {
> > -   LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> > -__FUNCTION__, nodeGroupName_s.c_str(),
> > -saf_error(m_errno));
> > -   rc = false;
> > -   } else {
> > -   m_errno = saImmOmCcbApply(m_ccbHandle);
> > -   if (m_errno != SA_AIS_OK) {
> > -   LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> > -   __FUNCTION__, saf_error(m_errno));
> > -   rc = false;
> > -   }
> > -   }
> > +while (true) {
> [rafael]  The while loop is here to retry CcbObjectDelete only one time.
> A much clearer way is to call CcbObjectDelete twice. As it looks now I
> needed to reread this section before I understood the logic. By trying
> to avoid writing extra code you have complicated a simple function. Call
> it twice instead of having a while loop that will retry after BAD_HANDLE
> endless amounts of times. A possibility for an infinite loop is not a
> good idea. After the second CcbObjectDelete you can fail normally.
> Please explain in comments or remove this while loop.

[Lennart] Unfortunately we use C API for IMM that requires tests of return 
codes and in some cases actions and retries of the request. In order to 
encapsulate this handling and to avoid return and goto statements in several 
places a while loop is used. In this simple function where for the moment 
nothing is done after this handling it would have been possible to use a 
sequence containing a check where CcbObjectDelete() where called a second time 
in case of BAD HANDLE error. I have chosen to keep the original solution but 
has done some minor changes that hopefully will make the code easier to 
understand. I have also written some comments explaining why a retry mechanism 
is needed

> > +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> > +);
> > +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
> > +// Handles may have been invalidated/timed out
> > +(void)immutil_saImmOmFinalize(m_omHandle);
> > +bool rc = getAllImmHandles();
> > +if (rc == false) {
> > +LOG_NO("%s: getAllImmHandles Fail",
> > +   __FUNCTION__);
> > +break;
> > +}
> > +continue;
> > +}
> > +if (m_errno != SA_AIS_OK) {
> > +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> > +  __FUNCTION__, nodeGroupName_s.c_str(),
> > +  saf_error(m_errno));
> > +  rc = false;
> > +  break;
> > +} else {
> > +m_errno = saImmOmCcbApply(m_ccbHandle);
> > +if (m_errno != SA_AIS_OK) {
> > +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> > +  __FUNCTION__, saf_error(m_errno));
> > +  rc = false;
> > +}
> > +break;
> > +}
> > +}
> >
> > TRACE_LEAVE();
> > return rc;
> > @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp
> > , 0, adminOp, params,
> > _rc, smfd_cb->adminOpTimeout);
> >
> > +if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK)
> > +break;
> > +
> > if (retry <= 0) {
> > LOG_NO("Fail to invoke admin operation, too many
> OI "
> >

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-23 Thread Lennart Lund

My own comments:

1)
Do not set m_errno in object delete method. Not used, not needed. Instead use 
internal ais_rc where needed

See also [Lennart] inline

/Lennart

> -Original Message-
> From: Lennart Lund [mailto:lennart.l...@ericsson.com]
> Sent: den 22 september 2016 15:34
> To: Rafael Odzakow ;
> reddy.neelaka...@oracle.com
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle
> when deleting node group [#2046]
> 
>  osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49
> +
>  1 files changed, 33 insertions(+), 16 deletions(-)
> 
> 
> Fix the deleteNodeGroup() method must be so that if the delete operation
> fails
> with bad handles new handles are created so that the node group can be
> deleted
> 
> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
>   );
> 
>   TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> -
> - m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> - );
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> -  __FUNCTION__, nodeGroupName_s.c_str(),
> -  saf_error(m_errno));
> - rc = false;
> - } else {
> - m_errno = saImmOmCcbApply(m_ccbHandle);
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> - __FUNCTION__, saf_error(m_errno));
> - rc = false;
> - }
> - }
> +while (true) {
> +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> +);
> +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
> +// Handles may have been invalidated/timed out
> +(void)immutil_saImmOmFinalize(m_omHandle);
> +bool rc = getAllImmHandles();
[Lennart] Incorrect. Is mixed up with merthod return code. Separate method 
return code and this internal return code. Set method return code if 
getAllImmHandles() fail.

> +if (rc == false) {
> +LOG_NO("%s: getAllImmHandles Fail",
> +   __FUNCTION__);
> +break;
> +}
> +continue;
> +}
> +if (m_errno != SA_AIS_OK) {
> +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> +  __FUNCTION__, nodeGroupName_s.c_str(),
> +  saf_error(m_errno));
> +  rc = false;
> +  break;
> +} else {
> +m_errno = saImmOmCcbApply(m_ccbHandle);
> +if (m_errno != SA_AIS_OK) {
> +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> +  __FUNCTION__, saf_error(m_errno));
> +  rc = false;
> +}
> +break;
> +}
> +}
> 
>   TRACE_LEAVE();
>   return rc;
> @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp
>   , 0, adminOp, params,
>   _rc, smfd_cb->adminOpTimeout);
> 
> +if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK)
> +break;
> +
>   if (retry <= 0) {
>   LOG_NO("Fail to invoke admin operation, too many
> OI "
>   "TRY_AGAIN, giving up. %s",
> __FUNCTION__);
> 
> --
> ___
> Opensaf-devel mailing list
> Opensaf-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/opensaf-devel

--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

2016-09-23 Thread Rafael Odzakow

comments one comment under [rafael] tag.


On 09/22/2016 03:33 PM, Lennart Lund wrote:
>   osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc |  49 
> +
>   1 files changed, 33 insertions(+), 16 deletions(-)
>
>
> Fix the deleteNodeGroup() method must be so that if the delete operation fails
> with bad handles new handles are created so that the node group can be deleted
>
> diff --git a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc 
> b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> --- a/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> +++ b/osaf/services/saf/smfsv/smfd/SmfUpgradeStep.cc
> @@ -3580,22 +3580,36 @@ bool SmfAdminOperation::deleteNodeGroup(
>   );
>   
>   TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
> -
> - m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> - );
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> -  __FUNCTION__, nodeGroupName_s.c_str(),
> -  saf_error(m_errno));
> - rc = false;
> - } else {
> - m_errno = saImmOmCcbApply(m_ccbHandle);
> - if (m_errno != SA_AIS_OK) {
> - LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> - __FUNCTION__, saf_error(m_errno));
> - rc = false;
> - }
> - }
> +while (true) {
[rafael]  The while loop is here to retry CcbObjectDelete only one time. 
A much clearer way is to call CcbObjectDelete twice. As it looks now I 
needed to reread this section before I understood the logic. By trying 
to avoid writing extra code you have complicated a simple function. Call 
it twice instead of having a while loop that will retry after BAD_HANDLE 
endless amounts of times. A possibility for an infinite loop is not a 
good idea. After the second CcbObjectDelete you can fail normally. 
Please explain in comments or remove this while loop.
> +m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
> +);
> +if (m_errno == SA_AIS_ERR_BAD_HANDLE) {
> +// Handles may have been invalidated/timed out
> +(void)immutil_saImmOmFinalize(m_omHandle);
> +bool rc = getAllImmHandles();
> +if (rc == false) {
> +LOG_NO("%s: getAllImmHandles Fail",
> +   __FUNCTION__);
> +break;
> +}
> +continue;
> +}
> +if (m_errno != SA_AIS_OK) {
> +  LOG_NO("%s: saImmOmCcbObjectDelete '%s' Fail %s",
> +  __FUNCTION__, nodeGroupName_s.c_str(),
> +  saf_error(m_errno));
> +  rc = false;
> +  break;
> +} else {
> +m_errno = saImmOmCcbApply(m_ccbHandle);
> +if (m_errno != SA_AIS_OK) {
> +LOG_NO("%s: saImmOmCcbApply() Fail '%s'",
> +  __FUNCTION__, saf_error(m_errno));
> +  rc = false;
> +}
> +break;
> +}
> +}
>   
>   TRACE_LEAVE();
>   return rc;
> @@ -3633,6 +3647,9 @@ bool SmfAdminOperation::nodeGroupAdminOp
>   , 0, adminOp, params,
>   _rc, smfd_cb->adminOpTimeout);
>   
> +if (m_errno == SA_AIS_OK && oi_rc == SA_AIS_OK)
> +break;
> +
>   if (retry <= 0) {
>   LOG_NO("Fail to invoke admin operation, too many OI "
>   "TRY_AGAIN, giving up. %s", __FUNCTION__);


--
___
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

Re: [devel] [PATCH 1 of 1] smf: Recreate IMM handles if bad handle when deleting node group [#2046]

8 matches

Site Navigation

Mail list logo

Footer information