chore: general changes for cancel deployment (#1631)

Co-authored-by: Tianci Shen <[email protected]>

src/main/java/com/aws/greengrass/deployment/DeploymentConfigMerger.java

@@ -40,6 +40,7 @@
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
@@ -66,6 +67,7 @@ public class DeploymentConfigMerger {
     private Kernel kernel;
     private DeviceConfiguration deviceConfiguration;
     private DynamicComponentConfigurationValidator validator;
+    private ExecutorService executorService;
 
     /**
      * Merge in new configuration values and new services.
@@ -106,7 +108,11 @@ public Future<DeploymentResult> mergeInNewConfig(Deployment deployment,
         } else {
             logger.atInfo().log("Deployment is configured to skip update policy check,"
                     + " not waiting for disruptable time to update");
-            updateActionForDeployment(newConfig, deployment, activator, totallyCompleteFuture);
+            // use executor service to execute updateActionForDeployment
+            // prevents default deployment cancellation from directly interrupting
+            executorService.execute(() -> updateActionForDeployment(newConfig, deployment, activator,
+                    totallyCompleteFuture));
+
         }
 
         return totallyCompleteFuture;

src/main/java/com/aws/greengrass/deployment/activator/DeploymentActivator.java

@@ -44,6 +44,9 @@ public abstract void activate(Map<String, Object> newConfig, Deployment deployme
                                   CompletableFuture<DeploymentResult> totallyCompleteFuture);
 
     protected boolean takeConfigSnapshot(CompletableFuture<DeploymentResult> totallyCompleteFuture) {
+         if (totallyCompleteFuture.isCancelled()) {
+            return false;
+        }
         try {
             deploymentDirectoryManager.takeConfigSnapshot(deploymentDirectoryManager.getSnapshotFilePath());
             return true;

src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java

@@ -827,9 +827,10 @@ void setClosed(boolean b) {
      * Start Service.
      */
     final void requestStart() {
-        // Ignore start requests if the service is closed
-        if (isClosed.get()) {
-            return;
+        // It's ok to start service again if the lifecycle thread is in the middle of closing
+        if (isClosed.compareAndSet(true, false)) {
+            logger.atWarn("service-shutdown-in-progress")
+                    .log("Requesting service to start while it is closing");
         }
         try (LockScope ls = LockScope.lock(desiredStateLock)) {
             if (desiredStateList.isEmpty() || desiredStateList.equals(Collections.singletonList(State.FINISHED))) {
@@ -851,9 +852,10 @@ final void requestStart() {
      * ReInstall Service.
      */
     final void requestReinstall() {
-        // Ignore reinstall requests if the service is closed
-        if (isClosed.get()) {
-            return;
+        // It's ok to reinstall service again if the lifecycle thread is in the middle of closing
+        if (isClosed.compareAndSet(true, false)) {
+            logger.atWarn("service-shutdown-in-progress")
+                    .log("Requesting service to reinstall while it is closing");
         }
         try (LockScope ls = LockScope.lock(desiredStateLock)) {
             setDesiredState(State.NEW, State.RUNNING);

src/test/java/com/aws/greengrass/deployment/DeploymentConfigMergerTest.java

@@ -47,6 +47,7 @@
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -90,6 +91,8 @@ class DeploymentConfigMergerTest {
     @Mock
     private DynamicComponentConfigurationValidator validator;
     @Mock
+    private ExecutorService executorService;
+    @Mock
     private Context context;
 
     @BeforeEach
@@ -307,7 +310,7 @@ void GIVEN_deployment_WHEN_check_safety_selected_THEN_check_safety_before_update
         when(deploymentActivatorFactory.getDeploymentActivator(any())).thenReturn(deploymentActivator);
         when(context.get(DeploymentActivatorFactory.class)).thenReturn(deploymentActivatorFactory);
 
-        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator);
+        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator, executorService);
 
         DeploymentDocument doc = new DeploymentDocument();
         doc.setConfigurationArn("NoSafetyCheckDeploy");
@@ -345,7 +348,7 @@ void GIVEN_deployment_WHEN_task_cancelled_THEN_update_is_cancelled() throws Thro
         });
 
         // GIVEN
-        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator);
+        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator, executorService);
         DeploymentDocument doc = mock(DeploymentDocument.class);
         when(doc.getDeploymentId()).thenReturn("DeploymentId");
         when(doc.getComponentUpdatePolicy()).thenReturn(
@@ -381,7 +384,7 @@ void GIVEN_deployment_WHEN_task_not_cancelled_THEN_update_is_continued() throws
         when(context.get(DefaultActivator.class)).thenReturn(defaultActivator);
 
         // GIVEN
-        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator);
+        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator, executorService);
         DeploymentDocument doc = mock(DeploymentDocument.class);
         when(doc.getDeploymentId()).thenReturn("DeploymentId");
         when(doc.getComponentUpdatePolicy()).thenReturn(
@@ -437,7 +440,7 @@ void GIVEN_deployment_activate_WHEN_deployment_has_new_config_THEN_new_config_is
         newConfig2.put(DEFAULT_NUCLEUS_COMPONENT_NAME, newConfig3);
         newConfig.put(SERVICES_NAMESPACE_TOPIC, newConfig2);
         // GIVEN
-        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator);
+        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator, executorService);
         DeploymentDocument doc = mock(DeploymentDocument.class);
         when(doc.getDeploymentId()).thenReturn("DeploymentId");
         when(doc.getComponentUpdatePolicy()).thenReturn(
@@ -498,7 +501,7 @@ void GIVEN_deployment_activate_WHEN_deployment_has_some_new_config_THEN_old_conf
         newConfig2.put(DEFAULT_NUCLEUS_COMPONENT_NAME, newConfig3);
         newConfig.put(SERVICES_NAMESPACE_TOPIC, newConfig2);
         // GIVEN
-        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator);
+        DeploymentConfigMerger merger = new DeploymentConfigMerger(kernel, deviceConfiguration, validator, executorService);
         DeploymentDocument doc = mock(DeploymentDocument.class);
         when(doc.getDeploymentId()).thenReturn("DeploymentId");
         when(doc.getComponentUpdatePolicy()).thenReturn(

src/test/java/com/aws/greengrass/deployment/activator/KernelUpdateActivatorTest.java

@@ -47,12 +47,15 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.lenient;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 @ExtendWith({GGExtension.class, MockitoExtension.class})
 class KernelUpdateActivatorTest {
@@ -210,4 +213,14 @@ void GIVEN_launch_dir_corrupted_WHEN_deployment_activate_THEN_deployment_fail(Ex
         List<String> expectedTypes = Collections.singletonList("NUCLEUS_ERROR");
         TestUtils.validateGenerateErrorReport(result.getFailureCause(), expectedStack, expectedTypes);
     }
+
+    @Test
+    void GIVEN_deployment_activate_WHEN_bootstrap_a_finishes_THEN_request_restart() throws Exception  {
+        when(totallyCompleteFuture.isCancelled()).thenReturn(true);
+        kernelUpdateActivator.activate(newConfig, deployment, totallyCompleteFuture);
+        verify(deploymentDirectoryManager, never()).takeConfigSnapshot(any());
+        verify(bootstrapManager, never()).persistBootstrapTaskList(any());
+        verify(kernelAlternatives, never()).prepareBootstrap(any());
+        verify(kernel, never()).shutdown(anyInt(), anyInt());
+    }
 }