Add time limit to local guard locking facility

Modified the local policy guard locking facility to add
a time parameter.
Modified the control loop event manager to extend the lock
when lockCurrentOperation() is re-invoked.
Modified the rules to retract the lock if the lock request
was denied.
Reorder assertions in junit test.

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

5 files changed, 75 insertions, 70 deletions

diff --git a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager.java b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager.java
index bc9d3df79..5f36bcc20 100644
--- a/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager.java
+++ b/controlloop/common/eventmanager/src/main/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManager.java
@@ -60,6 +60,12 @@ public class ControlLoopEventManager implements LockCallback, Serializable {
     private static final String GENERIC_VNF_IS_CLOSED_LOOP_DISABLED = "generic-vnf.is-closed-loop-disabled";
     private static final String VSERVER_IS_CLOSED_LOOP_DISABLED = "vserver.is-closed-loop-disabled";
 
+    /**
+     * Additional time, in seconds, to add to a "lock" request. This ensures that the lock
+     * won't expire right before an operation completes.
+     */
+    private static final int ADDITIONAL_LOCK_SEC = 60;
+
     private static final Logger logger = LoggerFactory.getLogger(ControlLoopEventManager.class);
 
     private static final long serialVersionUID = -1216568161322872641L;
@@ -447,14 +453,17 @@ public class ControlLoopEventManager implements LockCallback, Serializable {
             // TODO: Make sure the current lock is for the same target.
             // Currently, it should be. But in the future it may not.
             //
-            return new LockResult<>(GuardResult.LOCK_ACQUIRED, this.targetLock);
+            GuardResult result = PolicyGuard.lockTarget(targetLock,
+                            this.currentOperation.getOperationTimeout() + ADDITIONAL_LOCK_SEC);
+            return new LockResult<>(result, this.targetLock);
         } else {
             //
             // Ask the Guard
             //
             LockResult<GuardResult, TargetLock> lockResult =
                     PolicyGuard.lockTarget(this.currentOperation.policy.getTarget().getType(),
-                            this.currentOperation.getTargetEntity(), this.onset.getRequestId(), this);
+                            this.currentOperation.getTargetEntity(), this.onset.getRequestId(), this,
+                            this.currentOperation.getOperationTimeout() + ADDITIONAL_LOCK_SEC);
             //
             // Was it acquired?
             //
diff --git a/controlloop/common/eventmanager/src/test/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManagerTest.java b/controlloop/common/eventmanager/src/test/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManagerTest.java
index 182791d22..1af4a312d 100644
--- a/controlloop/common/eventmanager/src/test/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManagerTest.java
+++ b/controlloop/common/eventmanager/src/test/java/org/onap/policy/controlloop/eventmanager/ControlLoopEventManagerTest.java
@@ -755,9 +755,11 @@ public class ControlLoopEventManagerTest {
 
         LockResult<GuardResult, TargetLock> lockLock = manager.lockCurrentOperation();
         assertNotNull(lockLock);
+        assertEquals(GuardResult.LOCK_ACQUIRED, lockLock.getA());
 
         LockResult<GuardResult, TargetLock> lockLockAgain = manager.lockCurrentOperation();
         assertNotNull(lockLockAgain);
+        assertEquals(GuardResult.LOCK_ACQUIRED, lockLockAgain.getA());
         assertEquals(lockLock.getB(), lockLockAgain.getB());
 
         assertEquals(lockLock.getB(), manager.unlockCurrentOperation());
diff --git a/controlloop/common/guard/src/main/java/org/onap/policy/guard/PolicyGuard.java b/controlloop/common/guard/src/main/java/org/onap/policy/guard/PolicyGuard.java
index 66a25ea63..64d8a2f74 100644
--- a/controlloop/common/guard/src/main/java/org/onap/policy/guard/PolicyGuard.java
+++ b/controlloop/common/guard/src/main/java/org/onap/policy/guard/PolicyGuard.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * guard
  * ================================================================================
- * Copyright (C) 2017 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2017-2018 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.
@@ -21,8 +21,6 @@
 package org.onap.policy.guard;
 
 import java.util.UUID;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Future;
 import org.onap.policy.controlloop.policy.TargetType;
 import org.onap.policy.drools.core.lock.PolicyResourceLockManager;
 import org.onap.policy.guard.impl.PNFTargetLock;
@@ -87,15 +85,16 @@ public class PolicyGuard {
      * @param targetInstance the target instance
      * @param requestID the request Id
      * @param callback the LockCallback
+     * @param holdSec maximum number of seconds to hold the lock
      * @return the LockResult
      * @throws IllegalArgumentException if an argument is null
      */
     public static LockResult<GuardResult, TargetLock> lockTarget(TargetType targetType, String targetInstance,
-            UUID requestID, LockCallback callback) {
+            UUID requestID, LockCallback callback, int holdSec) {
         
         String owner = makeOwner(targetType, requestID);
         
-        GuardResult guardResult = managerLockTarget(targetInstance, owner);
+        GuardResult guardResult = managerLockTarget(targetInstance, owner, holdSec);
         if(guardResult != GuardResult.LOCK_ACQUIRED) {
             return LockResult.createLockResult(guardResult, null);
         }
@@ -133,31 +132,31 @@ public class PolicyGuard {
         logger.debug("Locked {}", lock);
         return LockResult.createLockResult(GuardResult.LOCK_ACQUIRED, lock);
     }
+    
+    /**
+     * Extends a lock on a target.
+     * @param lock      current lock
+     * @param holdSec maximum number of seconds to hold the lock
+     * @return the result: acquired or denied
+     */
+    public static GuardResult lockTarget(TargetLock lock, int holdSec) {
+        String owner = makeOwner(lock.getTargetType(), lock.getRequestID());
+        GuardResult result = managerLockTarget(lock.getTargetInstance(), owner, holdSec);
+        
+        logger.debug("Lock {} extend {}", lock, result);
+        return result;
+    }
 
     /**
      * Asks the manager to lock the given target.
      * @param targetInstance
      * @param owner
-     * @return the result: acquired, denied, or exception
+     * @param holdSec maximum number of seconds to hold the lock
+     * @return the result: acquired or denied
      */
-    private static GuardResult managerLockTarget(String targetInstance, String owner) {
-        try {
-            Future<Boolean> result = factory.getManager().lock(targetInstance, owner, null);
-            return(result.get() ? GuardResult.LOCK_ACQUIRED : GuardResult.LOCK_DENIED);
-            
-        } catch(IllegalStateException e) {
-            logger.warn("{} attempted to re-lock {}", owner, targetInstance);
-            return GuardResult.LOCK_DENIED;
-            
-        } catch (InterruptedException e) {
-            logger.error("exception getting lock for {}", targetInstance, e);
-            Thread.currentThread().interrupt();
-            return GuardResult.LOCK_EXCEPTION;
-            
-        } catch (ExecutionException e) {
-            logger.error("exception getting lock for {}", targetInstance, e);
-            return GuardResult.LOCK_EXCEPTION;
-        }
+    private static GuardResult managerLockTarget(String targetInstance, String owner, int holdSec) {
+        boolean result = factory.getManager().lock(targetInstance, owner, holdSec);
+        return(result ? GuardResult.LOCK_ACQUIRED : GuardResult.LOCK_DENIED);
     }
 
     /**
diff --git a/controlloop/common/guard/src/test/java/org/onap/policy/guard/PolicyGuardTest.java b/controlloop/common/guard/src/test/java/org/onap/policy/guard/PolicyGuardTest.java
index b073fe5a9..9cd34b70a 100644
--- a/controlloop/common/guard/src/test/java/org/onap/policy/guard/PolicyGuardTest.java
+++ b/controlloop/common/guard/src/test/java/org/onap/policy/guard/PolicyGuardTest.java
@@ -25,13 +25,12 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.doThrow;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import java.util.UUID;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Future;
 import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.BeforeClass;
@@ -46,6 +45,7 @@ import org.onap.policy.guard.impl.VNFTargetLock;
 
 public class PolicyGuardTest {
     private static final String INSTANCENAME = "targetInstance";
+    private static final int LOCK_SEC = 10;
     
     private static Factory saveFactory;
     
@@ -131,7 +131,7 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
@@ -155,7 +155,7 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
@@ -180,7 +180,7 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
@@ -204,7 +204,7 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_EXCEPTION, result.getA());
@@ -221,18 +221,12 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
         assertEquals(VMTargetLock.class, result.getB().getClass());
 
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
-        assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-
-        assertEquals(GuardResult.LOCK_DENIED, result.getA());
-        assertNull(result.getB());
-
         // Test isLocked after lock removed
         PolicyGuard.unlockTarget(new DummyTargetLock(type, uuid));
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
@@ -248,13 +242,16 @@ public class PolicyGuardTest {
 
         // Test isLocked before and after lock added
         assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
-        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        LockResult<GuardResult, TargetLock> result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
         assertEquals(VMTargetLock.class, result.getB().getClass());
 
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        UUID uuid2 = UUID.randomUUID();
+        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid2, dlcb, LOCK_SEC);
+        assertFalse(PolicyGuard.isLocked(type, INSTANCENAME, uuid2));
+
         assertTrue(PolicyGuard.isLocked(type, INSTANCENAME, uuid));
 
         assertEquals(GuardResult.LOCK_DENIED, result.getA());
@@ -283,47 +280,41 @@ public class PolicyGuardTest {
     public void testManagerLockTarget() throws Exception {
         TargetType type = TargetType.VM;
         
-        @SuppressWarnings("unchecked")
-        Future<Boolean> fut = mock(Future.class);
-        
         mgr = mock(PolicyResourceLockManager.class);
-        when(mgr.lock(any(), any(), any())).thenReturn(fut);
 
         LockResult<GuardResult, TargetLock> result;
 
         // acquired
-        when(fut.get()).thenReturn(true);
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        when(mgr.lock(anyString(), anyString(), anyInt())).thenReturn(true);
+        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC);
+        verify(mgr).lock(INSTANCENAME, type.toString()+":"+uuid.toString(), LOCK_SEC);
         assertEquals(GuardResult.LOCK_ACQUIRED, result.getA());
         assertEquals(VMTargetLock.class, result.getB().getClass());
 
         // denied
-        when(fut.get()).thenReturn(false);
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
+        when(mgr.lock(anyString(), anyString(), anyInt())).thenReturn(false);
+        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb, LOCK_SEC+2);
+        verify(mgr).lock(INSTANCENAME, type.toString()+":"+uuid.toString(), LOCK_SEC+2);
         assertEquals(GuardResult.LOCK_DENIED, result.getA());
         assertNull(result.getB());
+    }
 
-        // illegal state exception
-        doThrow(new IllegalStateException("state expected")).when(fut).get();
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
-        assertEquals(GuardResult.LOCK_DENIED, result.getA());
-        assertNull(result.getB());
-        assertFalse(Thread.currentThread().isInterrupted());
+    @Test
+    public void testManagerLockTargetTargetLockInt() throws Exception {
+        TargetType type = TargetType.VM;
+        DummyTargetLock lock = new DummyTargetLock(type, uuid);
+        
+        mgr = mock(PolicyResourceLockManager.class);
 
-        // interrupted exception
-        doThrow(new InterruptedException("interrupt expected")).when(fut).get();
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
-        assertEquals(GuardResult.LOCK_EXCEPTION, result.getA());
-        assertNull(result.getB());
-        // verify interrupted & reset interrupt status
-        assertTrue(Thread.interrupted());
+        // acquired
+        when(mgr.lock(anyString(), anyString(), anyInt())).thenReturn(true);
+        assertEquals(GuardResult.LOCK_ACQUIRED, PolicyGuard.lockTarget(lock, LOCK_SEC));
+        verify(mgr).lock(INSTANCENAME, type.toString()+":"+uuid.toString(), LOCK_SEC);
 
-        // exec exception
-        doThrow(new ExecutionException(new IllegalArgumentException("exec expected"))).when(fut).get();
-        result = PolicyGuard.lockTarget(type, INSTANCENAME, uuid, dlcb);
-        assertEquals(GuardResult.LOCK_EXCEPTION, result.getA());
-        assertNull(result.getB());
-        assertFalse(Thread.currentThread().isInterrupted());
+        // denied
+        when(mgr.lock(anyString(), anyString(), anyInt())).thenReturn(false);
+        assertEquals(GuardResult.LOCK_DENIED, PolicyGuard.lockTarget(lock, LOCK_SEC+1));
+        verify(mgr).lock(INSTANCENAME, type.toString()+":"+uuid.toString(), LOCK_SEC+1);
     }
 
     @Test(expected = IllegalArgumentException.class)
diff --git a/controlloop/templates/archetype-cl-amsterdam/src/main/resources/archetype-resources/src/main/resources/__closedLoopControlName__.drl b/controlloop/templates/archetype-cl-amsterdam/src/main/resources/archetype-resources/src/main/resources/__closedLoopControlName__.drl
index b1773f556..1274d6c16 100644
--- a/controlloop/templates/archetype-cl-amsterdam/src/main/resources/archetype-resources/src/main/resources/__closedLoopControlName__.drl
+++ b/controlloop/templates/archetype-cl-amsterdam/src/main/resources/archetype-resources/src/main/resources/__closedLoopControlName__.drl
@@ -449,6 +449,10 @@ rule "${policyName}.EVENT.MANAGER"
               	retract($event);
               	retract($manager);
               	retract($clTimer);
+              	
+              	if(result.getB() != null) {
+              	     retract(result.getB());
+              	}
               }
               logger.info("{}: {}: starting operation={}", 
                           $params.getClosedLoopControlName(), drools.getRule().getName(),