From f76d2083c9440bac3ac866b30b52a3abedb9292c Mon Sep 17 00:00:00 2001 From: Jim Hahn Date: Wed, 23 Jun 2021 16:27:20 -0400 Subject: Send pdp-update if PDP response doesn't match DB Because multiple PAPs can be updating the DB, it's possible that a pdp-update sent by a PAP does not reflect the latest deployment data in the DB. To solve that problem, modified code to compare any response received from a PDP with what's in the DB, potentially generating a new pdp-update (and/or pdp-state-change). Issue-ID: POLICY-3426 Change-Id: I241994330d7645c0fffe66abc33de67d71d77250 Signed-off-by: Jim Hahn --- .../policy/pap/main/comm/PdpModifyRequestMap.java | 23 ++++- .../pap/main/comm/PdpStatusMessageHandler.java | 45 ++++++++- .../policy/pap/main/comm/msgdata/RequestImpl.java | 2 +- .../pap/main/comm/msgdata/RequestListener.java | 5 +- .../policy/pap/main/startstop/PapActivator.java | 2 +- .../pap/main/comm/PdpModifyRequestMapTest.java | 22 ++++- .../pap/main/comm/PdpStatusMessageHandlerTest.java | 101 +++++++++++++++++++++ .../pap/main/comm/msgdata/RequestImplTest.java | 16 ++-- 8 files changed, 199 insertions(+), 17 deletions(-) create mode 100644 main/src/test/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandlerTest.java 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 087aa577..bf9f290a 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 @@ -39,6 +39,7 @@ import org.onap.policy.models.pdp.concepts.PdpGroup; import org.onap.policy.models.pdp.concepts.PdpGroupFilter; import org.onap.policy.models.pdp.concepts.PdpMessage; import org.onap.policy.models.pdp.concepts.PdpStateChange; +import org.onap.policy.models.pdp.concepts.PdpStatus; import org.onap.policy.models.pdp.concepts.PdpSubGroup; import org.onap.policy.models.pdp.concepts.PdpUpdate; import org.onap.policy.models.pdp.enums.PdpState; @@ -350,6 +351,14 @@ public class PdpModifyRequestMap { return new PdpRequests(pdpName, policyNotifier); } + /** + * Makes a handler for PDP responses. + * @return a response handler + */ + protected PdpStatusMessageHandler makePdpResponseHandler() { + return new PdpStatusMessageHandler(params.getParams()); + } + /** * Listener for singleton request events. */ @@ -397,8 +406,20 @@ public class PdpModifyRequestMap { } @Override - public void success(String responsePdpName) { + public void success(String responsePdpName, PdpStatus response) { requestCompleted(responsePdpName); + + if (!(request instanceof UpdateReq)) { + // other response types may not include the list of policies + return; + } + + /* + * Update PDP time stamps. Also send pdp-update and pdp-state-change, as + * necessary, if the response does not reflect what's in the DB. + */ + var handler = makePdpResponseHandler(); + handler.handlePdpStatus(response); } /** diff --git a/main/src/main/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandler.java b/main/src/main/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandler.java index 897a0049..b430bb33 100644 --- a/main/src/main/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandler.java +++ b/main/src/main/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandler.java @@ -22,8 +22,10 @@ package org.onap.policy.pap.main.comm; +import java.sql.SQLIntegrityConstraintViolationException; import java.time.Instant; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -32,6 +34,7 @@ import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.apache.commons.lang3.builder.EqualsBuilder; +import org.eclipse.persistence.exceptions.EclipseLinkException; import org.onap.policy.models.base.PfModelException; import org.onap.policy.models.pdp.concepts.Pdp; import org.onap.policy.models.pdp.concepts.PdpGroup; @@ -90,6 +93,10 @@ public class PdpStatusMessageHandler extends PdpMessageGenerator { * @param message the PdpStatus message */ public void handlePdpStatus(final PdpStatus message) { + if (message.getPolicies() == null) { + message.setPolicies(Collections.emptyList()); + } + long diffms = System.currentTimeMillis() - message.getTimestampMs(); if (diffms > params.getMaxMessageAgeMs()) { long diffsec = TimeUnit.SECONDS.convert(diffms, TimeUnit.MILLISECONDS); @@ -107,9 +114,45 @@ public class PdpStatusMessageHandler extends PdpMessageGenerator { } catch (final PolicyPapException exp) { LOGGER.error("Operation Failed", exp); } catch (final Exception exp) { - LOGGER.error("Failed connecting to database provider", exp); + if (isDuplicateKeyException(exp)) { + /* + * this is to be expected, if multiple PAPs are processing the same + * heartbeat at a time, thus we log the exception at a trace level + * instead of an error level. + */ + LOGGER.info("Failed updating PDP information for {} - may have been added by another PAP", + message.getName()); + LOGGER.trace("Failed updating PDP information for {}", message.getName(), exp); + } else { + LOGGER.error("Failed connecting to database provider", exp); + } + } + } + } + + /** + * Determines if the exception indicates a duplicate key. + * + * @param thrown exception to check + * @return {@code true} if the exception occurred due to a duplicate key + */ + protected static boolean isDuplicateKeyException(Throwable thrown) { + while (thrown != null) { + if (thrown instanceof SQLIntegrityConstraintViolationException) { + return true; + } + + if (thrown instanceof EclipseLinkException) { + EclipseLinkException ele = (EclipseLinkException) thrown; + if (isDuplicateKeyException(ele.getInternalException())) { + return true; + } } + + thrown = thrown.getCause(); } + + return false; } private void handlePdpRegistration(final PdpStatus message, final PolicyModelsProvider databaseProvider) diff --git a/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestImpl.java b/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestImpl.java index 31b2df02..ff161469 100644 --- a/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestImpl.java +++ b/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestImpl.java @@ -272,7 +272,7 @@ public abstract class RequestImpl implements Request { } logger.info("{} successful", getName()); - listener.success(pdpName); + listener.success(pdpName, response); } } diff --git a/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestListener.java b/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestListener.java index 13ebfe3c..8fa2b950 100644 --- a/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestListener.java +++ b/main/src/main/java/org/onap/policy/pap/main/comm/msgdata/RequestListener.java @@ -20,6 +20,8 @@ package org.onap.policy.pap.main.comm.msgdata; +import org.onap.policy.models.pdp.concepts.PdpStatus; + /** * Listener for request events. */ @@ -37,8 +39,9 @@ public interface RequestListener { * Indicates that a successful response was received from a PDP. * * @param pdpName name of the PDP from which the response was received + * @param response successful response */ - public void success(String pdpName); + public void success(String pdpName, PdpStatus response); /** * Indicates that the retry count was exhausted. diff --git a/main/src/main/java/org/onap/policy/pap/main/startstop/PapActivator.java b/main/src/main/java/org/onap/policy/pap/main/startstop/PapActivator.java index 78a301f6..ed73c3e1 100644 --- a/main/src/main/java/org/onap/policy/pap/main/startstop/PapActivator.java +++ b/main/src/main/java/org/onap/policy/pap/main/startstop/PapActivator.java @@ -234,7 +234,7 @@ public class PapActivator extends ServiceManagerContainer { frequencyMs, TimeUnit.MILLISECONDS); }, - () -> pdpExpirationTimer.get().shutdownNow()); + () -> pdpExpirationTimer.get().shutdown()); addAction("PAP client executor", () -> 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 0e9be09f..fd5ff86e 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 @@ -101,6 +101,9 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { @Mock private PolicyUndeployer undeployer; + @Mock + private PdpStatusMessageHandler responseHandler; + private MyMap map; private PdpUpdate update; private PdpStateChange change; @@ -149,9 +152,10 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { assertFalse(map.isEmpty()); // indicate success - getListener(getSingletons(1).get(0)).success(PDP1); + getListener(getSingletons(1).get(0)).success(PDP1, response); assertTrue(map.isEmpty()); + verify(responseHandler, never()).handlePdpStatus(response); } @Test @@ -325,7 +329,9 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { map.addRequest(change); // indicate success - getListener(getSingletons(1).get(0)).success(PDP1); + getListener(getSingletons(1).get(0)).success(PDP1, response); + + verify(responseHandler, never()).handlePdpStatus(response); /* * the above should have removed the requests so next time should allocate a new @@ -344,7 +350,10 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { // indicate success with the update when(requests.startNextRequest(updateReq)).thenReturn(true); - getListener(updateReq).success(PDP1); + getListener(updateReq).success(PDP1, response); + + // should be called for the update + verify(responseHandler).handlePdpStatus(response); // should have started the next request verify(requests).startNextRequest(updateReq); @@ -654,7 +663,7 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { * @param count expected number of requests */ private void invokeSuccessHandler(int count) { - getListener(getSingletons(count).get(0)).success(PDP1); + getListener(getSingletons(count).get(0)).success(PDP1, response); } /** @@ -764,5 +773,10 @@ public class PdpModifyRequestMapTest extends CommonRequestBase { ++nalloc; return requests; } + + @Override + protected PdpStatusMessageHandler makePdpResponseHandler() { + return responseHandler; + } } } diff --git a/main/src/test/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandlerTest.java b/main/src/test/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandlerTest.java new file mode 100644 index 00000000..5129bf5f --- /dev/null +++ b/main/src/test/java/org/onap/policy/pap/main/comm/PdpStatusMessageHandlerTest.java @@ -0,0 +1,101 @@ +/*- + * ============LICENSE_START======================================================= + * ONAP + * ================================================================================ + * Copyright (C) 2021 AT&T Intellectual Property. All rights reserved. + * ================================================================================ + * 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. + * ============LICENSE_END========================================================= + */ + +package org.onap.policy.pap.main.comm; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.sql.SQLIntegrityConstraintViolationException; +import org.eclipse.persistence.exceptions.EclipseLinkException; +import org.junit.Test; + +public class PdpStatusMessageHandlerTest { + + @Test + public void testIsDuplicateKeyException() { + + // @formatter:off + + // null exception + assertThat(PdpStatusMessageHandler.isDuplicateKeyException(null)).isFalse(); + + // plain exception + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new Exception())) + .isFalse(); + + // cause is also plain + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new Exception( + new Exception()))) + .isFalse(); + + // dup key + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new SQLIntegrityConstraintViolationException())) + .isTrue(); + + // cause is dup key + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new Exception( + new SQLIntegrityConstraintViolationException()))) + .isTrue(); + + // eclipselink exception, no internal exception + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new MyEclipseLinkException())) + .isFalse(); + + // eclipselink exception, cause is plain + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new MyEclipseLinkException( + new Exception()))) + .isFalse(); + + // eclipselink exception, cause is dup + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new MyEclipseLinkException( + new SQLIntegrityConstraintViolationException()))) + .isTrue(); + + // multiple cause both inside and outside of the eclipselink exception + assertThat(PdpStatusMessageHandler.isDuplicateKeyException( + new Exception( + new Exception( + new MyEclipseLinkException( + new Exception( + new SQLIntegrityConstraintViolationException())))))) + .isTrue(); + + // @formatter:on + } + + public static class MyEclipseLinkException extends EclipseLinkException { + private static final long serialVersionUID = 1L; + + public MyEclipseLinkException() { + // do nothing + } + + public MyEclipseLinkException(Exception exception) { + setInternalException(exception); + } + } +} diff --git a/main/src/test/java/org/onap/policy/pap/main/comm/msgdata/RequestImplTest.java b/main/src/test/java/org/onap/policy/pap/main/comm/msgdata/RequestImplTest.java index abce7eb3..dd635627 100644 --- a/main/src/test/java/org/onap/policy/pap/main/comm/msgdata/RequestImplTest.java +++ b/main/src/test/java/org/onap/policy/pap/main/comm/msgdata/RequestImplTest.java @@ -292,7 +292,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener).success(PDP1); + verify(listener).success(PDP1, response); verify(listener, never()).failure(any(), any()); verify(timer).cancel(); } @@ -305,7 +305,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener, never()).success(any()); + verify(listener, never()).success(any(), any()); verify(listener, never()).failure(any(), any()); } @@ -317,7 +317,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener, never()).success(any()); + verify(listener, never()).success(any(), any()); verify(listener, never()).failure(any(), any()); verify(timer, never()).cancel(); } @@ -330,7 +330,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener, never()).success(any()); + verify(listener, never()).success(any(), any()); verify(listener).failure(DIFFERENT, "PDP name does not match"); verify(timer).cancel(); } @@ -390,7 +390,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener).success(PDP1); + verify(listener).success(PDP1, response); verify(listener, never()).failure(any(), any()); } @@ -402,7 +402,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener, never()).success(any()); + verify(listener, never()).success(any(), any()); verify(listener).failure(null, "null PDP name"); } @@ -414,7 +414,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener, never()).success(any()); + verify(listener, never()).success(any(), any()); verify(listener).failure(DIFFERENT, "PDP name does not match"); } @@ -427,7 +427,7 @@ public class RequestImplTest extends CommonRequestBase { invokeProcessResponse(response); - verify(listener).success(DIFFERENT); + verify(listener).success(DIFFERENT, response); verify(listener, never()).failure(any(), any()); } -- cgit 1.2.3-korg