Skip to content

Commit 3d491fe

Browse files
yitingbyitingb
authored
fix(fss): exclude waiting for non-autostartable services and their hard dependencies (#1695)
1 parent 74c2796 commit 3d491fe

File tree

6 files changed

+172
-56
lines changed

6 files changed

+172
-56
lines changed

‎src/main/java/com/aws/greengrass/deployment/KernelUpdateDeploymentTask.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.nio.file.Path;
3030
import java.util.Collections;
3131
import java.util.List;
32+
import java.util.Set;
3233
import java.util.concurrent.CancellationException;
3334
import java.util.concurrent.CompletableFuture;
3435
import java.util.concurrent.ExecutionException;
@@ -89,9 +90,7 @@ private void waitForServicesToStart() {
8990
Deployment.DeploymentStage stage = deployment.getDeploymentStage();
9091
DeploymentResult result = null;
9192
try {
92-
List<GreengrassService> servicesToTrack =
93-
kernel.orderedDependencies().stream().filter(GreengrassService::shouldAutoStart)
94-
.filter(o -> !kernel.getMain().equals(o)).collect(Collectors.toList());
93+
Set<GreengrassService> servicesToTrack = kernel.findAutoStartableServicesToTrack();
9594
long mergeTimestamp = kernel.getConfig().lookup("system", "rootpath").getModtime();
9695

9796
logger.atInfo().kv("serviceToTrack", servicesToTrack).kv("mergeTime", mergeTimestamp)

‎src/main/java/com/aws/greengrass/deployment/activator/DefaultActivator.java‎

Lines changed: 9 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,9 @@
1313
import com.aws.greengrass.lifecyclemanager.GreengrassService;
1414
import com.aws.greengrass.lifecyclemanager.Kernel;
1515
import com.aws.greengrass.lifecyclemanager.exceptions.ServiceLoadException;
16-
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
1716

1817
import java.util.HashMap;
19-
import java.util.LinkedList;
2018
import java.util.Map;
21-
import java.util.Queue;
2219
import java.util.Set;
2320
import java.util.concurrent.CompletableFuture;
2421
import java.util.stream.Collectors;
@@ -82,6 +79,9 @@ public void activate(Map<String, Object> newConfig, Deployment deployment,
8279

8380
try {
8481
Set<GreengrassService> servicesToTrack = servicesChangeManager.servicesToTrack();
82+
// Exclude all non-autoStartable services and their dependers
83+
// Find the services which both changed and auto startable
84+
servicesToTrack.retainAll(kernel.findAutoStartableServicesToTrack());
8585
logger.atDebug(MERGE_CONFIG_EVENT_KEY).kv("serviceToTrack", servicesToTrack).kv("mergeTime", mergeTime)
8686
.log("Applied new service config. Waiting for services to complete update");
8787
waitForServicesToStart(servicesToTrack, mergeTime, kernel, totallyCompleteFuture);
@@ -144,10 +144,13 @@ void rollback(DeploymentDocument deploymentDocument, CompletableFuture<Deploymen
144144
// be expected to still be broken
145145
Set<GreengrassService> servicesToTrackForRollback = rollbackManager.servicesToTrack();
146146

147-
// Also don't track services if they have (transitive) hard dependencies on already-broken services
148-
Set<GreengrassService> brokenServiceAndDependers
149-
= findServiceDependers(servicesToTrackForRollback, rollbackManager.getAlreadyBrokenServices());
147+
// Convert broken services names from String to GreengrassService type
148+
Set<GreengrassService> brokenServices = servicesToTrackForRollback.stream()
149+
.filter(service -> rollbackManager.getAlreadyBrokenServices().contains(service.getName()))
150+
.collect(Collectors.toSet());
150151

152+
// Also don't track services if they have (transitive) hard dependencies on already-broken services
153+
Set<GreengrassService> brokenServiceAndDependers = kernel.findDependers(brokenServices);
151154
servicesToTrackForRollback.removeAll(brokenServiceAndDependers);
152155

153156
logger.atInfo(MERGE_CONFIG_EVENT_KEY)
@@ -170,37 +173,6 @@ void rollback(DeploymentDocument deploymentDocument, CompletableFuture<Deploymen
170173
}
171174
}
172175

173-
/**
174-
* Finds all services which are dependers of given broken services, directly or indirectly
175-
* This method performs a breadth-first search, starting from the broken services and traversing through
176-
* service dependencies.
177-
* @param rollbackServices the set of rollback services to track
178-
* @param brokenServiceNames the set of broken service names
179-
* @return a set of all services depending on the broken services, including themselves
180-
*/
181-
@SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE")
182-
private Set<GreengrassService> findServiceDependers(final Set<GreengrassService> rollbackServices,
183-
final Set<String> brokenServiceNames) {
184-
185-
Set<GreengrassService> dependerServices = rollbackServices.stream()
186-
.filter(service -> brokenServiceNames.contains(service.getName()))
187-
.collect(Collectors.toSet());
188-
Queue<GreengrassService> dependers = new LinkedList<>(dependerServices);
189-
190-
// Breadth-first search to find all dependent services, staring from broken services
191-
while (!dependers.isEmpty()) {
192-
GreengrassService currentService = dependers.poll();
193-
for (GreengrassService depender : currentService.getHardDependers()) {
194-
// Ensure dependers haven't been processed
195-
if (dependerServices.add(depender)) {
196-
dependers.offer(depender);
197-
}
198-
}
199-
}
200-
return dependerServices;
201-
}
202-
203-
204176
private void handleFailureRollback(CompletableFuture totallyCompleteFuture, Throwable deploymentFailureCause,
205177
Throwable rollbackFailureCause) {
206178
// Rollback execution failed
@@ -209,5 +181,4 @@ private void handleFailureRollback(CompletableFuture totallyCompleteFuture, Thro
209181
totallyCompleteFuture.complete(new DeploymentResult(DeploymentResult.DeploymentStatus.FAILED_UNABLE_TO_ROLLBACK,
210182
deploymentFailureCause));
211183
}
212-
213184
}

‎src/main/java/com/aws/greengrass/lifecyclemanager/Kernel.java‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,13 @@
8080
import java.util.HashMap;
8181
import java.util.HashSet;
8282
import java.util.LinkedHashSet;
83+
import java.util.LinkedList;
8384
import java.util.List;
8485
import java.util.Locale;
8586
import java.util.Map;
8687
import java.util.Optional;
88+
import java.util.Queue;
89+
import java.util.Set;
8790
import java.util.concurrent.ConcurrentHashMap;
8891
import java.util.concurrent.Executor;
8992
import java.util.concurrent.ExecutorService;
@@ -913,4 +916,49 @@ private void setupProxy() {
913916
public List<String> getSupportedCapabilities() {
914917
return SUPPORTED_CAPABILITIES;
915918
}
919+
920+
/**
921+
* Finds all auto startable services with auto startable dependencies.
922+
* This method performs a breadth-first search, starting from the target services and traversing through
923+
* all hard dependencies and exclude non auto startable services from.
924+
*
925+
* @return a set of all services that only contains auto startable services and their dependencies are all
926+
* auto startable services
927+
*/
928+
public Set<GreengrassService> findAutoStartableServicesToTrack() {
929+
// Find all non auto startable services
930+
Set<GreengrassService> nonAutoStartableServices = orderedDependencies().stream()
931+
.filter(service -> !service.shouldAutoStart()).collect(Collectors.toSet());
932+
933+
Set<GreengrassService> nonAutoStartableDependers = findDependers(nonAutoStartableServices);
934+
935+
// Return the set which excludes all non auto startable services and their dependers
936+
return orderedDependencies().stream()
937+
.filter(service -> !nonAutoStartableDependers.contains(service))
938+
.collect(Collectors.toSet());
939+
}
940+
941+
/**
942+
* Finds all services which are dependers of initial services, directly or indirectly
943+
* This method performs a breadth-first search, starting from the initial services and traversing through
944+
* all hard dependencies.
945+
* @param initialServices the set of services that we want to find dependers
946+
* @return a set of all services that depend on the target services, including the initial services
947+
*/
948+
@SuppressFBWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE")
949+
public Set<GreengrassService> findDependers(Set<GreengrassService> initialServices) {
950+
Queue<GreengrassService> dependers = new LinkedList<>(initialServices);
951+
952+
// Breadth-first search to find all dependent services, staring from non auto startable services
953+
while (!dependers.isEmpty()) {
954+
GreengrassService currentService = dependers.poll();
955+
for (GreengrassService depender : currentService.getHardDependers()) {
956+
// Ensure dependers haven't been processed
957+
if (initialServices.add(depender)) {
958+
dependers.offer(depender);
959+
}
960+
}
961+
}
962+
return initialServices;
963+
}
916964
}

‎src/main/java/com/aws/greengrass/lifecyclemanager/KernelLifecycle.java‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -600,19 +600,19 @@ GreengrassService getMain() {
600600
* @return true if all services in terminal states
601601
*/
602602
public boolean allServicesInTerminalState() {
603-
List<GreengrassService> servicesToTrack = kernel.orderedDependencies().stream()
604-
.filter(GreengrassService::shouldAutoStart).collect(Collectors.toList());
603+
List<GreengrassService> servicesToTrack = kernel.findAutoStartableServicesToTrack()
604+
.stream().collect(Collectors.toList());
605605
return servicesToTrack.stream().allMatch(service -> {
606-
State state = service.getState();
607-
// service is broken
608-
if (State.BROKEN.equals(state)) {
609-
return true;
610-
}
611-
// or service has reached desired state, and it is either running or finished
612-
if (service.reachedDesiredState()) {
613-
return State.RUNNING.equals(state) || State.FINISHED.equals(state);
614-
}
615-
return false;
616-
});
606+
State state = service.getState();
607+
// service is broken
608+
if (State.BROKEN.equals(state)) {
609+
return true;
610+
}
611+
// or service has reached desired state, and it is either running or finished
612+
if (service.reachedDesiredState()) {
613+
return State.RUNNING.equals(state) || State.FINISHED.equals(state);
614+
}
615+
return false;
616+
});
617617
}
618618
}

‎src/test/java/com/aws/greengrass/deployment/KernelUpdateDeploymentTaskTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import java.util.concurrent.ExecutorService;
3838
import java.util.concurrent.Executors;
3939
import java.util.concurrent.Future;
40+
import java.util.stream.Collectors;
4041

4142
import static com.aws.greengrass.dependency.State.BROKEN;
4243
import static com.aws.greengrass.dependency.State.FINISHED;
@@ -99,7 +100,7 @@ void beforeEach() throws Exception {
99100
lenient().doReturn("A").when(greengrassService).getName();
100101
lenient().doReturn(mainService).when(kernel).getMain();
101102
lenient().doReturn(true).when(greengrassService).shouldAutoStart();
102-
lenient().doReturn(Arrays.asList(greengrassService)).when(kernel).orderedDependencies();
103+
lenient().doReturn(Arrays.asList(greengrassService).stream().collect(Collectors.toSet())).when(kernel).findAutoStartableServicesToTrack();
103104
lenient().doNothing().when(componentManager).cleanupStaleVersions();
104105
lenient().doReturn(nucleusPaths).when(kernel).getNucleusPaths();
105106

‎src/test/java/com/aws/greengrass/lifecyclemanager/KernelTest.java‎

Lines changed: 98 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,10 @@
5252
import java.util.Arrays;
5353
import java.util.Collections;
5454
import java.util.HashMap;
55+
import java.util.HashSet;
5556
import java.util.List;
5657
import java.util.Map;
58+
import java.util.Set;
5759

5860
import static com.amazon.aws.iot.greengrass.component.common.SerializerFactory.getRecipeSerializer;
5961
import static com.aws.greengrass.componentmanager.KernelConfigResolver.VERSION_CONFIG_KEY;
@@ -80,6 +82,7 @@
8082
import static org.junit.jupiter.api.Assertions.assertNull;
8183
import static org.junit.jupiter.api.Assertions.assertThrows;
8284
import static org.junit.jupiter.api.Assertions.assertTrue;
85+
import static org.junit.jupiter.api.Assertions.assertFalse;
8386
import static org.mockito.ArgumentMatchers.any;
8487
import static org.mockito.ArgumentMatchers.anyString;
8588
import static org.mockito.ArgumentMatchers.eq;
@@ -112,7 +115,14 @@ class KernelTest {
112115
private Path mockFile;
113116
private Path tempFile;
114117
DeviceConfiguration deviceConfiguration;
115-
118+
@Mock
119+
private GreengrassService service1;
120+
@Mock
121+
private GreengrassService service2;
122+
@Mock
123+
private GreengrassService service3;
124+
@Mock
125+
private GreengrassService service4;
116126
@BeforeEach
117127
void beforeEach() throws Exception{
118128
System.setProperty("root", tempRootDir.toAbsolutePath().toString());
@@ -788,7 +798,94 @@ void GIVEN_unpack_dir_is_nucleus_root_WHEN_initializeComponentStore_THEN_copy_to
788798
mockNucleusUnpackDir.assertDirectoryEquals(nucleusPaths.unarchiveArtifactPath(
789799
componentIdentifier, DEFAULT_NUCLEUS_COMPONENT_NAME.toLowerCase()));
790800
}
801+
@Test
802+
void GIVEN_all_services_autoStartable_THEN_all_services_in_services_to_track() {
803+
kernel = spy(kernel);
804+
// Arrange
805+
when(service1.shouldAutoStart()).thenReturn(true);
806+
when(service2.shouldAutoStart()).thenReturn(true);
807+
Set<GreengrassService> services = new HashSet<>(Arrays.asList(service1, service2));
808+
809+
when(kernel.orderedDependencies()).thenReturn(Arrays.asList(service1, service2));
810+
811+
// Act
812+
Set<GreengrassService> result = kernel.findAutoStartableServicesToTrack();
813+
814+
// Assert
815+
assertEquals(services, result);
816+
assertEquals(2, result.size());
817+
}
791818

819+
@Test
820+
void GIVEN_no_autoStartableServices_THEN_services_to_track_list_empty() {
821+
kernel = spy(kernel);
822+
// Arrange
823+
when(service1.shouldAutoStart()).thenReturn(false);
824+
when(service2.shouldAutoStart()).thenReturn(false);
825+
Set<GreengrassService> services = new HashSet<>(Arrays.asList(service1, service2));
826+
827+
when(kernel.orderedDependencies()).thenReturn(services);
828+
when(service1.getHardDependers()).thenReturn(Collections.emptyList());
829+
when(service2.getHardDependers()).thenReturn(Collections.emptyList());
830+
831+
// Act
832+
Set<GreengrassService> result = kernel.findAutoStartableServicesToTrack();
833+
834+
// Assert
835+
assertTrue(result.isEmpty());
836+
}
837+
838+
@Test
839+
void GIVEN_mixed_autoStartableServices_THEN_services_to_track_only_contain_autoStartableServices() {
840+
kernel = spy(kernel);
841+
// Arrange
842+
when(service1.shouldAutoStart()).thenReturn(true);
843+
when(service2.shouldAutoStart()).thenReturn(false);
844+
when(service3.shouldAutoStart()).thenReturn(true);
845+
when(service4.shouldAutoStart()).thenReturn(true);
846+
847+
Set<GreengrassService> services = new HashSet<>(Arrays.asList(service1, service2, service3, service4));
848+
849+
// service3 depends on service2 (non-auto-startable)
850+
when(service2.getHardDependers()).thenReturn(Arrays.asList(service3));
851+
when(service3.getHardDependers()).thenReturn(Collections.emptyList());
852+
853+
when(kernel.orderedDependencies()).thenReturn(services);
854+
855+
// Act
856+
Set<GreengrassService> result = kernel.findAutoStartableServicesToTrack();
857+
858+
// Assert
859+
assertEquals(2, result.size());
860+
assertTrue(result.contains(service1));
861+
assertTrue(result.contains(service4));
862+
assertFalse(result.contains(service2));
863+
assertFalse(result.contains(service3));
864+
}
865+
866+
@Test
867+
void GIVEN_chained_services_THEN_services_to_track_only_contain_autoStartableServices() {
868+
kernel = spy(kernel);
869+
// Arrange
870+
when(service1.shouldAutoStart()).thenReturn(false);
871+
when(service2.shouldAutoStart()).thenReturn(true);
872+
when(service3.shouldAutoStart()).thenReturn(true);
873+
874+
Set<GreengrassService> services = new HashSet<>(Arrays.asList(service1, service2, service3));
875+
876+
// service2 depends on service1, service3 depends on service2
877+
when(service1.getHardDependers()).thenReturn(Arrays.asList(service2));
878+
when(service2.getHardDependers()).thenReturn(Arrays.asList(service3));
879+
when(service3.getHardDependers()).thenReturn(Collections.emptyList());
880+
881+
when(kernel.orderedDependencies()).thenReturn(services);
882+
883+
// Act
884+
Set<GreengrassService> result = kernel.findAutoStartableServicesToTrack();
885+
886+
// Assert
887+
assertTrue(result.isEmpty());
888+
}
792889
static class TestClass extends GreengrassService {
793890
public TestClass(Topics topics) {
794891
super(topics);

0 commit comments

Comments
 (0)