[APPC-143] OAM restart operation with stop error

Fix OAM restart operation does not report stop failure. And also remove
unused LCM OperationStatus and related comments.

Issue-Id: APPC-143
Change-Id: Ia2580717ef515856725e3e50c63b17404f123325
Signed-off-by: beili.zhou <beili.zhou@amdocs.com>

7 files changed, 92 insertions, 134 deletions

diff --git a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/impl/ListenerImpl.java b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/impl/ListenerImpl.java
index d0ba7fbf5..c9ac104f3 100644
--- a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/impl/ListenerImpl.java
+++ b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/impl/ListenerImpl.java
@@ -87,7 +87,6 @@ public class ListenerImpl extends AbstractListener {
                         if (isValid(incoming)) {
                             String requestIdWithSubId = getRequestIdWithSubId(incoming.getBody());
                             LOG.info("Acknowledging Message: " + requestIdWithSubId);
-//                            dmaap.postStatus(incoming.toOutgoing(OperationStatus.PENDING));
                         }
                     }
                     for (DmaapIncomingMessage incoming : messages) {
diff --git a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/model/InputBody.java b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/model/InputBody.java
index 88e515b0d..9808daf56 100644
--- a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/model/InputBody.java
+++ b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/model/InputBody.java
@@ -24,14 +24,11 @@
 
 package org.openecomp.appc.listener.LCM.model;
 
-import org.openecomp.appc.listener.LCM.operation.OperationStatus;
-
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.databind.annotation.JsonSerialize;
 
-
 @JsonSerialize(include = JsonSerialize.Inclusion.NON_NULL)
 @JsonIgnoreProperties(ignoreUnknown = true)
 public class InputBody {
@@ -85,19 +82,6 @@ public class InputBody {
         this.commonHeader = commonHeader;
     }
 
-//    public String toOutgoing(OperationStatus operationStatus) {
-//        OutputBody out = new OutputBody(this);
-//        out.setStatus(new ResponseStatus(operationStatus.getCode(), operationStatus.getValue()));
-//        return out.toResponse().toString();
-//    }
-
-//    public String toOutgoing(OperationStatus operationStatus,String islocked) {
-//        OutputBody out = new OutputBody(this);
-//        out.setStatus(new ResponseStatus(operationStatus.getCode(), operationStatus.getValue()));
-//        out.setLocked(islocked);
-//        return out.toResponse().toString();
-//    }
-
     @JsonIgnore
     public boolean isValid() {
         return getCommonHeader() != null;
diff --git a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/GenericProviderOperationRequestFormatter.java b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/GenericProviderOperationRequestFormatter.java
index e68d23bb6..dc8b5eab7 100644
--- a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/GenericProviderOperationRequestFormatter.java
+++ b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/GenericProviderOperationRequestFormatter.java
@@ -36,8 +36,6 @@ import com.att.eelf.configuration.EELFManager;
 
 import java.net.URL;
 
-
-
 public class GenericProviderOperationRequestFormatter implements ProviderOperationRequestFormatter {
 
     private final EELFLogger LOG = EELFManager.getInstance().getLogger(GenericProviderOperationRequestFormatter.class);
@@ -52,44 +50,12 @@ public class GenericProviderOperationRequestFormatter implements ProviderOperati
         return url.getPath() + ":"+rpcName;
     }
 
-//    private String convertActionToUrl(String action) {
-//        String res = "";
-//        switch (action) {
-//            case "SoftwareUpload":
-//                res = "software-upload";
-//                break;
-//            case "LiveUpgrade":
-//                res = "live-upgrade";
-//                break;
-//            case "HealthCheck":
-//                res = "health-check";
-//                break;
-//            case "CheckLock":
-//                res = "check-lock";
-//                break;
-//            default :
-//                res = action;
-//        }
-//        return res.toLowerCase();
-//    }
-
     @Override
     public String buildRequest(InputBody msg) {
         JSONObject jsonObject = Mapper.toJsonObject(msg);
         return String.format(TEMPLATE, jsonObject.toString());
     }
 
-/*    @Override
-    public OperationStatus getOperationStatus(JSONObject responseBody) throws APPCException {
-        try {
-            JSONObject status = responseBody.getJSONObject("output").getJSONObject("status");
-            return new OperationStatus(String.valueOf(status.getInt("code")), status.getString("message"));
-        } catch (Exception e) {
-            LOG.error("Unknown error processing failed response from provider. Json not in expected format", e);
-            throw new APPCException("APPC has an unknown RPC error");
-        }
-    }*/
-
     @Override
     public ResponseStatus getResponseStatus(JsonNode responseBody)throws APPCException{
         try {
diff --git a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/OperationStatus.java b/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/OperationStatus.java
deleted file mode 100644
index e60c90916..000000000
--- a/appc-event-listener/appc-event-listener-bundle/src/main/java/org/openecomp/appc/listener/LCM/operation/OperationStatus.java
+++ /dev/null
@@ -1,69 +0,0 @@
-/*-
- * ============LICENSE_START=======================================================
- * ONAP : APPC
- * ================================================================================
- * Copyright (C) 2017 AT&T Intellectual Property. All rights reserved.
- * ================================================================================
- * Copyright (C) 2017 Amdocs
- * =============================================================================
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- * 
- *      http://www.apache.org/licenses/LICENSE-2.0
- * 
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- * 
- * ECOMP is a trademark and service mark of AT&T Intellectual Property.
- * ============LICENSE_END=========================================================
- */
-
-package org.openecomp.appc.listener.LCM.operation;
-
-
-public class OperationStatus {
-    public final static OperationStatus PENDING = new OperationStatus("PENDING", "PENDING");
-    public final static OperationStatus ACTIVE = new OperationStatus("ACTIVE", "ACTIVE");
-    public final static OperationStatus SUCCESS = new OperationStatus("SUCCESS", "SUCCESS");
-    public final static OperationStatus FAILURE = new OperationStatus("FAILURE", "FAILURE");
-
-    private String code;
-    private String value;
-
-    public OperationStatus() {
-    }
-
-
-    public OperationStatus(String code, String value) {
-        this.code = code;
-        this.value = value;
-    }
-
-    public String getCode() {
-        return code;
-    }
-
-    public void setCode(String code) {
-        this.code = code;
-    }
-
-    public String getValue() {
-        return value;
-    }
-
-    public void setValue(String value) {
-        this.value = value;
-    }
-
-    public boolean isSucceeded() {
-        if (code == null) {
-            return false;
-        }
-        int intCode = Integer.parseInt(code);
-        return (intCode >= 200) && (intCode < 300); // All 2xx statuses are success
-    }
-}
diff --git a/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/BaseActionRunnable.java b/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/BaseActionRunnable.java
index 5df2c805e..2ffad6993 100644
--- a/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/BaseActionRunnable.java
+++ b/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/BaseActionRunnable.java
@@ -51,6 +51,8 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
     final String ABORT_MESSAGE_FORMAT = "Aborting %s operation.";
     /** Timeout message format with flexible operation name */
     final String TIMEOUT_MESSAGE_FORMAT = "%s operation has reached timeout %d milliseconds.";
+    /** Failure message format with flexible number of bundles */
+    final String BUNDLE_OPERATION_FAILED_FORMAT = "%d bundle(s) failed, see logs for details.";
 
     private boolean isWaiting = false;
     private AppcOamStates currentState;
@@ -65,6 +67,11 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
     BaseProcessor myParent;
     Map<String, Future<?>> bundleNameToFuture = new HashMap<>();
 
+    /**
+     * Constructor
+     *
+     * @param parent BaseProcessor who has called this constructor.
+     */
     BaseActionRunnable(BaseProcessor parent) {
         super(parent.logger, parent.configurationHelper, parent.stateHelper, parent.operationHelper);
 
@@ -76,6 +83,9 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
         setTimeoutValues();
     }
 
+    /**
+     * Set timeout in milliseconds
+     */
     void setTimeoutValues() {
         Integer timeoutSeconds = myParent.timeoutSeconds;
         if (timeoutSeconds == null) {
@@ -120,6 +130,10 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
         }
     }
 
+    /**
+     * Keep waiting to be override by children classes for different behaviors.
+     * Timeout is validated here.
+     */
     void keepWaiting() {
         logDebug(String.format("%s runnable waiting, current state is %s.",
                 actionName, currentState == null ? "null" : currentState.toString()));
@@ -127,6 +141,12 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
         isTimeout("keepWaiting");
     }
 
+    /**
+     * Check if the timeout milliseconds has reached.
+     *
+     * @param parentName String of the caller, for logging purpose.
+     * @return true if the timeout has reached, otherwise false.
+     */
     boolean isTimeout(String parentName) {
         logDebug(String.format("%s task isTimeout called from %s", actionName, parentName));
         if (doTimeoutChecking
@@ -142,6 +162,23 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
     }
 
     /**
+     * Check if all bundle operations are successful through BundleHelper.
+     * If there's failed bundler operation, set error status and trigger postAction with Error state.
+     *
+     * @return true if bundler operations have failure, otherwise false.
+     */
+    boolean hasBundleOperationFailure() {
+        long failedTask = myParent.bundleHelper.getFailedMetrics(bundleNameToFuture);
+        if (failedTask == 0) {
+            return false;
+        }
+
+        setStatus(OAMCommandStatus.UNEXPECTED_ERROR, String.format(BUNDLE_OPERATION_FAILED_FORMAT, failedTask));
+        postAction(AppcOamStates.Error);
+        return true;
+    }
+
+    /**
      * Set class <b>status</b> to REJECTED with abort message.
      */
     void setAbortStatus() {
@@ -150,6 +187,7 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
 
     /**
      * Final handling. The thread is cancelled.
+     *
      * @param setState boolean to indicate if set OAM state or not
      */
     void postDoAction(boolean setState) {
@@ -157,7 +195,8 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
     }
 
     /**
-     * Handling for after doAction. does post notification,  issue audit log and set OAM state based on input
+     * Handling for after doAction. does post notification, issue audit log and set OAM state based on input.
+     *
      * @param state of AppcOamState to be set as OAM state when it is not null.
      */
     void postAction(AppcOamStates state) {
@@ -174,6 +213,7 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
 
     /**
      * Check state
+     *
      * @return true if final state reached, otherwise return false
      */
     boolean checkState() {
@@ -186,11 +226,7 @@ abstract class BaseActionRunnable extends BaseCommon implements Runnable {
             return false;
         }
 
-        long failedTask = myParent.bundleHelper.getFailedMetrics(bundleNameToFuture);
-        if (failedTask != 0) {
-            String errorMsg = failedTask + " bundle(s) failed, see logs for details.";
-            setStatus(OAMCommandStatus.UNEXPECTED_ERROR, errorMsg);
-            postAction(AppcOamStates.Error);
+        if (hasBundleOperationFailure()) {
             return true;
         }
 
diff --git a/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/OamRestartProcessor.java b/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/OamRestartProcessor.java
index 529d25004..e9f0ada56 100644
--- a/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/OamRestartProcessor.java
+++ b/appc-oam/appc-oam-bundle/src/main/java/org/openecomp/appc/oam/processor/OamRestartProcessor.java
@@ -45,14 +45,14 @@ public class OamRestartProcessor extends BaseProcessor {
      * <br> -Stopped: check if all bundle state reached stopped
      * <br> -ToStart: call bundles start
      * <br> -Started: action is full completed
-     * <br> -Timeout: indication of timeout reached
+     * <br> -Error: indication of error, such as timeout reached, bundler operation failure and etc.
      */
     private enum ActionPhases {
         ToStop,
         Stopped,
         ToStart,
         Started,
-        Timeout
+        Error
     }
 
     /**
@@ -137,7 +137,7 @@ public class OamRestartProcessor extends BaseProcessor {
                                 AppcOam.RPC.start, bundleNameToFuture, myParent.asyncTaskHelper);
                         currentPhase = ActionPhases.Started;
                         break;
-                    case Timeout:
+                    case Error:
                         // do nothing
                         break;
                     default:
@@ -148,8 +148,9 @@ public class OamRestartProcessor extends BaseProcessor {
                         return false;
                 }
 
-                if (isTimeout("restart doAction")) {
-                    currentPhase = ActionPhases.Timeout;
+                if (isTimeout("restart doAction")
+                        || hasBundleOperationFailure()) {
+                    currentPhase = ActionPhases.Error;
                     return true;
                 }
                 if (isBundleOperationCompleted) {
diff --git a/appc-oam/appc-oam-bundle/src/test/java/org/openecomp/appc/oam/processor/BaseActionRunnableTest.java b/appc-oam/appc-oam-bundle/src/test/java/org/openecomp/appc/oam/processor/BaseActionRunnableTest.java
index 5fd51ef11..70adca0fc 100644
--- a/appc-oam/appc-oam-bundle/src/test/java/org/openecomp/appc/oam/processor/BaseActionRunnableTest.java
+++ b/appc-oam/appc-oam-bundle/src/test/java/org/openecomp/appc/oam/processor/BaseActionRunnableTest.java
@@ -36,6 +36,7 @@ import org.openecomp.appc.i18n.Msg;
 import org.openecomp.appc.oam.AppcOam;
 import org.openecomp.appc.oam.OAMCommandStatus;
 import org.openecomp.appc.oam.util.AsyncTaskHelper;
+import org.openecomp.appc.oam.util.BundleHelper;
 import org.openecomp.appc.oam.util.ConfigurationHelper;
 import org.openecomp.appc.oam.util.OperationHelper;
 import org.openecomp.appc.oam.util.StateHelper;
@@ -46,6 +47,7 @@ import java.util.Date;
 
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.anyInt;
+import static org.mockito.Matchers.anyMap;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
@@ -101,6 +103,7 @@ public class BaseActionRunnableTest {
     private OperationHelper mockOperHelper = mock(OperationHelper.class);
     private ConfigurationHelper mockConfigHelper = mock(ConfigurationHelper.class);
     private Configuration mockConfig = mock(Configuration.class);
+    private BundleHelper mockBundleHelper = mock(BundleHelper.class);
 
     @SuppressWarnings("ResultOfMethodCallIgnored")
     @Before
@@ -113,8 +116,9 @@ public class BaseActionRunnableTest {
 
         testProcessor = spy(
                 new TestProcessor(mockLogger, mockConfigHelper, mockStateHelper, null, mockOperHelper));
-        testBaseAcionRunnable = spy(new TestAbc(testProcessor));
+        Whitebox.setInternalState(testProcessor, "bundleHelper", mockBundleHelper);
 
+        testBaseAcionRunnable = spy(new TestAbc(testProcessor));
         Whitebox.setInternalState(testBaseAcionRunnable, "commonHeader", mock(CommonHeader.class));
     }
 
@@ -163,13 +167,13 @@ public class BaseActionRunnableTest {
         Whitebox.setInternalState(testBaseAcionRunnable, "doActionResult", true);
 
         // with checkState return true
-        Mockito.doReturn(targetState).when(mockStateHelper).getBundlesState();
+        Mockito.doReturn(true).when(testBaseAcionRunnable).checkState();
         testBaseAcionRunnable.run();
         Assert.assertFalse("isWaiting should still be false",
                 Whitebox.getInternalState(testBaseAcionRunnable, "isWaiting"));
 
         // with checkState return false
-        Mockito.doReturn(AppcOamStates.Started).when(mockStateHelper).getBundlesState();
+        Mockito.doReturn(false).when(testBaseAcionRunnable).checkState();
         testBaseAcionRunnable.run();
         Assert.assertTrue("isWaiting should still be true",
                 Whitebox.getInternalState(testBaseAcionRunnable, "isWaiting"));
@@ -191,6 +195,27 @@ public class BaseActionRunnableTest {
 
     @Test
     public void testCheckState() throws Exception {
+        // 1. with isTimeout true
+        Mockito.doReturn(true).when(testBaseAcionRunnable).isTimeout("checkState");
+        Assert.assertTrue("Should return true", testBaseAcionRunnable.checkState());
+
+        // 2. with isTimeout false and
+        Mockito.doReturn(false).when(testBaseAcionRunnable).isTimeout("checkState");
+
+        // 2.1 with task not all done
+        Mockito.doReturn(false).when(mockBundleHelper).isAllTaskDone(any());
+        Assert.assertFalse("Should return false", testBaseAcionRunnable.checkState());
+
+        // 2. 2 with task all done
+        Mockito.doReturn(true).when(mockBundleHelper).isAllTaskDone(any());
+
+        // 2.2.1 with has bundle failure
+        Mockito.doReturn(true).when(testBaseAcionRunnable).hasBundleOperationFailure();
+        Assert.assertTrue("Should return true", testBaseAcionRunnable.checkState());
+
+        // 2.2.2 with no bundle failure
+        Mockito.doReturn(false).when(testBaseAcionRunnable).hasBundleOperationFailure();
+
         Mockito.doReturn(targetState).when(mockStateHelper).getBundlesState();
         Assert.assertTrue("Should return true", testBaseAcionRunnable.checkState());
 
@@ -240,4 +265,20 @@ public class BaseActionRunnableTest {
                 testBaseAcionRunnable.status.getMessage().endsWith(
                         String.format(testBaseAcionRunnable.TIMEOUT_MESSAGE_FORMAT, testRpc.name(), timeoutMs)));
     }
+
+    @SuppressWarnings("unchecked")
+    @Test
+    public void testHasBundleOperationFailure() throws Exception {
+        Mockito.when(mockBundleHelper.getFailedMetrics(anyMap())).thenReturn(Long.valueOf("0"));
+        Assert.assertFalse("should return false", testBaseAcionRunnable.hasBundleOperationFailure());
+
+        Mockito.when(mockStateHelper.getCurrentOamState()).thenReturn(AppcOamStates.Restarting);
+        long failedNumber = 1;
+        Mockito.doReturn(failedNumber).when(mockBundleHelper).getFailedMetrics(anyMap());
+        Assert.assertTrue("should return true", testBaseAcionRunnable.hasBundleOperationFailure());
+        Mockito.verify(testBaseAcionRunnable,
+                times(1)).setStatus(OAMCommandStatus.UNEXPECTED_ERROR,
+                String.format(testBaseAcionRunnable.BUNDLE_OPERATION_FAILED_FORMAT, failedNumber));
+        Mockito.verify(testBaseAcionRunnable, times(1)).postAction(AppcOamStates.Error);
+    }
 }