Thanks for the comments Rafael. 1) I will look at that page, and refactor that part of the code. I agree that it is difficult to read.
2) The reason setting the maintenance status was moved to after the node
up, is because AMF will generate the operational state change
notification with the saAmfSUMaintenanceCampaign object set in the
notification for the old software when the node is brought down. SMF
would detect this as a failure, even though it is not, because it is the
old software going down. It was being set too early. According to
4.2.1.3 it should be set right before the activation unit is activated
with the new software.
I will submit a new patch soon.
Alex
On 11/10/2016 05:17 AM, Rafael Odzakow wrote:
> ------------------------------------------------------------------------
> NOTICE: This email was received from an EXTERNAL sender
> ------------------------------------------------------------------------
>
> Hello,
>
> looks good to me. Got a question and a comment below under the [rafael] tag.
>
>
> On 10/31/2016 09:31 PM, Alex Jones wrote:
>> osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc | 175
> +++++++++++++++++++++-
>> osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh | 3 +
>> osaf/services/saf/smfsv/smfd/SmfStepTypes.cc | 32 ++-
>> 3 files changed, 191 insertions(+), 19 deletions(-)
>>
>>
>> This patch adds support for section 4.2.1.3 of SMF A.01.02 spec.
>>
>> diff --git a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc
> b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc
>> --- a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc
>> +++ b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.cc
>> @@ -39,6 +39,7 @@
>> #include "SmfUtils.hh"
>>
>> SmfCampaignThread *SmfCampaignThread::s_instance = NULL;
>> +SaNtfSubscriptionIdT SmfCampaignThread::operStateSubId(1);
>>
>> #define SMF_RDA_RETRY_COUNT 25 /* This is taken as the csi's are
> assigned in parallel */
>>
>> @@ -140,7 +141,12 @@ void SmfCampaignThread::main(NCSCONTEXT
>> SmfCampaignThread::~SmfCampaignThread()
>> {
>> TRACE_ENTER();
>> - SaAisErrorT rc = saNtfFinalize(m_ntfHandle);
>> + SaAisErrorT rc(saNtfNotificationUnsubscribe(operStateSubId));
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("Failed to unsubscribe to oper state notifications %u", rc);
>> + }
>> +
>> + rc = saNtfFinalize(m_ntfHandle);
>> if (rc != SA_AIS_OK) {
>> LOG_ER("Failed to finalize NTF handle %u", rc);
>> }
>> @@ -303,10 +309,14 @@ int SmfCampaignThread::initNtf(void)
>> {
>> SaAisErrorT rc = SA_AIS_ERR_TRY_AGAIN;
>> SaVersionT ntfVersion = { 'A', 1, 1 };
>> + SaNtfCallbacksT callbacks = {
>> + ntfNotificationCallback,
>> + 0
>> + };
>> unsigned int numOfTries = 50;
>>
>> while (rc == SA_AIS_ERR_TRY_AGAIN && numOfTries > 0) {
>> - rc = saNtfInitialize(&m_ntfHandle, NULL, &ntfVersion);
>> + rc = saNtfInitialize(&m_ntfHandle, &callbacks, &ntfVersion);
>> if (rc != SA_AIS_ERR_TRY_AGAIN) {
>> break;
>> }
>> @@ -319,9 +329,142 @@ int SmfCampaignThread::initNtf(void)
>> return -1;
>> }
>>
>> + // subscribe to operational state change notifications generated by AMF
>> + rc = initNtfSubscriptions();
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("initNtfSubscriptions FAILED rc=%s", saf_error(rc));
>> + return -1;
>> + }
>> +
>> return 0;
>> }
>>
>> +SaAisErrorT SmfCampaignThread::initNtfSubscriptions(void) {
>> + TRACE_ENTER();
>> + SaAisErrorT rc(SA_AIS_OK);
>> +
>> + do {
>> + SaNtfStateChangeNotificationFilterT stateChangeFilter;
>> +
>> + SaAisErrorT rc(saNtfStateChangeNotificationFilterAllocate(
>> + m_ntfHandle,
>> + &stateChangeFilter,
>> + 0,
>> + 0,
>> + 0,
>> + 0,
>> + 0,
>> + 0));
>> +
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("saNtfAttributeChangeNotificationFilterAllocate FAILED rc=%s",
>> + saf_error(rc));
>> + break;
>> + }
>> +
>> + SaNtfNotificationTypeFilterHandlesT notificationFilterHandles = {
>> + 0,
>> + 0,
>> + stateChangeFilter.notificationFilterHandle,
>> + 0,
>> + 0
>> + };
>> +
>> + rc = saNtfNotificationSubscribe(¬ificationFilterHandles,
> operStateSubId);
>> +
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("saNtfNotificationSubscribe FAILED rc=%s", saf_error(rc));
>> + break;
>> + }
>> +
>> + rc =
> saNtfNotificationFilterFree(stateChangeFilter.notificationFilterHandle);
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("saNtfNotificationFilterFree FAILED rc=%s", saf_error(rc));
>> + break;
>> + }
>> + } while (false);
>> +
>> + TRACE_LEAVE();
>> + return rc;
>> +}
>> +
>> +void SmfCampaignThread::ntfNotificationCallback(
>> + SaNtfSubscriptionIdT subId,
>> + const SaNtfNotificationsT *notification) {
>> + TRACE_ENTER();
>> +
>> + do {
>> + if (subId != operStateSubId) {
>> + TRACE("unknown subscription id received in ntfNotificationCallback:
> %d", subId);
>> + break;
>> + }
>> +
>> + // check if an SU was disabled after it had been upgraded
>> + if (notification->notificationType == SA_NTF_TYPE_STATE_CHANGE) {
>> + const SaNtfStateChangeNotificationT& stateChangeNotification(
>> + notification->notification.stateChangeNotification);
>> +
>> + if (stateChangeNotification.notificationHeader.eventType &&
>> + *stateChangeNotification.notificationHeader.eventType ==
>> + SA_NTF_OBJECT_STATE_CHANGE) {
>> + if (stateChangeNotification.notificationHeader.notificationClassId &&
>> +
> stateChangeNotification.notificationHeader.notificationClassId->vendorId ==
>> + SA_NTF_VENDOR_ID_SAF &&
>> +
> stateChangeNotification.notificationHeader.notificationClassId->majorId ==
>> + SA_SVC_AMF &&
>> +
> stateChangeNotification.notificationHeader.notificationClassId->minorId ==
>> + SA_AMF_NTFID_SU_OP_STATE) {
> [rafael] If possible try to improve the readability. This would be
> applicable here. Specially for the do while and nested if clauses.
>
> http://www.informit.com/articles/article.aspx?p=1398607
>> + for (SaUint16T i(0); i < stateChangeNotification.numStateChanges; i++) {
>> + if (stateChangeNotification.changedStates[i].newState ==
>> + SA_AMF_OPERATIONAL_DISABLED &&
>> + stateChangeNotification.notificationHeader.additionalInfo) {
>> + for (SaUint16T j(0);
>> + j < stateChangeNotification.notificationHeader.numAdditionalInfo;
>> + j++) {
>> + if (stateChangeNotification.notificationHeader.additionalInfo[j].infoId
>> + == SA_AMF_MAINTENANCE_CAMPAIGN_DN &&
>> + stateChangeNotification.notificationHeader.additionalInfo[j].infoType
>> + == SA_NTF_VALUE_LDAP_NAME) {
>> + SaNameT *name(0);
>> + SaUint16T dataSize(0);
>> +
>> + SaAisErrorT rc(saNtfPtrValGet(
>> + stateChangeNotification.notificationHandle,
>> + &stateChangeNotification.notificationHeader.additionalInfo[j].infoValue,
>> + reinterpret_cast<void **>(&name),
>> + &dataSize));
>> +
>> + if (rc == SA_AIS_OK) {
>> + SaConstStringT maintenanceCampaign(saAisNameBorrow(name));
>> +
>> + if (!strcmp(maintenanceCampaign,
>> + s_instance->m_campaign->getDn().c_str())) {
>> + LOG_ER("SU: %s failed after upgrade in campaign",
>> +
> saAisNameBorrow(stateChangeNotification.notificationHeader.notificationObject));
>> + s_instance->m_campaign->getUpgradeCampaign()->suspend();
>> + s_instance->m_campaign->getUpgradeCampaign()->setCampState(
>> + SA_SMF_CMPG_ERROR_DETECTED);
>> +
>> + std::string error(saAisNameBorrow(
>> + stateChangeNotification.notificationHeader.notificationObject));
>> +
>> + error += " failed after upgrade";
>> + s_instance->m_campaign->setError(error);
>> + }
>> + } else {
>> + LOG_ER("saNtfPtrValGet failed: %d", rc);
>> + }
>> + }
>> + }
>> + }
>> + }
>> + }
>> + }
>> + }
>> + } while (false);
>> +
>> + TRACE_LEAVE();
>> +}
>> /**
>> * SmfCampaignThread::send
>> * send event to the thread.
>> @@ -672,18 +815,31 @@ int SmfCampaignThread::handleEvents(void
>> {
>> TRACE_ENTER();
>> NCS_SEL_OBJ mbx_fd = ncs_ipc_get_sel_obj(&m_mbx);
>> - struct pollfd fds[1];
>> +
>> + SaSelectionObjectT ntfSelObj(0);
>> +
>> + SaAisErrorT rc(saNtfSelectionObjectGet(m_ntfHandle, &ntfSelObj));
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("saNtfSelectionObjectGet FAILED - %s", saf_error(rc));
>> + return 1;
>> + }
>> +
>> + struct pollfd fds[2];
>>
>> /* Set up all file descriptors to listen to */
>> fds[0].fd = mbx_fd.rmv_obj;
>> fds[0].events = POLLIN;
>> fds[0].revents = 0;
>>
>> + fds[1].fd = ntfSelObj;
>> + fds[1].events = POLLIN;
>> + fds[1].revents = 0;
>> +
>> TRACE("Campaign thread %s waiting for events",
> m_campaign->getDn().c_str());
>>
>> while (m_running) {
>>
>> - int ret = poll(fds, 1, SMF_UPDATE_ELAPSED_TIME_INTERVAL);
>> + int ret = poll(fds, sizeof(fds) / sizeof(pollfd),
> SMF_UPDATE_ELAPSED_TIME_INTERVAL);
>>
>> if (ret == -1) {
>> if (errno == EINTR)
>> @@ -699,7 +855,16 @@ int SmfCampaignThread::handleEvents(void
>> processEvt();
>> }
>>
>> - m_campaign->updateElapsedTime();
>> + if (fds[1].revents & POLLIN) {
>> + // dispatch NTF events
>> + rc = saNtfDispatch(m_ntfHandle, SA_DISPATCH_ALL);
>> +
>> + if (rc != SA_AIS_OK) {
>> + LOG_ER("saNtfDispatch FAILED - %s", saf_error(rc));
>> + }
>> + }
>> +
>> + m_campaign->updateElapsedTime();
>> }
>> TRACE_LEAVE();
>> return 0;
>> diff --git a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh
> b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh
>> --- a/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh
>> +++ b/osaf/services/saf/smfsv/smfd/SmfCampaignThread.hh
>> @@ -190,9 +190,12 @@ class SmfCampaignThread {
>>
>> SaAisErrorT createImmHandle(SmfCampaign * i_campaign);
>> SaAisErrorT deleteImmHandle();
>> + SaAisErrorT initNtfSubscriptions(void);
>>
>> + static void ntfNotificationCallback(SaNtfSubscriptionIdT, const
> SaNtfNotificationsT *);
>> static void main(NCSCONTEXT info);
>> static SmfCampaignThread *s_instance;
>> + static SaNtfSubscriptionIdT operStateSubId;
>>
>> NCSCONTEXT m_task_hdl;
>> SYSF_MBX m_mbx; /* mailbox */
>> diff --git a/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc
> b/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc
>> --- a/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc
>> +++ b/osaf/services/saf/smfsv/smfd/SmfStepTypes.cc
>> @@ -1247,8 +1247,8 @@ SmfStepTypeNodeReboot::execute()
>> return false;
>> }
>>
>> - /* Modify information model and set maintenance status */
>> - LOG_NO("STEP: Modify information model and set maintenance status");
>> + /* Modify information model */
>> + LOG_NO("STEP: Modify information model");
>> if (m_step->modifyInformationModel() != SA_AIS_OK) {
>> LOG_ER("Failed to Modify information model in
> step=%s",m_step->getRdn().c_str());
>> return false;
>> @@ -1261,11 +1261,6 @@ SmfStepTypeNodeReboot::execute()
>> return false;
>> }
>>
>> - if (m_step->setMaintenanceStateActUnits() == false) {
>> - LOG_ER("Failed to set maintenance state in
> step=%s",m_step->getRdn().c_str());
>> - return false;
>> - }
>> -
>> /* The action below is an add on to
> SMF.---------------------------------------*/
>> /* See if any of the software bundles installed at online installation
> has the */
>> /* saSmfBundleInstallOfflineScope attribute set to
> SA_SMF_CMD_SCOPE_PLM_EE */
>> @@ -1372,6 +1367,13 @@ SmfStepTypeNodeReboot::execute()
>> /* the units in the same state as before locking */
>> m_step->copyDuInitStateToAu();
>>
>> + /* Now that the node is up, set the maintenance status */
>> + LOG_NO("STEP: Set Maintenance Status");
>> + if (m_step->setMaintenanceStateActUnits() == false) {
>> + LOG_ER("Failed to set maintenance state in
> step=%s",m_step->getRdn().c_str());
>> + return false;
>> + }
>> +
> [rafael] Why was setting the maintenance status moved to after the node up?
>> /* Instantiate activation units */
>> LOG_NO("STEP: Instantiate activation units");
>> if (m_step->instantiateActivationUnits() == false) {
>> @@ -1685,8 +1687,8 @@ SmfStepTypeNodeRebootAct::execute()
>> return false;
>> }
>>
>> - /* Modify information model and set maintenance status */
>> - LOG_NO("STEP: Modify information model and set maintenance status");
>> + /* Modify information model */
>> + LOG_NO("STEP: Modify information model");
>> if (m_step->modifyInformationModel() != SA_AIS_OK) {
>> LOG_ER("Failed to Modify information model in
> step=%s",m_step->getRdn().c_str());
>> return false;
>> @@ -1699,11 +1701,6 @@ SmfStepTypeNodeRebootAct::execute()
>> return false;
>> }
>>
>> - if (m_step->setMaintenanceStateActUnits() == false) {
>> - LOG_ER("Failed to set maintenance state in
> step=%s",m_step->getRdn().c_str());
>> - return false;
>> - }
>> -
>> /* Offline installation of new software */
>> LOG_NO("STEP: Offline installation of new software");
>> if (m_step->offlineInstallNewBundles() == false) {
>> @@ -1746,6 +1743,13 @@ SmfStepTypeNodeRebootAct::execute()
>> /* the units in the same state as before locking */
>> m_step->copyDuInitStateToAu();
>>
>> + /* Now that the node is up, set the maintenance status */
>> + LOG_NO("STEP: Set Maintenance Status");
>> + if (m_step->setMaintenanceStateActUnits() == false) {
>> + LOG_ER("Failed to set maintenance state in
> step=%s",m_step->getRdn().c_str());
>> + return false;
>> + }
>> +
>> /* Instantiate activation units */
>> LOG_NO("STEP: Instantiate activation units");
>> if (m_step->instantiateActivationUnits() == false) {
>>
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
_______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
