PAP should not deactivate PDPs

Modified the code so that it does not send a PASSIVE request to
PDPs when they are fail, whether due to an inability to deploy a
policy or due to a timeout.  However, it still removes the PDP from
the PDP Group, if a timeout occurs (but not due to a deployment
failure).

This review does not include any changes to undeploy failed policies;
that will come in a later review.

Issue-ID: POLICY-2155
Signed-off-by: Jim Hahn <jrh3@att.com>
Change-Id: I15821d299bc3261478fd7fbb9ee62ea4a90123a4

2 files changed, 105 insertions, 102 deletions

diff --git a/main/src/main/java/org/onap/policy/pap/main/comm/PdpModifyRequestMap.java b/main/src/main/java/org/onap/policy/pap/main/comm/PdpModifyRequestMap.java
index 6f763204..49ded962 100644
--- a/main/src/main/java/org/onap/policy/pap/main/comm/PdpModifyRequestMap.java
+++ b/main/src/main/java/org/onap/policy/pap/main/comm/PdpModifyRequestMap.java
@@ -21,7 +21,6 @@
 package org.onap.policy.pap.main.comm;
 
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -326,27 +325,36 @@ public class PdpModifyRequestMap {
     private class SingletonListener implements RequestListener {
         private final PdpRequests requests;
         private final Request request;
+        private final String pdpName;
 
         public SingletonListener(PdpRequests requests, Request request) {
             this.requests = requests;
             this.request = request;
+            this.pdpName = requests.getPdpName();
         }
 
         @Override
-        public void failure(String pdpName, String reason) {
-            if (requests.getPdpName().equals(pdpName)) {
-                disablePdp(requests);
-            }
+        public void failure(String responsePdpName, String reason) {
+            requestCompleted(responsePdpName);
         }
 
         @Override
-        public void success(String pdpName) {
-            if (requests.getPdpName().equals(pdpName)) {
-                if (pdp2requests.get(requests.getPdpName()) == requests) {
-                    startNextRequest(requests, request);
+        public void success(String responsePdpName) {
+            requestCompleted(responsePdpName);
+        }
+
+        /**
+         * Handles a request completion, starting the next request, if there is one.
+         *
+         * @param responsePdpName name of the PDP provided in the response
+         */
+        private void requestCompleted(String responsePdpName) {
+            if (pdpName.equals(responsePdpName)) {
+                if (pdp2requests.get(pdpName) == requests) {
+                    startNextRequest(request);
 
                 } else {
-                    logger.info("discard old requests for {}", pdpName);
+                    logger.info("discard old requests for {}", responsePdpName);
                     requests.stopPublishing();
                 }
             }
@@ -354,65 +362,42 @@ public class PdpModifyRequestMap {
 
         @Override
         public void retryCountExhausted() {
-            disablePdp(requests);
+            removePdp();
         }
 
         /**
          * Starts the next request associated with a PDP.
          *
-         * @param requests current set of requests
          * @param request the request that just completed
          */
-        private void startNextRequest(PdpRequests requests, Request request) {
+        private void startNextRequest(Request request) {
             if (!requests.startNextRequest(request)) {
-                pdp2requests.remove(requests.getPdpName(), requests);
+                pdp2requests.remove(pdpName, requests);
             }
         }
 
         /**
-         * Disables a PDP by removing it from its subgroup and then sending it a PASSIVE
-         * request.
-         *
-         * @param requests the requests associated with the PDP to be disabled
+         * Removes a PDP from its subgroup.
          */
-        private void disablePdp(PdpRequests requests) {
-
-            policyNotifier.removePdp(requests.getPdpName());
+        private void removePdp() {
+            requests.stopPublishing();
 
             // remove the requests from the map
-            if (!pdp2requests.remove(requests.getPdpName(), requests)) {
-                // don't have the info we need to disable it
-                logger.warn("no requests with which to disable {}", requests.getPdpName());
+            if (!pdp2requests.remove(pdpName, requests)) {
+                // wasn't in the map - the requests must be old
+                logger.warn("discarding old requests for {}", pdpName);
                 return;
             }
 
-            logger.warn("disabling {}", requests.getPdpName());
+            logger.warn("removing {}", pdpName);
 
-            requests.stopPublishing();
+            policyNotifier.removePdp(pdpName);
 
             // remove the PDP from all groups
-            boolean removed = false;
             try {
-                removed = removeFromGroups(requests.getPdpName());
+                removeFromGroups(pdpName);
             } catch (PfModelException e) {
-                logger.info("unable to remove PDP {} from subgroup", requests.getPdpName(), e);
-            }
-
-            // send the state change
-            PdpStateChange change = new PdpStateChange();
-            change.setName(requests.getPdpName());
-            change.setState(PdpState.PASSIVE);
-
-            if (removed) {
-                // send an update, too
-                PdpUpdate update = new PdpUpdate();
-                update.setName(requests.getPdpName());
-                update.setPolicies(Collections.emptyList());
-
-                addRequest(update, change);
-
-            } else {
-                addRequest(change);
+                logger.info("unable to remove PDP {} from subgroup", pdpName, e);
             }
         }
     }
diff --git a/main/src/test/java/org/onap/policy/pap/main/comm/PdpModifyRequestMapTest.java b/main/src/test/java/org/onap/policy/pap/main/comm/PdpModifyRequestMapTest.java
index e90566eb..74f8b392 100644
--- a/main/src/test/java/org/onap/policy/pap/main/comm/PdpModifyRequestMapTest.java
+++ b/main/src/test/java/org/onap/policy/pap/main/comm/PdpModifyRequestMapTest.java
@@ -24,7 +24,6 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.any;
@@ -335,7 +334,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
     }
 
     @Test
-    public void testDisablePdp() throws Exception {
+    public void testRemovePdp() throws Exception {
         map.addRequest(update);
 
         // put the PDP in a group
@@ -344,8 +343,8 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
 
         when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
 
-        // indicate failure
-        invokeFailureHandler(1);
+        // indicate retries exhausted
+        invokeLastRetryHandler(1);
 
         // should have stopped publishing
         verify(requests).stopPublishing();
@@ -353,66 +352,58 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
         // should have generated a notification
         verify(notifier).removePdp(PDP1);
 
-        // should have published a state-change
-        PdpMessage msg2 = getSingletons(3).get(1).getMessage();
-        assertNotNull(msg2);
-        assertTrue(msg2 instanceof PdpStateChange);
-
-        change = (PdpStateChange) msg2;
-        assertEquals(PDP1, change.getName());
-        assertEquals(PdpState.PASSIVE, change.getState());
-
-        // should have published a new update
-        msg2 = getSingletons(3).get(2).getMessage();
-        assertNotNull(msg2);
-        assertTrue(msg2 instanceof PdpUpdate);
-
-        // update should have null group & subgroup
-        update = (PdpUpdate) msg2;
-        assertEquals(PDP1, update.getName());
-        assertNull(update.getPdpGroup());
-        assertNull(update.getPdpSubgroup());
+        // should have removed from the group
+        List<PdpGroup> groups = getGroupUpdates();
+        assertEquals(1, groups.size());
+        assertSame(group, groups.get(0));
+        assertEquals(0, group.getPdpSubgroups().get(0).getCurrentInstanceCount());
     }
 
     @Test
-    public void testDisablePdp_NotInGroup() {
+    public void testRemovePdp_NotInGroup() throws PfModelException {
         map.addRequest(update);
 
-        // indicate failure
-        invokeFailureHandler(1);
+        // indicate retries exhausted
+        invokeLastRetryHandler(1);
 
         // should have stopped publishing
         verify(requests).stopPublishing();
 
-        // should have published a new state-change
-        PdpMessage msg2 = getSingletons(2).get(1).getMessage();
-        assertNotNull(msg2);
-        assertTrue(msg2 instanceof PdpStateChange);
-
-        change = (PdpStateChange) msg2;
-        assertEquals(PDP1, change.getName());
-        assertEquals(PdpState.PASSIVE, change.getState());
+        // should not have done any updates
+        verify(dao, never()).updatePdpGroups(any());
     }
 
     @Test
-    public void testDisablePdp_AlreadyRemoved() {
+    public void testRemovePdp_AlreadyRemovedFromMap() throws PfModelException {
         map.addRequest(change);
         map.stopPublishing(PDP1);
 
-        invokeFailureHandler(1);
+        // put the PDP in a group
+        PdpGroup group = makeGroup(MY_GROUP);
+        group.setPdpSubgroups(Arrays.asList(makeSubGroup(MY_SUBGROUP, PDP1)));
 
-        // should not have stopped publishing a second time
-        verify(requests, times(1)).stopPublishing();
+        when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
+
+        invokeLastRetryHandler(1);
+
+        // should have stopped publishing a second time
+        verify(requests, times(2)).stopPublishing();
+
+        // should NOT have done any updates
+        verify(dao, never()).updatePdpGroups(any());
     }
 
     @Test
-    public void testDisablePdp_NoGroup() {
+    public void testRemovePdp_NoGroup() throws PfModelException {
         map.addRequest(change);
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         // should not have stopped publishing
         verify(requests).stopPublishing();
+
+        // should not have done any updates
+        verify(dao, never()).updatePdpGroups(any());
     }
 
     @Test
@@ -425,7 +416,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
 
         when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         // verify that the PDP was removed from the subgroup
         List<PdpGroup> groups = getGroupUpdates();
@@ -445,7 +436,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
 
         when(dao.getFilteredPdpGroups(any())).thenThrow(new PfModelException(Status.BAD_REQUEST, "expected exception"));
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         // should still stop publishing
         verify(requests).stopPublishing();
@@ -459,7 +450,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
     public void testRemoveFromGroup_NoGroups() throws Exception {
         map.addRequest(change);
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         verify(dao, never()).updatePdpGroups(any());
     }
@@ -473,7 +464,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
 
         when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         verify(dao, never()).updatePdpGroups(any());
     }
@@ -487,7 +478,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
 
         when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
 
-        invokeFailureHandler(1);
+        invokeLastRetryHandler(1);
 
         // verify that the PDP was removed from the subgroup
         List<PdpGroup> groups = getGroupUpdates();
@@ -519,22 +510,29 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
         // invoke the method
         invokeFailureHandler(1);
 
-        verify(requests).stopPublishing();
+        verify(requests, never()).stopPublishing();
+
+        // requests should have been removed from the map so this should allocate another
+        map.addRequest(update);
+        assertEquals(2, map.nalloc);
     }
 
     @Test
-    public void testSingletonListenerFailure_WrongPdpName() throws Exception {
+    public void testSingletonListenerSuccess() throws Exception {
         map.addRequest(change);
 
-        // invoke the method - has wrong PDP name
-        when(requests.getPdpName()).thenReturn(DIFFERENT);
-        invokeFailureHandler(1);
+        // invoke the method
+        invokeSuccessHandler(1);
 
         verify(requests, never()).stopPublishing();
+
+        // requests should have been removed from the map so this should allocate another
+        map.addRequest(update);
+        assertEquals(2, map.nalloc);
     }
 
     @Test
-    public void testSingletonListenerSuccess_LastRequest() throws Exception {
+    public void testRequestCompleted_LastRequest() throws Exception {
         map.addRequest(change);
 
         // invoke the method
@@ -548,11 +546,19 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
     }
 
     @Test
-    public void testSingletonListenerSuccess_NameMismatch() throws Exception {
+    public void testRequestCompleted_NameMismatch() throws Exception {
+        // use a different name
+        when(requests.getPdpName()).thenReturn(DIFFERENT);
+
         map.addRequest(change);
 
-        // invoke the method - with a different name
-        when(requests.getPdpName()).thenReturn(DIFFERENT);
+        // put the PDP in a group
+        PdpGroup group = makeGroup(MY_GROUP);
+        group.setPdpSubgroups(Arrays.asList(makeSubGroup(MY_SUBGROUP, PDP1, DIFFERENT)));
+
+        when(dao.getFilteredPdpGroups(any())).thenReturn(Arrays.asList(group));
+
+        // invoke the method - with a different name (i.e., PDP1 instead of DIFFERENT)
         invokeSuccessHandler(1);
 
         verify(requests, never()).stopPublishing();
@@ -560,10 +566,13 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
         // no effect on the map
         map.addRequest(update);
         assertEquals(1, map.nalloc);
+
+        // no updates
+        verify(dao, never()).updatePdpGroups(any());
     }
 
     @Test
-    public void testSingletonListenerSuccess_AlreadyStopped() throws Exception {
+    public void testRequestCompleted_AlreadyStopped() throws Exception {
         map.addRequest(change);
 
         map.stopPublishing(PDP1);
@@ -584,7 +593,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
         map.addRequest(change);
 
         // invoke the method
-        getListener(getSingletons(1).get(0)).retryCountExhausted();
+        invokeLastRetryHandler(1);
 
         verify(requests).stopPublishing();
     }
@@ -609,6 +618,15 @@ public class PdpModifyRequestMapTest extends CommonRequestBase {
     }
 
     /**
+     * Invokes the first request's listener.retryCountExhausted() method.
+     *
+     * @param count expected number of requests
+     */
+    private void invokeLastRetryHandler(int count) {
+        getListener(getSingletons(count).get(0)).retryCountExhausted();
+    }
+
+    /**
      * Gets the name of the PDPs contained within a subgroup.
      *
      * @param subgroup subgroup of interest