-----Original Message-----
From: Rafael Odzakow
Sent: den 23 oktober 2017 14:29
To: Lennart Lund <[email protected]>
Cc: [email protected]
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<unitNameAndState>
*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[] = {
&safAmfNodeGroup, &saAmfNGAdminState, &saAmfNGNodeListAttr,
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<SaImmClassNameT>("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 ccb and release admin ownership of node group parent
- releaseNodeGroupOm();
+ releaseNgAdmOwnerAndCcb();
if (nodeName != NULL) free(nodeName);
if (nodeNameList != NULL) free(nodeNameList);
TRACE_LEAVE2("method_rc %s", method_rc ? "Ok" : "Fail");
@@ -3688,29 +3686,25 @@ bool
SmfAdminOperation::createNodeGroup(SaAmfAdminStateT i_fromState) {
/// Return false if Fail. m_errno is set
///
bool SmfAdminOperation::deleteNodeGroup() {
- m_errno = SA_AIS_OK;
-
TRACE_ENTER();
std::string nodeGroupName_s =
m_instanceNodeGroupName + "," + m_nodeGroupParentDn;
SaNameT nodeGroupName;
osaf_extended_name_lend(nodeGroupName_s.c_str(),
&nodeGroupName);
+ TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
- // ------------------------------------
- // 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.
- TRACE("%s: initNodeGroupOm", __FUNCTION__);
- bool method_rc = true;
- if (initNodeGroupOm() == false) {
+ m_errno = SA_AIS_OK;
+ bool method_rc = true;
+ const uint32_t MAX_NO_RETRIES = 3;
[rafael] this variable should not be in all capital letters.
+ uint32_t retry_cnt = 0;
+ while (++retry_cnt <= MAX_NO_RETRIES) {
+ // Note: OM handle and admin owner name is created in the
constructor
+ if (becomeNgAdmOwnerAndInitCcb() == false) {
LOG_NO("%s: initNodeGroupOm() Fail", __FUNCTION__);
method_rc = false;
+ break;
}
- TRACE("\t Deleting nodeGroup '%s'", nodeGroupName_s.c_str());
- const uint32_t MAX_NO_RETRIES = 3;
- uint32_t retry_cnt = 0;
- while (++retry_cnt <= MAX_NO_RETRIES) {
m_errno = immutil_saImmOmCcbObjectDelete(m_ccbHandle,
&nodeGroupName);
TRACE("%s: immutil_saImmOmCcbObjectDelete %s", __FUNCTION__,
saf_error(m_errno));
@@ -3719,20 +3713,15 @@ bool SmfAdminOperation::deleteNodeGroup()
{
m_errno == SA_AIS_ERR_BAD_OPERATION) {
LOG_NO("%s: saImmOmCcbObjectDelete 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;
@@ -3749,13 +3738,14 @@ bool SmfAdminOperation::deleteNodeGroup()
{
method_rc = false;
} else {
method_rc = true;
+ TRACE("%s CCB is applied", __FUNCTION__);
}
- break;
}
+ break;
}
// Finalize the ccb and release admin ownership of node group parent
- releaseNodeGroupOm();
+ releaseNgAdmOwnerAndCcb();
TRACE_LEAVE2("method_rc %s, m_errno %s", method_rc ? "Ok" : "Fail",
saf_error(m_errno));
diff --git a/src/smf/smfd/SmfUpgradeStep.h
b/src/smf/smfd/SmfUpgradeStep.h
index e293acebc..61acd2cb5 100644
--- a/src/smf/smfd/SmfUpgradeStep.h
+++ b/src/smf/smfd/SmfUpgradeStep.h
@@ -861,9 +861,9 @@ class SmfAdminOperation {
bool restart();
private:
- bool initAdminOwner();
- bool initNodeGroupOm();
- void releaseNodeGroupOm();
+ bool initImmOmAndSetAdminOwnerName();
+ bool becomeNgAdmOwnerAndInitCcb();
+ void releaseNgAdmOwnerAndCcb();
bool becomeAdminOwnerOf(std::string& object_name);
bool releaseAdminOwnerOf(std::string& object_name);
void finalizeNodeGroupOm();
