diff --git a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/DefaultServicesStartStrategy.java b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/DefaultServicesStartStrategy.java index ac56c70626..defbd5e7cb 100644 --- a/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/DefaultServicesStartStrategy.java +++ b/wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/environment/server/DefaultServicesStartStrategy.java @@ -17,7 +17,7 @@ import org.eclipse.che.api.environment.server.model.CheServiceImpl; import org.eclipse.che.api.environment.server.model.CheServicesEnvironmentImpl; import java.util.HashMap; -import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Optional; @@ -29,7 +29,8 @@ import static java.lang.String.format; /** * Finds order of Che services to start that respects dependencies between services. * - * author Alexander Garagatyi + * @author Alexander Garagatyi + * @author Alexander Andrienko */ public class DefaultServicesStartStrategy { /** @@ -55,7 +56,6 @@ public class DefaultServicesStartStrategy { throws IllegalArgumentException { HashMap weights = new HashMap<>(); - Set machinesLeft = new HashSet<>(services.keySet()); // create machines dependency graph Map> dependencies = new HashMap<>(services.size()); @@ -67,32 +67,20 @@ public class DefaultServicesStartStrategy { service.getVolumesFrom().size()); for (String dependsOn : service.getDependsOn()) { - if (!services.containsKey(dependsOn)) { - throw new IllegalArgumentException( - format("Dependency '%s' in machine '%s' points to not known machine.", - dependsOn, serviceEntry.getKey())); - } + checkDependency(dependsOn, serviceEntry.getKey(), services, "A machine can not depend on itself"); machineDependencies.add(dependsOn); } // links also counts as dependencies for (String link : service.getLinks()) { String dependency = getServiceFromLink(link); - if (!services.containsKey(dependency)) { - throw new IllegalArgumentException( - format("Dependency '%s' in machine '%s' points to not known machine.", - dependency, serviceEntry.getKey())); - } + checkDependency(dependency, serviceEntry.getKey(), services, "A machine can not link to itself"); machineDependencies.add(dependency); } // volumesFrom also counts as dependencies for (String volumesFrom : service.getVolumesFrom()) { String dependency = getServiceFromVolumesFrom(volumesFrom); - if (!services.containsKey(dependency)) { - throw new IllegalArgumentException( - format("Dependency '%s' in machine '%s' points to not known machine.", - dependency, serviceEntry.getKey())); - } + checkDependency(dependency, serviceEntry.getKey(), services, "A machine can not contain 'volumes_from' to itself"); machineDependencies.add(dependency); } dependencies.put(serviceEntry.getKey(), machineDependencies); @@ -101,48 +89,36 @@ public class DefaultServicesStartStrategy { // Find weight of each machine in graph. // Weight of machine is calculated as sum of all weights of machines it depends on. // Nodes with no dependencies gets weight 0 - - // If this flag is not set during cycle of machine evaluation loop - // then no more weight of machine can be evaluated - boolean weightEvaluatedInCycleRun = true; - while (weights.size() != dependencies.size() && weightEvaluatedInCycleRun) { - weightEvaluatedInCycleRun = false; - for (String service : dependencies.keySet()) { + while (!dependencies.isEmpty()) { + int previousSize = dependencies.size(); + for (Iterator>> it = dependencies.entrySet().iterator(); it.hasNext();) { // process not yet processed machines only - if (machinesLeft.contains(service)) { - if (dependencies.get(service).size() == 0) { - // no links - smallest weight 0 - weights.put(service, 0); - machinesLeft.remove(service); - weightEvaluatedInCycleRun = true; - } else { - // machine has dependencies - check if it has not weighted dependencies - Optional nonWeightedLink = dependencies.get(service) - .stream() - .filter(machinesLeft::contains) - .findAny(); - if (!nonWeightedLink.isPresent()) { - // all connections are weighted - lets evaluate current machine - Optional maxWeight = dependencies.get(service) - .stream() - .max((o1, o2) -> weights.get(o1).compareTo(weights.get(o2))); - // optional can't be empty because size of the list is checked above - //noinspection OptionalGetWithoutIsPresent - weights.put(service, weights.get(maxWeight.get()) + 1); - machinesLeft.remove(service); - weightEvaluatedInCycleRun = true; - } + Map.Entry> serviceEntry = it.next(); + String service = serviceEntry.getKey(); + Set serviceDependencies = serviceEntry.getValue(); + + if (serviceDependencies.isEmpty()) { + // no links - smallest weight 0 + weights.put(service, 0); + it.remove(); + } else { + // machine has dependencies - check if it has not weighted dependencies + if (weights.keySet().containsAll(serviceDependencies)) { + // all connections are weighted - lets evaluate current machine + Optional maxWeight = serviceDependencies.stream() + .max((o1, o2) -> weights.get(o1).compareTo(weights.get(o2))); + // optional can't be empty because size of the list is checked above + //noinspection OptionalGetWithoutIsPresent + weights.put(service, weights.get(maxWeight.get()) + 1); + it.remove(); } } } - } - - // Not evaluated weights of machines left. - // Probably because of circular dependency. - if (weights.size() != services.size()) { - throw new IllegalArgumentException("Launch order of machines '" + - Joiner.on(", ").join(machinesLeft) + - "' can't be evaluated"); + if (dependencies.size() == previousSize) { + throw new IllegalArgumentException("Launch order of machines '" + + Joiner.on(", ").join(dependencies.keySet()) + + "' can't be evaluated. Circular dependency."); + } } return weights; @@ -185,4 +161,15 @@ public class DefaultServicesStartStrategy { .map(Map.Entry::getKey) .collect(Collectors.toList()); } + + private void checkDependency(String dependency, String serviceName, Map services, String errorMessage) { + if (serviceName.equals(dependency)) { + throw new IllegalArgumentException(errorMessage + ": " + serviceName); + } + if (!services.containsKey(dependency)) { + throw new IllegalArgumentException( + format("Dependency '%s' in machine '%s' points to unknown machine.", + dependency, serviceName)); + } + } } diff --git a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/environment/server/compose/DefaultServicesStartStrategyTest.java b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/environment/server/compose/DefaultServicesStartStrategyTest.java index 1ea82c92aa..65052dba78 100644 --- a/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/environment/server/compose/DefaultServicesStartStrategyTest.java +++ b/wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/environment/server/compose/DefaultServicesStartStrategyTest.java @@ -26,6 +26,7 @@ import static org.testng.Assert.assertTrue; /** * @author Alexander Garagatyi + * @author Alexander Andrienko */ public class DefaultServicesStartStrategyTest { DefaultServicesStartStrategy strategy = new DefaultServicesStartStrategy(); @@ -48,6 +49,24 @@ public class DefaultServicesStartStrategyTest { assertEquals(actual, expected); } + @Test + public void shouldOrderServicesWithDependenciesWhereOrderIsStrict2() { + // given + CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); + composeEnvironment.getServices().put("web", new CheServiceImpl().withDependsOn(asList("db", "redis"))); + composeEnvironment.getServices().put("redis", new CheServiceImpl().withDependsOn(singletonList("dev-machine"))); + composeEnvironment.getServices().put("db", new CheServiceImpl().withDependsOn(singletonList("redis"))); + composeEnvironment.getServices().put("dev-machine", new CheServiceImpl().withDependsOn(emptyList())); + + List expected = asList("dev-machine", "redis", "db", "web"); + + // when + List actual = strategy.order(composeEnvironment); + + // then + assertEquals(actual, expected); + } + @Test public void testOrderingOfServicesWithoutDependencies() throws Exception { // given @@ -87,7 +106,7 @@ public class DefaultServicesStartStrategyTest { } @Test(expectedExceptions = IllegalArgumentException.class, - expectedExceptionsMessageRegExp = "Launch order of machines '.*, .*' can't be evaluated") + expectedExceptionsMessageRegExp = "Launch order of machines '.*, .*' can't be evaluated. Circular dependency.") public void shouldFailIfCircularDependencyFound() throws Exception { // given CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); @@ -99,6 +118,37 @@ public class DefaultServicesStartStrategyTest { strategy.order(composeEnvironment); } + @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "A machine can not link to itself: .*") + public void shouldFailIfMachineLinksByItSelf() { + // given + CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); + composeEnvironment.getServices().put("first", new CheServiceImpl().withLinks(singletonList("first"))); + + // when + strategy.order(composeEnvironment); + } + + @Test(expectedExceptions = IllegalArgumentException.class, expectedExceptionsMessageRegExp = "A machine can not depend on itself: .*") + public void shouldFailIfMachineDependsOnByItSelf() { + // given + CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); + composeEnvironment.getServices().put("first", new CheServiceImpl().withDependsOn(singletonList("first"))); + + // when + strategy.order(composeEnvironment); + } + + @Test(expectedExceptions = IllegalArgumentException.class, + expectedExceptionsMessageRegExp = "A machine can not contain 'volumes_from' to itself:.*") + public void shouldFailIfMachineContainsVolumesFromByItSelf() { + // given + CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); + composeEnvironment.getServices().put("first", new CheServiceImpl().withVolumesFrom(singletonList("first"))); + + // when + strategy.order(composeEnvironment); + } + @Test public void shouldOrderServicesWithLinks() throws Exception { // given @@ -203,7 +253,7 @@ public class DefaultServicesStartStrategyTest { } @Test(expectedExceptions = IllegalArgumentException.class, - expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'second' points to not known machine.") + expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'second' points to unknown machine.") public void shouldFailIfDependsOnFieldContainsNonExistingService() throws Exception { // given CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); @@ -216,7 +266,7 @@ public class DefaultServicesStartStrategyTest { } @Test(expectedExceptions = IllegalArgumentException.class, - expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'third' points to not known machine.") + expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'third' points to unknown machine.") public void shouldFailIfVolumesFromFieldContainsNonExistingService() throws Exception { // given CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl(); @@ -244,7 +294,7 @@ public class DefaultServicesStartStrategyTest { } @Test(expectedExceptions = IllegalArgumentException.class, - expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'second' points to not known machine.") + expectedExceptionsMessageRegExp = "Dependency 'fifth' in machine 'second' points to unknown machine.") public void shouldFailIfLinksFieldContainsNonExistingService() throws Exception { // given CheServicesEnvironmentImpl composeEnvironment = new CheServicesEnvironmentImpl();