From fc556cec79885034897d8647b957125c6fc95b5e Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Thu, 7 Nov 2019 16:37:18 -0500 Subject: 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 Change-Id: I15821d299bc3261478fd7fbb9ee62ea4a90123a4 --- .../policy/pap/main/comm/PdpModifyRequestMap.java | 77 +++++------- .../pap/main/comm/PdpModifyRequestMapTest.java | 130 ++++++++++++--------- 2 files changed, 105 insertions(+), 102 deletions(-) (limited to 'main/src') 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 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 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 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(); } @@ -608,6 +617,15 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { getListener(getSingletons(count).get(0)).failure(PDP1, MY_REASON); } + /** + * 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. * -- cgit 1.2.3-korg