opensaf-devel

[devel] [PATCH 0 of 1] Review Request for amfnd: avoid creating existing safHealthcheckKey record in amfnd database [#1363]

[<pra...@or...>]

Summary: amfnd: avoid creating existing safHealthcheckKey record in amfnd database [#1363] 
Review request for Trac Ticket(s): <<IF ANY LIST THE #>>
Peer Reviewer(s): Hans, Nagendra 
Pull request to: <<LIST THE PERSON WITH PUSH ACCESS HERE>>
Affected branch(es): ALL 
Development branch: <<IF ANY GIVE THE REPO URL>>

--------------------------------
Impacted area       Impact y/n
--------------------------------
 Docs                    n
 Build system            n
 RPM/packaging           n
 Configuration files     n
 Startup scripts         n
 SAF services            y
 OpenSAF services        n
 Core libraries          n
 Samples                 n
 Tests                   n
 Other                   n


Comments (indicate scope for each "y" above):
---------------------------------------------

changeset 6e0831c4e4688db34acfa8035e464af6df7900b2
Author:	pra...@or...
Date:	Sun, 03 May 2015 00:25:29 +0530

	amfnd: avoid creating existing safHealthcheckKey record in amfnd database
	[#1363]

	saAmfHealthcheckStart() API fails with SA_AIS_ERR_NOT_EXIST in the reported
	case.

	In the reported case, Application is up with 2 SUs with one component in
	each SU. Each component belongs to same component type.HealtchCheckKey is
	configured for component type. Now, dynamically, a SU is added in the system
	having one component of same component type and a different health check key
	is configured for the same component type. When this SU is unlocked,
	saAmfHealthcheckStart() API fails with SA_AIS_ERR_NOT_EXIST. Why ? As a
	part of dynamic addition of SU, AMFND reads SU configuration from IMM. While
	reading information for healthCheckType, AMFND reads all keys again for the
	same compType and try to create again record for it. Since record already
	exists in its database, record is not added in database and at this error
	AMF does not continue to read other healthCheckType from IMM.AMFND must skip
	creating existing records and must continue to read other records from IMM.

	 Patch ensures that AMFND first checks if record already exists in its
	database. It creates record only if it does not exists. Thus error situation
	while addition of record is avoided and all records are read.


Complete diffstat:
------------------
 osaf/services/saf/amf/amfnd/chc.cc  |   2 +-
 osaf/services/saf/amf/amfnd/hcdb.cc |  12 +++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)


Testing Commands:
-----------------
As per ticket description.

Testing, Expected Results:
--------------------------
HealthStart() API is successful when SU in unlocked-in.

Conditions of Submission:
-------------------------
Ack from any reviewer.

Arch      Built     Started    Linux distro
-------------------------------------------
mips        n          n
mips64      n          n
x86         n          n
x86_64      y          y
powerpc     n          n
powerpc64   n          n


Reviewer Checklist:
-------------------
[Submitters: make sure that your review doesn't trigger any checkmarks!]


Your checkin has not passed review because (see checked entries):

___ Your RR template is generally incomplete; it has too many blank entries
    that need proper data filled in.

___ You have failed to nominate the proper persons for review and push.

___ Your patches do not have proper short+long header

___ You have grammar/spelling in your header that is unacceptable.

___ You have exceeded a sensible line length in your headers/comments/text.

___ You have failed to put in a proper Trac Ticket # into your commits.

___ You have incorrectly put/left internal data in your comments/files
    (i.e. internal bug tracking tool IDs, product names etc)

___ You have not given any evidence of testing beyond basic build tests.
    Demonstrate some level of runtime or other sanity testing.

___ You have ^M present in some of your files. These have to be removed.

___ You have needlessly changed whitespace or added whitespace crimes
    like trailing spaces, or spaces before tabs.

___ You have mixed real technical changes with whitespace and other
    cosmetic code cleanup changes. These have to be separate commits.

___ You need to refactor your submission into logical chunks; there is
    too much content into a single commit.

___ You have extraneous garbage in your review (merge commits etc)

___ You have giant attachments which should never have been sent;
    Instead you should place your content in a public tree to be pulled.

___ You have too many commits attached to an e-mail; resend as threaded
    commits, or place in a public tree for a pull.

___ You have resent this content multiple times without a clear indication
    of what has changed between each re-send.

___ You have failed to adequately and individually address all of the
    comments and change requests that were proposed in the initial review.

___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)

___ Your computer have a badly configured date and time; confusing the
    the threaded patch review.

___ Your changes affect IPC mechanism, and you don't present any results
    for in-service upgradability test.

___ Your changes affect user manual and documentation, your patch series
    do not contain the patch that updates the Doxygen manual.

[devel] [PATCH 1 of 1] amfnd: avoid creating existing safHealthcheckKey record in amfnd database [#1363]

[<pra...@or...>]

 osaf/services/saf/amf/amfnd/chc.cc  |   2 +-
 osaf/services/saf/amf/amfnd/hcdb.cc |  12 +++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)


saAmfHealthcheckStart() API fails with SA_AIS_ERR_NOT_EXIST in the reported case.

In the reported case, Application is up with 2 SUs with one component in each SU.
Each component belongs to same component type.HealtchCheckKey is configured for
component type. Now, dynamically, a SU is added in the system having one component
of same component type and a different health check key is configured for the same
component type. When this SU is unlocked, saAmfHealthcheckStart() API fails with
SA_AIS_ERR_NOT_EXIST. Why  ? As a part of dynamic addition of SU, AMFND reads SU
configuration from IMM. While reading information for healthCheckType,
AMFND reads all keys again for the same compType and try to create again record for
it. Since record already exists in its database, record is not added in database and
at this error AMF does not continue to read other healthCheckType from IMM.AMFND must
skip creating existing records and must continue to read other records from IMM.


Patch ensures that AMFND first checks if record already exists in its database. It creates
record only if it does not exists. Thus error situation while addition of record is avoided
and all records are read.

diff --git a/osaf/services/saf/amf/amfnd/chc.cc b/osaf/services/saf/amf/amfnd/chc.cc
--- a/osaf/services/saf/amf/amfnd/chc.cc
+++ b/osaf/services/saf/amf/amfnd/chc.cc
@@ -100,7 +100,7 @@ uint32_t avnd_evt_ava_hc_start_evh(AVND_
 	}
 	/* validate the healthcheck start message */
 	avnd_comp_hc_param_val(cb, AVSV_AMF_HC_START, (uint8_t *)hc_start, &comp, 0, &amf_rc);
-
+	TRACE_1("sending response:%d",amf_rc);
 	/* send the response back to AvA */
 	rc = avnd_amf_resp_send(cb, AVSV_AMF_HC_START, amf_rc, 0, &api_info->dest, &evt->mds_ctxt, comp, msg_from_avnd);
 
diff --git a/osaf/services/saf/amf/amfnd/hcdb.cc b/osaf/services/saf/amf/amfnd/hcdb.cc
--- a/osaf/services/saf/amf/amfnd/hcdb.cc
+++ b/osaf/services/saf/amf/amfnd/hcdb.cc
@@ -326,9 +326,15 @@ SaAisErrorT avnd_hctype_config_get(SaImm
 	while (immutil_saImmOmSearchNext_2(searchHandle, &hc_name, (SaImmAttrValuesT_2 ***)&attributes) == SA_AIS_OK) {
 
 		TRACE_1("'%s'", hc_name.value);
-
-		if (hctype_create(avnd_cb, &hc_name, attributes) == NULL)
-			goto done2;
+		//A record may get created in the context of some other component of same comptype.
+		AVND_HCTYPE *hctype = NULL;
+		if ((hctype = (AVND_HCTYPE *)ncs_patricia_tree_get(&hctypedb,
+						(uint8_t *)&hc_name)) == NULL) {
+			if (hctype_create(avnd_cb, &hc_name, attributes) == NULL)
+				goto done2;
+		}
+		else 
+			TRACE_2("Record already exists");
 	}
 
 	error = SA_AIS_OK;

Re: [devel] [PATCH 1 of 1] amfnd: avoid creating existing safHealthcheckKey record in amfnd database [#1363]

[Hans N. <han...@er...>]

ack, code review only. /Thanks HansN

On 05/04/2015 12:10 PM, pra...@or... wrote:
>   osaf/services/saf/amf/amfnd/chc.cc  |   2 +-
>   osaf/services/saf/amf/amfnd/hcdb.cc |  12 +++++++++---
>   2 files changed, 10 insertions(+), 4 deletions(-)
>
>
> saAmfHealthcheckStart() API fails with SA_AIS_ERR_NOT_EXIST in the reported case.
>
> In the reported case, Application is up with 2 SUs with one component in each SU.
> Each component belongs to same component type.HealtchCheckKey is configured for
> component type. Now, dynamically, a SU is added in the system having one component
> of same component type and a different health check key is configured for the same
> component type. When this SU is unlocked, saAmfHealthcheckStart() API fails with
> SA_AIS_ERR_NOT_EXIST. Why  ? As a part of dynamic addition of SU, AMFND reads SU
> configuration from IMM. While reading information for healthCheckType,
> AMFND reads all keys again for the same compType and try to create again record for
> it. Since record already exists in its database, record is not added in database and
> at this error AMF does not continue to read other healthCheckType from IMM.AMFND must
> skip creating existing records and must continue to read other records from IMM.
>
>
> Patch ensures that AMFND first checks if record already exists in its database. It creates
> record only if it does not exists. Thus error situation while addition of record is avoided
> and all records are read.
>
> diff --git a/osaf/services/saf/amf/amfnd/chc.cc b/osaf/services/saf/amf/amfnd/chc.cc
> --- a/osaf/services/saf/amf/amfnd/chc.cc
> +++ b/osaf/services/saf/amf/amfnd/chc.cc
> @@ -100,7 +100,7 @@ uint32_t avnd_evt_ava_hc_start_evh(AVND_
>   	}
>   	/* validate the healthcheck start message */
>   	avnd_comp_hc_param_val(cb, AVSV_AMF_HC_START, (uint8_t *)hc_start, &comp, 0, &amf_rc);
> -
> +	TRACE_1("sending response:%d",amf_rc);
>   	/* send the response back to AvA */
>   	rc = avnd_amf_resp_send(cb, AVSV_AMF_HC_START, amf_rc, 0, &api_info->dest, &evt->mds_ctxt, comp, msg_from_avnd);
>   
> diff --git a/osaf/services/saf/amf/amfnd/hcdb.cc b/osaf/services/saf/amf/amfnd/hcdb.cc
> --- a/osaf/services/saf/amf/amfnd/hcdb.cc
> +++ b/osaf/services/saf/amf/amfnd/hcdb.cc
> @@ -326,9 +326,15 @@ SaAisErrorT avnd_hctype_config_get(SaImm
>   	while (immutil_saImmOmSearchNext_2(searchHandle, &hc_name, (SaImmAttrValuesT_2 ***)&attributes) == SA_AIS_OK) {
>   
>   		TRACE_1("'%s'", hc_name.value);
> -
> -		if (hctype_create(avnd_cb, &hc_name, attributes) == NULL)
> -			goto done2;
> +		//A record may get created in the context of some other component of same comptype.
> +		AVND_HCTYPE *hctype = NULL;
> +		if ((hctype = (AVND_HCTYPE *)ncs_patricia_tree_get(&hctypedb,
> +						(uint8_t *)&hc_name)) == NULL) {
> +			if (hctype_create(avnd_cb, &hc_name, attributes) == NULL)
> +				goto done2;
> +		}
> +		else
> +			TRACE_2("Record already exists");
>   	}
>   
>   	error = SA_AIS_OK;