From 39ec3cdd87f77647e1dc5d787beacaf826e6b82f Mon Sep 17 00:00:00 2001 From: Henning Schmiedehausen Date: Wed, 25 Jun 2014 16:35:31 -0700 Subject: [PATCH 1/2] Fix bugs with allow-duplicate-installations-on-an-agent. - Make sure that the config setting is respected everywhere. - If less agents than requested installs are present and duplicate installation is allowed, install more than one instance of a service per agent. --- .../airship/coordinator/AdminResource.java | 6 ++- .../airship/coordinator/Coordinator.java | 41 +++++++++++++++---- .../coordinator/CoordinatorSlotResource.java | 7 +++- .../coordinator/TestAdminResource.java | 2 +- .../TestCoordinatorSlotResource.java | 5 ++- 5 files changed, 47 insertions(+), 14 deletions(-) diff --git a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java index 6bebb1f8..62589f97 100644 --- a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java +++ b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java @@ -32,12 +32,14 @@ public class AdminResource { private final Coordinator coordinator; private final Repository repository; + private final CoordinatorConfig config; @Inject - public AdminResource(Coordinator coordinator, Repository repository) + public AdminResource(Coordinator coordinator, Repository repository, CoordinatorConfig config) { this.coordinator = coordinator; this.repository = repository; + this.config = config; } @GET @@ -80,7 +82,7 @@ public Response getAllAgents(@Context UriInfo uriInfo) Predicate agentPredicate = AgentFilterBuilder.build(uriInfo, transform(coordinator.getAgents(), idGetter()), transform(allSlotStatus, SlotStatus.uuidGetter()), - false, + config.isAllowDuplicateInstallationsOnAnAgent(), repository); List agents = coordinator.getAgents(agentPredicate); diff --git a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/Coordinator.java b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/Coordinator.java index 31202805..b51837b5 100644 --- a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/Coordinator.java +++ b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/Coordinator.java @@ -41,6 +41,7 @@ import javax.annotation.PostConstruct; import java.net.URI; +import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -65,6 +66,7 @@ import java.util.concurrent.TimeUnit; import static com.google.common.base.Preconditions.checkNotNull; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.collect.Iterables.concat; import static com.google.common.collect.Iterables.filter; import static com.google.common.collect.Iterables.transform; @@ -438,19 +440,44 @@ public List install(Predicate filter, int limit, Assign { final Installation installation = InstallationUtils.toInstallation(repository, assignment); - List targetAgents = new ArrayList<>(selectAgents(filter, installation)); - targetAgents = targetAgents.subList(0, Math.min(targetAgents.size(), limit)); + List candidateAgents = new ArrayList<>(selectAgents(filter, installation)); + // Prune agent list if more than required exist. + if (candidateAgents.size() > limit) { + candidateAgents = candidateAgents.subList(0, limit); + } + + int totalInstalls = limit; + int installsPerHost = allowDuplicateInstallationsOnAnAgent ? limit / candidateAgents.size() : 1; + + ImmutableList.Builder> builder = ImmutableList.builder(); + for (RemoteAgent agent : candidateAgents) { + int installsToAssign = Math.min(totalInstalls, installsPerHost); + if (installsToAssign <= 0) { + break; + } + builder.add(new AbstractMap.SimpleImmutableEntry<>(agent, installsToAssign)); + totalInstalls -= installsToAssign; + } + + List> targetAgents = builder.build(); - return parallel(targetAgents, new Function() + List> results = parallel(targetAgents, new Function, List>() { @Override - public SlotStatus apply(RemoteAgent agent) + public List apply(Map.Entry entry) { - SlotStatus slotStatus = agent.install(installation); - stateManager.setExpectedState(new ExpectedSlotStatus(slotStatus.getId(), STOPPED, installation.getAssignment())); - return slotStatus; + RemoteAgent agent = entry.getKey(); + ImmutableList.Builder builder = ImmutableList.builder(); + for (int i = 0; i < entry.getValue(); i++) { + SlotStatus slotStatus = agent.install(installation); + builder.add(slotStatus); + stateManager.setExpectedState(new ExpectedSlotStatus(slotStatus.getId(), STOPPED, installation.getAssignment())); + } + return builder.build(); } }); + + return ImmutableList.copyOf(Iterables.concat(results)); } private List selectAgents(Predicate filter, Installation installation) diff --git a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java index 095272d7..9773b22a 100644 --- a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java +++ b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java @@ -56,15 +56,18 @@ public class CoordinatorSlotResource private final Coordinator coordinator; private final Repository repository; + private final CoordinatorConfig config; @Inject - public CoordinatorSlotResource(Coordinator coordinator, Repository repository) + public CoordinatorSlotResource(Coordinator coordinator, Repository repository, CoordinatorConfig config) { Preconditions.checkNotNull(coordinator, "coordinator must not be null"); Preconditions.checkNotNull(repository, "repository is null"); this.coordinator = coordinator; this.repository = repository; + + this.config = Preconditions.checkNotNull(config, "coordinatorConfig is null"); } @GET @@ -102,7 +105,7 @@ public Response install( Predicate agentFilter = AgentFilterBuilder.build(uriInfo, transform(coordinator.getAgents(), idGetter()), transform(coordinator.getAllSlotStatus(), uuidGetter()), - false, + config.isAllowDuplicateInstallationsOnAnAgent(), repository); List agents = coordinator.getAgents(agentFilter); diff --git a/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestAdminResource.java b/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestAdminResource.java index 89ec644a..66375100 100644 --- a/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestAdminResource.java +++ b/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestAdminResource.java @@ -61,7 +61,7 @@ public void setUp() new MockServiceInventory(), new Duration(1, TimeUnit.DAYS), false); - resource = new AdminResource(coordinator, repository); + resource = new AdminResource(coordinator, repository, new CoordinatorConfig()); } @AfterMethod diff --git a/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestCoordinatorSlotResource.java b/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestCoordinatorSlotResource.java index a44c5e92..f566f4f9 100644 --- a/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestCoordinatorSlotResource.java +++ b/airship-coordinator/src/test/java/io/airlift/airship/coordinator/TestCoordinatorSlotResource.java @@ -50,20 +50,21 @@ public void setUp() throws Exception { NodeInfo nodeInfo = new NodeInfo("testing"); + CoordinatorConfig config = new CoordinatorConfig().setStatusExpiration(new Duration(1, TimeUnit.DAYS)); repository = new TestingMavenRepository(); provisioner = new MockProvisioner(); coordinator = new Coordinator(nodeInfo, new HttpServerInfo(new HttpServerConfig(), nodeInfo), - new CoordinatorConfig().setStatusExpiration(new Duration(1, TimeUnit.DAYS)), + config, provisioner.getCoordinatorFactory(), provisioner.getAgentFactory(), repository, provisioner, new InMemoryStateManager(), new MockServiceInventory()); - resource = new CoordinatorSlotResource(coordinator, repository); + resource = new CoordinatorSlotResource(coordinator, repository, config); } @AfterMethod From f8491624fbca141bdf2e16e69b6213d45e61b8d4 Mon Sep 17 00:00:00 2001 From: Henning Schmiedehausen Date: Mon, 27 Oct 2014 11:24:14 -0700 Subject: [PATCH 2/2] code review feedback --- .../io/airlift/airship/coordinator/AdminResource.java | 6 +++--- .../airship/coordinator/CoordinatorSlotResource.java | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java index 62589f97..ac74b405 100644 --- a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java +++ b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/AdminResource.java @@ -32,14 +32,14 @@ public class AdminResource { private final Coordinator coordinator; private final Repository repository; - private final CoordinatorConfig config; + private final boolean allowDuplicateInstallationsOnAnAgent; @Inject public AdminResource(Coordinator coordinator, Repository repository, CoordinatorConfig config) { this.coordinator = coordinator; this.repository = repository; - this.config = config; + this.allowDuplicateInstallationsOnAnAgent = config.isAllowDuplicateInstallationsOnAnAgent(); } @GET @@ -82,7 +82,7 @@ public Response getAllAgents(@Context UriInfo uriInfo) Predicate agentPredicate = AgentFilterBuilder.build(uriInfo, transform(coordinator.getAgents(), idGetter()), transform(allSlotStatus, SlotStatus.uuidGetter()), - config.isAllowDuplicateInstallationsOnAnAgent(), + allowDuplicateInstallationsOnAnAgent, repository); List agents = coordinator.getAgents(agentPredicate); diff --git a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java index 9773b22a..07dd1999 100644 --- a/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java +++ b/airship-coordinator/src/main/java/io/airlift/airship/coordinator/CoordinatorSlotResource.java @@ -56,18 +56,18 @@ public class CoordinatorSlotResource private final Coordinator coordinator; private final Repository repository; - private final CoordinatorConfig config; + private final boolean allowDuplicateInstallationsOnAnAgent; @Inject public CoordinatorSlotResource(Coordinator coordinator, Repository repository, CoordinatorConfig config) { Preconditions.checkNotNull(coordinator, "coordinator must not be null"); Preconditions.checkNotNull(repository, "repository is null"); + Preconditions.checkNotNull(config, "coordinatorConfig is null"); this.coordinator = coordinator; this.repository = repository; - - this.config = Preconditions.checkNotNull(config, "coordinatorConfig is null"); + this.allowDuplicateInstallationsOnAnAgent = config.isAllowDuplicateInstallationsOnAnAgent(); } @GET @@ -105,7 +105,7 @@ public Response install( Predicate agentFilter = AgentFilterBuilder.build(uriInfo, transform(coordinator.getAgents(), idGetter()), transform(coordinator.getAllSlotStatus(), uuidGetter()), - config.isAllowDuplicateInstallationsOnAnAgent(), + allowDuplicateInstallationsOnAnAgent, repository); List agents = coordinator.getAgents(agentFilter);