Re: [devel] [PATCH 2/2] smf: Upgrade nodes without using node group [#2592]

[Rafael Odzakow]

ACK


On 10/23/2017 04:23 PM, Lennart Lund wrote:

Hi Rafael,

* I will fix the name of the constant to follow Google style guide
* Found and fixed the method_rc problem
* I agree that the function should be smaller. However, refactoring have to 
involve
generic abstraction of IMM handling (could be done internally in SMF). The 
problem
here is that complicated IMM API sequences are handled inline and that is 
what makes
the function too long.

I will test and push this now without any more re-reviews.

Thanks
Lennart


-Original Message-
From: Rafael Odzakow
Sent: den 23 oktober 2017 14:29
To: Lennart Lund 
Cc: opensaf-devel@lists.sourceforge.net
Subject: Re: [PATCH 2/2] smf: Upgrade nodes without using node group
[#2592]

One minor comment inline under [rafael] tag. I had a comment previously
for the nodeGroupAdminOperation function which is not addressed.

[rafael] method_rc seems not to be set (to false) if the
AdminOperationInvoke fails without reaching timeout.


A general comment to big functions with break and continue is that the
flow is too complex to have a overview of. Consider refactoring in the
future.



On 10/19/2017 02:16 PM, Lennart Lund wrote:

Udates after review. This commit will be merged with the fix before push
---
   src/smf/smfd/SmfUpgradeStep.cc | 90 +++

---

   src/smf/smfd/SmfUpgradeStep.h  |  6 +--
   2 files changed, 43 insertions(+), 53 deletions(-)

diff --git a/src/smf/smfd/SmfUpgradeStep.cc

b/src/smf/smfd/SmfUpgradeStep.cc

index 892c32602..25f816485 100644
--- a/src/smf/smfd/SmfUpgradeStep.cc
+++ b/src/smf/smfd/SmfUpgradeStep.cc
@@ -2794,7 +2794,7 @@

SmfAdminOperation::SmfAdminOperation(std::list
*i_allUnits)

   m_admin_owner_name.c_str());

 // Become admin owner using above created name
-  bool rc = initAdminOwner();
+  bool rc = initImmOmAndSetAdminOwnerName();
 if (rc == false ) {
   LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
   m_creation_fail = true;
@@ -2978,7 +2978,7 @@ done:
   //   Use becomeAdminOwnerOf(object) and

releaseAdminOwnerOf(object)

   //
   //
-bool SmfAdminOperation::initAdminOwner() {
+bool SmfAdminOperation::initImmOmAndSetAdminOwnerName() {
 SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
 int timeout_try_cnt = 6;
 bool rc = true;
@@ -3020,7 +3020,7 @@ bool SmfAdminOperation::initAdminOwner() {
   // Note: m_omHandle must have been initialized
   // Note: Become admin owner of AMF cluster object
   //
-bool SmfAdminOperation::initNodeGroupOm() {
+bool SmfAdminOperation::becomeNgAdmOwnerAndInitCcb() {
 SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
 int timeout_try_cnt = 6;
 bool rc = true;
@@ -3067,7 +3067,7 @@ bool SmfAdminOperation::initNodeGroupOm() {
   // Note: m_omHandle must have been initialized
   // Note: Release admin owner of AMF cluster object
   //
-void SmfAdminOperation::releaseNodeGroupOm() {
+void SmfAdminOperation::releaseNgAdmOwnerAndCcb() {
 TRACE_ENTER();
 SaAisErrorT ais_rc = immutil_saImmOmCcbFinalize(m_ccbHandle);
 if (ais_rc != SA_AIS_OK) {
@@ -3588,16 +3588,6 @@ bool

SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {

 const SaImmAttrValuesT_2 *attrValues[] = {
 , , ,

NULL};

-  // 
-  // Initialize node group ccb handling
-  // Note: We become admin owner of the node group parent that must

be released

-  //   when node group operations are done.
-  bool method_rc = true;
-  if (initNodeGroupOm() == false) {
-LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
-method_rc = false;
-  }
-
 // 
 // Create the node group
 // Retry if:
@@ -3607,7 +3597,16 @@ bool

SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {

 //   Re-initialize and try again
 const uint32_t MAX_NO_RETRIES = 4;
 uint32_t retry_cnt = 0;
+  bool method_rc = true;
+  m_errno = SA_AIS_OK;
 while ((++retry_cnt <= MAX_NO_RETRIES) && (method_rc == true)) {
+// Note: OM handle and admin owner name is created in the

constructor

+if (becomeNgAdmOwnerAndInitCcb() == false) {
+  LOG_NO("%s: becomeNgAdmOwnerAndInitCcb() Fail",

__FUNCTION__);

+  method_rc = false;
+  break;
+}
+
   // Fill in nodeGroupParentDn parameter for object create operation
   SaImmClassNameT className =

const_cast("SaAmfNodeGroup");

   SaNameT nodeGroupParentDn;
@@ -3628,6 +3627,10 @@ bool

SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {

saf_error(m_errno));
 TRACE("%s: A node group unexpectedly already exist, "
 "deleteNodeGroup", __FUNCTION__);
+
+  // Must be released so that deleteNodeGroup() can be come owner

and

+  // create a CCB for deleting
+  releaseNgAdmOwnerAndCcb();
 bool delete_rc = deleteNodeGroup();
 if (delete_rc == false) {
   

Re: [devel] [PATCH 2/2] smf: Upgrade nodes without using node group [#2592]

[Lennart Lund]

Hi Rafael,

* I will fix the name of the constant to follow Google style guide
* Found and fixed the method_rc problem
* I agree that the function should be smaller. However, refactoring have to 
involve
   generic abstraction of IMM handling (could be done internally in SMF). The 
problem
   here is that complicated IMM API sequences are handled inline and that is 
what makes
   the function too long.

I will test and push this now without any more re-reviews. 

Thanks
Lennart

> -Original Message-
> From: Rafael Odzakow
> Sent: den 23 oktober 2017 14:29
> To: Lennart Lund 
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 2/2] smf: Upgrade nodes without using node group
> [#2592]
> 
> One minor comment inline under [rafael] tag. I had a comment previously
> for the nodeGroupAdminOperation function which is not addressed.
> 
> [rafael] method_rc seems not to be set (to false) if the
> AdminOperationInvoke fails without reaching timeout.
> 
> 
> A general comment to big functions with break and continue is that the
> flow is too complex to have a overview of. Consider refactoring in the
> future.
> 
> 
> 
> On 10/19/2017 02:16 PM, Lennart Lund wrote:
> > Udates after review. This commit will be merged with the fix before push
> > ---
> >   src/smf/smfd/SmfUpgradeStep.cc | 90 +++
> ---
> >   src/smf/smfd/SmfUpgradeStep.h  |  6 +--
> >   2 files changed, 43 insertions(+), 53 deletions(-)
> >
> > diff --git a/src/smf/smfd/SmfUpgradeStep.cc
> b/src/smf/smfd/SmfUpgradeStep.cc
> > index 892c32602..25f816485 100644
> > --- a/src/smf/smfd/SmfUpgradeStep.cc
> > +++ b/src/smf/smfd/SmfUpgradeStep.cc
> > @@ -2794,7 +2794,7 @@
> SmfAdminOperation::SmfAdminOperation(std::list
> *i_allUnits)
> >   m_admin_owner_name.c_str());
> >
> > // Become admin owner using above created name
> > -  bool rc = initAdminOwner();
> > +  bool rc = initImmOmAndSetAdminOwnerName();
> > if (rc == false ) {
> >   LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
> >   m_creation_fail = true;
> > @@ -2978,7 +2978,7 @@ done:
> >   //   Use becomeAdminOwnerOf(object) and
> releaseAdminOwnerOf(object)
> >   //
> >   //
> > -bool SmfAdminOperation::initAdminOwner() {
> > +bool SmfAdminOperation::initImmOmAndSetAdminOwnerName() {
> > SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
> > int timeout_try_cnt = 6;
> > bool rc = true;
> > @@ -3020,7 +3020,7 @@ bool SmfAdminOperation::initAdminOwner() {
> >   // Note: m_omHandle must have been initialized
> >   // Note: Become admin owner of AMF cluster object
> >   //
> > -bool SmfAdminOperation::initNodeGroupOm() {
> > +bool SmfAdminOperation::becomeNgAdmOwnerAndInitCcb() {
> > SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
> > int timeout_try_cnt = 6;
> > bool rc = true;
> > @@ -3067,7 +3067,7 @@ bool SmfAdminOperation::initNodeGroupOm() {
> >   // Note: m_omHandle must have been initialized
> >   // Note: Release admin owner of AMF cluster object
> >   //
> > -void SmfAdminOperation::releaseNodeGroupOm() {
> > +void SmfAdminOperation::releaseNgAdmOwnerAndCcb() {
> > TRACE_ENTER();
> > SaAisErrorT ais_rc = immutil_saImmOmCcbFinalize(m_ccbHandle);
> > if (ais_rc != SA_AIS_OK) {
> > @@ -3588,16 +3588,6 @@ bool
> SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
> > const SaImmAttrValuesT_2 *attrValues[] = {
> > , , ,
> NULL};
> >
> > -  // 
> > -  // Initialize node group ccb handling
> > -  // Note: We become admin owner of the node group parent that must
> be released
> > -  //   when node group operations are done.
> > -  bool method_rc = true;
> > -  if (initNodeGroupOm() == false) {
> > -LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
> > -method_rc = false;
> > -  }
> > -
> > // 
> > // Create the node group
> > // Retry if:
> > @@ -3607,7 +3597,16 @@ bool
> SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
> > //   Re-initialize and try again
> > const uint32_t MAX_NO_RETRIES = 4;
> > uint32_t retry_cnt = 0;
> > +  bool method_rc = true;
> > +  m_errno = SA_AIS_OK;
> > while ((++retry_cnt <= MAX_NO_RETRIES) && (method_rc == true)) {
> > +// Note: OM handle and admin owner name is created in the
> constructor
> > +if (becomeNgAdmOwnerAndInitCcb() == false) {
> > +  LOG_NO("%s: becomeNgAdmOwnerAndInitCcb() Fail",
> __FUNCTION__);
> > +  method_rc = false;
> > +  break;
> > +}
> > +
> >   // Fill in nodeGroupParentDn parameter for object create operation
> >   SaImmClassNameT className =
> const_cast("SaAmfNodeGroup");
> >   SaNameT nodeGroupParentDn;
> > @@ -3628,6 +3627,10 @@ bool
> SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
> >saf_error(m_errno));
> > TRACE("%s: A node group unexpectedly already exist, "
> > 

Re: [devel] [PATCH 2/2] smf: Upgrade nodes without using node group [#2592]

[Rafael Odzakow]

One minor comment inline under [rafael] tag. I had a comment previously 
for the nodeGroupAdminOperation function which is not addressed.


[rafael] method_rc seems not to be set (to false) if the 
AdminOperationInvoke fails without reaching timeout.



A general comment to big functions with break and continue is that the 
flow is too complex to have a overview of. Consider refactoring in the 
future.




On 10/19/2017 02:16 PM, Lennart Lund wrote:

Udates after review. This commit will be merged with the fix before push
---
  src/smf/smfd/SmfUpgradeStep.cc | 90 +++---
  src/smf/smfd/SmfUpgradeStep.h  |  6 +--
  2 files changed, 43 insertions(+), 53 deletions(-)

diff --git a/src/smf/smfd/SmfUpgradeStep.cc b/src/smf/smfd/SmfUpgradeStep.cc
index 892c32602..25f816485 100644
--- a/src/smf/smfd/SmfUpgradeStep.cc
+++ b/src/smf/smfd/SmfUpgradeStep.cc
@@ -2794,7 +2794,7 @@ 
SmfAdminOperation::SmfAdminOperation(std::list *i_allUnits)
  m_admin_owner_name.c_str());
  
// Become admin owner using above created name

-  bool rc = initAdminOwner();
+  bool rc = initImmOmAndSetAdminOwnerName();
if (rc == false ) {
  LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
  m_creation_fail = true;
@@ -2978,7 +2978,7 @@ done:
  //   Use becomeAdminOwnerOf(object) and releaseAdminOwnerOf(object)
  //
  //
-bool SmfAdminOperation::initAdminOwner() {
+bool SmfAdminOperation::initImmOmAndSetAdminOwnerName() {
SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
int timeout_try_cnt = 6;
bool rc = true;
@@ -3020,7 +3020,7 @@ bool SmfAdminOperation::initAdminOwner() {
  // Note: m_omHandle must have been initialized
  // Note: Become admin owner of AMF cluster object
  //
-bool SmfAdminOperation::initNodeGroupOm() {
+bool SmfAdminOperation::becomeNgAdmOwnerAndInitCcb() {
SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
int timeout_try_cnt = 6;
bool rc = true;
@@ -3067,7 +3067,7 @@ bool SmfAdminOperation::initNodeGroupOm() {
  // Note: m_omHandle must have been initialized
  // Note: Release admin owner of AMF cluster object
  //
-void SmfAdminOperation::releaseNodeGroupOm() {
+void SmfAdminOperation::releaseNgAdmOwnerAndCcb() {
TRACE_ENTER();
SaAisErrorT ais_rc = immutil_saImmOmCcbFinalize(m_ccbHandle);
if (ais_rc != SA_AIS_OK) {
@@ -3588,16 +3588,6 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
const SaImmAttrValuesT_2 *attrValues[] = {
, , , NULL};
  
-  // 

-  // Initialize node group ccb handling
-  // Note: We become admin owner of the node group parent that must be released
-  //   when node group operations are done.
-  bool method_rc = true;
-  if (initNodeGroupOm() == false) {
-LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
-method_rc = false;
-  }
-
// 
// Create the node group
// Retry if:
@@ -3607,7 +3597,16 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
//   Re-initialize and try again
const uint32_t MAX_NO_RETRIES = 4;
uint32_t retry_cnt = 0;
+  bool method_rc = true;
+  m_errno = SA_AIS_OK;
while ((++retry_cnt <= MAX_NO_RETRIES) && (method_rc == true)) {
+// Note: OM handle and admin owner name is created in the constructor
+if (becomeNgAdmOwnerAndInitCcb() == false) {
+  LOG_NO("%s: becomeNgAdmOwnerAndInitCcb() Fail", __FUNCTION__);
+  method_rc = false;
+  break;
+}
+
  // Fill in nodeGroupParentDn parameter for object create operation
  SaImmClassNameT className = const_cast("SaAmfNodeGroup");
  SaNameT nodeGroupParentDn;
@@ -3628,6 +3627,10 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
   saf_error(m_errno));
TRACE("%s: A node group unexpectedly already exist, "
"deleteNodeGroup", __FUNCTION__);
+
+  // Must be released so that deleteNodeGroup() can be come owner and
+  // create a CCB for deleting
+  releaseNgAdmOwnerAndCcb();
bool delete_rc = deleteNodeGroup();
if (delete_rc == false) {
  LOG_NO("%s: deleteNodeGroup() Fail", __FUNCTION__);
@@ -3640,20 +3643,15 @@ bool 
SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
// Reinitialize and try again
LOG_NO("%s: saImmOmCcbObjectCreate_2 Fail %s", __FUNCTION__,
   saf_error(m_errno));
-  finalizeNodeGroupOm();
  
-  // After finalize a new OM handle has to be created and node group ccb

-  // handling has to be initialized
-  if (initAdminOwner() == false) {
+  // Note: After finalize a new OM handle and admin owner name has to be
+  // created
+  finalizeNodeGroupOm();
+  if (initImmOmAndSetAdminOwnerName() == false) {
  LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
  method_rc = false;
  break;
}
-  if (initNodeGroupOm() == false) {
-

[devel] [PATCH 2/2] smf: Upgrade nodes without using node group [#2592]

[Lennart Lund]

Udates after review. This commit will be merged with the fix before push
---
 src/smf/smfd/SmfUpgradeStep.cc | 90 +++---
 src/smf/smfd/SmfUpgradeStep.h  |  6 +--
 2 files changed, 43 insertions(+), 53 deletions(-)

diff --git a/src/smf/smfd/SmfUpgradeStep.cc b/src/smf/smfd/SmfUpgradeStep.cc
index 892c32602..25f816485 100644
--- a/src/smf/smfd/SmfUpgradeStep.cc
+++ b/src/smf/smfd/SmfUpgradeStep.cc
@@ -2794,7 +2794,7 @@ 
SmfAdminOperation::SmfAdminOperation(std::list *i_allUnits)
 m_admin_owner_name.c_str());
 
   // Become admin owner using above created name
-  bool rc = initAdminOwner();
+  bool rc = initImmOmAndSetAdminOwnerName();
   if (rc == false ) {
 LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
 m_creation_fail = true;
@@ -2978,7 +2978,7 @@ done:
 //   Use becomeAdminOwnerOf(object) and releaseAdminOwnerOf(object)
 //
 //
-bool SmfAdminOperation::initAdminOwner() {
+bool SmfAdminOperation::initImmOmAndSetAdminOwnerName() {
   SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
   int timeout_try_cnt = 6;
   bool rc = true;
@@ -3020,7 +3020,7 @@ bool SmfAdminOperation::initAdminOwner() {
 // Note: m_omHandle must have been initialized
 // Note: Become admin owner of AMF cluster object
 //
-bool SmfAdminOperation::initNodeGroupOm() {
+bool SmfAdminOperation::becomeNgAdmOwnerAndInitCcb() {
   SaAisErrorT ais_rc = SA_AIS_ERR_TRY_AGAIN;
   int timeout_try_cnt = 6;
   bool rc = true;
@@ -3067,7 +3067,7 @@ bool SmfAdminOperation::initNodeGroupOm() {
 // Note: m_omHandle must have been initialized
 // Note: Release admin owner of AMF cluster object
 //
-void SmfAdminOperation::releaseNodeGroupOm() {
+void SmfAdminOperation::releaseNgAdmOwnerAndCcb() {
   TRACE_ENTER();
   SaAisErrorT ais_rc = immutil_saImmOmCcbFinalize(m_ccbHandle);
   if (ais_rc != SA_AIS_OK) {
@@ -3588,16 +3588,6 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
   const SaImmAttrValuesT_2 *attrValues[] = {
   , , , NULL};
 
-  // 
-  // Initialize node group ccb handling
-  // Note: We become admin owner of the node group parent that must be released
-  //   when node group operations are done.
-  bool method_rc = true;
-  if (initNodeGroupOm() == false) {
-LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
-method_rc = false;
-  }
-
   // 
   // Create the node group
   // Retry if:
@@ -3607,7 +3597,16 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
   //   Re-initialize and try again
   const uint32_t MAX_NO_RETRIES = 4;
   uint32_t retry_cnt = 0;
+  bool method_rc = true;
+  m_errno = SA_AIS_OK;
   while ((++retry_cnt <= MAX_NO_RETRIES) && (method_rc == true)) {
+// Note: OM handle and admin owner name is created in the constructor
+if (becomeNgAdmOwnerAndInitCcb() == false) {
+  LOG_NO("%s: becomeNgAdmOwnerAndInitCcb() Fail", __FUNCTION__);
+  method_rc = false;
+  break;
+}
+
 // Fill in nodeGroupParentDn parameter for object create operation
 SaImmClassNameT className = const_cast("SaAmfNodeGroup");
 SaNameT nodeGroupParentDn;
@@ -3628,6 +3627,10 @@ bool SmfAdminOperation::createNodeGroup(SaAmfAdminStateT 
i_fromState) {
  saf_error(m_errno));
   TRACE("%s: A node group unexpectedly already exist, "
   "deleteNodeGroup", __FUNCTION__);
+
+  // Must be released so that deleteNodeGroup() can be come owner and
+  // create a CCB for deleting
+  releaseNgAdmOwnerAndCcb();
   bool delete_rc = deleteNodeGroup();
   if (delete_rc == false) {
 LOG_NO("%s: deleteNodeGroup() Fail", __FUNCTION__);
@@ -3640,20 +3643,15 @@ bool 
SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
   // Reinitialize and try again
   LOG_NO("%s: saImmOmCcbObjectCreate_2 Fail %s", __FUNCTION__,
  saf_error(m_errno));
-  finalizeNodeGroupOm();
 
-  // After finalize a new OM handle has to be created and node group ccb
-  // handling has to be initialized
-  if (initAdminOwner() == false) {
+  // Note: After finalize a new OM handle and admin owner name has to be
+  // created
+  finalizeNodeGroupOm();
+  if (initImmOmAndSetAdminOwnerName() == false) {
 LOG_NO("%s: initAdminOwner() Fail", __FUNCTION__);
 method_rc = false;
 break;
   }
-  if (initNodeGroupOm() == false) {
-LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
-method_rc = false;
-break;
-  }
 
   TRACE("\t Try again after %s", saf_error(m_errno));
   continue;
@@ -3670,14 +3668,14 @@ bool 
SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
 method_rc = false;
   } else {
 method_rc = true;
+TRACE("%s CCB is applied", __FUNCTION__);
   }
-  TRACE("%s CCB is applied", __FUNCTION__);
-  break;
 }
+break;
   }
 
   // Finalize the