diff --git a/src/main/java/com/aws/greengrass/deployment/DeploymentConfigMerger.java b/src/main/java/com/aws/greengrass/deployment/DeploymentConfigMerger.java index 961591dece..26723f719e 100644 --- a/src/main/java/com/aws/greengrass/deployment/DeploymentConfigMerger.java +++ b/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 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; diff --git a/src/main/java/com/aws/greengrass/deployment/activator/DeploymentActivator.java b/src/main/java/com/aws/greengrass/deployment/activator/DeploymentActivator.java index df15f2f7a8..db28c72f54 100644 --- a/src/main/java/com/aws/greengrass/deployment/activator/DeploymentActivator.java +++ b/src/main/java/com/aws/greengrass/deployment/activator/DeploymentActivator.java @@ -44,6 +44,9 @@ public abstract void activate(Map newConfig, Deployment deployme CompletableFuture totallyCompleteFuture); protected boolean takeConfigSnapshot(CompletableFuture totallyCompleteFuture) { + if (totallyCompleteFuture.isCancelled()) { + return false; + } try { deploymentDirectoryManager.takeConfigSnapshot(deploymentDirectoryManager.getSnapshotFilePath()); return true; diff --git a/src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java b/src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java index 35b6a54109..d23d70190c 100644 --- a/src/main/java/com/aws/greengrass/lifecyclemanager/Lifecycle.java +++ b/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); diff --git a/src/test/java/com/aws/greengrass/deployment/DeploymentConfigMergerTest.java b/src/test/java/com/aws/greengrass/deployment/DeploymentConfigMergerTest.java index a6a78438fb..78d37fae77 100644 --- a/src/test/java/com/aws/greengrass/deployment/DeploymentConfigMergerTest.java +++ b/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( diff --git a/src/test/java/com/aws/greengrass/deployment/activator/KernelUpdateActivatorTest.java b/src/test/java/com/aws/greengrass/deployment/activator/KernelUpdateActivatorTest.java index 564e398c17..d340fd8b69 100644 --- a/src/test/java/com/aws/greengrass/deployment/activator/KernelUpdateActivatorTest.java +++ b/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 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()); + } }