Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent flapping slave from rejoining cluster #1428

Merged
merged 9 commits into from Mar 20, 2017
@@ -0,0 +1,43 @@
package com.hubspot.singularity.data;

import java.util.HashSet;
import java.util.Set;

import org.apache.curator.framework.CuratorFramework;

import com.codahale.metrics.MetricRegistry;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import com.hubspot.singularity.config.SingularityConfiguration;

@Singleton
public class InactiveSlaveManager extends CuratorManager {
private static final String ROOT_PATH = "/inactive";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit pick here, but could we have this be /inactiveSlaves? when looking at the top level in zk would be easier to tell what it's for


@Inject
public InactiveSlaveManager(CuratorFramework curator,
SingularityConfiguration configuration,
MetricRegistry metricRegistry) {
super(curator, configuration, metricRegistry);
}

public Set<String> getInactiveSlaves() {
return new HashSet<>(getChildren(ROOT_PATH));
}

public void deactivateSlave(String host) {
create(pathOf(host));
}

public void activateSlave(String host) {
delete(pathOf(host));
}

public boolean isInactive(String host) {
return exists(pathOf(host));
}

private String pathOf(String host) {
return String.format("%s/%s", ROOT_PATH, host);
}
}
Expand Up @@ -22,6 +22,7 @@ protected void configure() {
bind(RackManager.class).in(Scopes.SINGLETON);
bind(RequestManager.class).in(Scopes.SINGLETON);
bind(SlaveManager.class).in(Scopes.SINGLETON);
bind(InactiveSlaveManager.class).in(Scopes.SINGLETON);
bind(TaskRequestManager.class).in(Scopes.SINGLETON);
bind(SandboxManager.class).in(Scopes.SINGLETON);
bind(SingularityValidator.class).in(Scopes.SINGLETON);
Expand Down
Expand Up @@ -41,6 +41,7 @@
import com.hubspot.singularity.SlavePlacement;
import com.hubspot.singularity.config.SingularityConfiguration;
import com.hubspot.singularity.data.AbstractMachineManager;
import com.hubspot.singularity.data.InactiveSlaveManager;
import com.hubspot.singularity.data.RackManager;
import com.hubspot.singularity.data.SlaveManager;
import com.hubspot.singularity.data.TaskManager;
Expand All @@ -58,12 +59,14 @@ public class SingularitySlaveAndRackManager {
private final RackManager rackManager;
private final SlaveManager slaveManager;
private final TaskManager taskManager;
private final InactiveSlaveManager inactiveSlaveManager;
private final SingularitySlaveAndRackHelper slaveAndRackHelper;
private final AtomicInteger activeSlavesLost;

@Inject
SingularitySlaveAndRackManager(SingularitySlaveAndRackHelper slaveAndRackHelper, SingularityConfiguration configuration, SingularityExceptionNotifier exceptionNotifier,
RackManager rackManager, SlaveManager slaveManager, TaskManager taskManager, @Named(SingularityMesosModule.ACTIVE_SLAVES_LOST_COUNTER) AtomicInteger activeSlavesLost) {
RackManager rackManager, SlaveManager slaveManager, TaskManager taskManager, InactiveSlaveManager inactiveSlaveManager,
@Named(SingularityMesosModule.ACTIVE_SLAVES_LOST_COUNTER) AtomicInteger activeSlavesLost) {
this.configuration = configuration;

this.exceptionNotifier = exceptionNotifier;
Expand All @@ -72,6 +75,7 @@ public class SingularitySlaveAndRackManager {
this.rackManager = rackManager;
this.slaveManager = slaveManager;
this.taskManager = taskManager;
this.inactiveSlaveManager = inactiveSlaveManager;
this.activeSlavesLost = activeSlavesLost;
}

Expand Down Expand Up @@ -405,7 +409,14 @@ public void checkOffer(Offer offer) {
final SingularitySlave slave = new SingularitySlave(slaveId, host, rackId, textAttributes, Optional.<MesosResourcesObject>absent());

if (check(slave, slaveManager) == CheckResult.NEW) {
LOG.info("Offer revealed a new slave {}", slave);
if (inactiveSlaveManager.isInactive(slave.getHost())) {
LOG.info("Slave {} on inactive host {} attempted to rejoin. Marking as decommissioned.", slave, host);
slaveManager.changeState(slave, MachineState.STARTING_DECOMMISSION,
Optional.of(String.format("Slave %s on inactive host %s attempted to rejoin cluster.", slaveId, host)),
Optional.<String>absent());
} else {
LOG.info("Offer revealed a new slave {}", slave);
}
}

final SingularityRack rack = new SingularityRack(rackId);
Expand Down
@@ -0,0 +1,57 @@
package com.hubspot.singularity.resources;

import java.util.Set;

import javax.ws.rs.DELETE;
import javax.ws.rs.GET;
import javax.ws.rs.POST;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.QueryParam;
import javax.ws.rs.core.MediaType;

import com.google.common.base.Optional;
import com.google.inject.Inject;
import com.hubspot.singularity.SingularityService;
import com.hubspot.singularity.SingularityUser;
import com.hubspot.singularity.auth.SingularityAuthorizationHelper;
import com.hubspot.singularity.data.InactiveSlaveManager;
import com.wordnik.swagger.annotations.Api;

@Path(InactiveSlaveResource.PATH)
@Produces(MediaType.APPLICATION_JSON)
@Api(description="Manages Singularity Deploys for existing requests", value=DisastersResource.PATH)
public class InactiveSlaveResource {
public static final String PATH = SingularityService.API_BASE_PATH + "/inactive";

private final InactiveSlaveManager inactiveSlaveManager;
private final SingularityAuthorizationHelper authorizationHelper;
private final Optional<SingularityUser> user;


@Inject
public InactiveSlaveResource(InactiveSlaveManager inactiveSlaveManager,
SingularityAuthorizationHelper authorizationHelper,
Optional<SingularityUser> user) {
this.inactiveSlaveManager = inactiveSlaveManager;
this.authorizationHelper = authorizationHelper;
this.user = user;
}

@GET
public Set<String> getInactiveSlaves() {
return inactiveSlaveManager.getInactiveSlaves();
}

@POST
public void deactivateSlave(@QueryParam("host") String host) {
authorizationHelper.checkAdminAuthorization(user);
inactiveSlaveManager.deactivateSlave(host);
}

@DELETE
public void reactivateSlave(@QueryParam("host") String host) {
authorizationHelper.checkAdminAuthorization(user);
inactiveSlaveManager.activateSlave(host);
}
}
Expand Up @@ -42,6 +42,7 @@ protected void configure() {
bind(DisastersResource.class);
bind(PriorityResource.class);
bind(RequestGroupResource.class);
bind(InactiveSlaveResource.class);

switch (uiConfiguration.getRootUrlMode()) {
case UI_REDIRECT: {
Expand Down
@@ -0,0 +1,37 @@
package com.hubspot.singularity.data;

import org.junit.Assert;
import org.junit.Test;

import com.hubspot.singularity.SingularitySlave;
import com.hubspot.singularity.scheduler.SingularitySchedulerTestBase;

public class InactiveSlaveManagerTest extends SingularitySchedulerTestBase {
public InactiveSlaveManagerTest() {
super(false);
}

@Test
public void itShouldContainAnInactiveHostWhenHostDeactivated() {
inactiveSlaveManager.deactivateSlave("localhost");

Assert.assertTrue(inactiveSlaveManager.getInactiveSlaves().contains("localhost"));
}

@Test
public void itShouldNotContainHostAfterActivatingHost() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to extend these tests, you can have the class extend SingularitySchedulerTestBase. In there is a resourceOffers method which mocks the sending of an offer, allowing you to trigger the code path of getting an offer from a new slave.

inactiveSlaveManager.deactivateSlave("localhost");
inactiveSlaveManager.activateSlave("localhost");

Assert.assertFalse(inactiveSlaveManager.getInactiveSlaves().contains("localhost"));
}

@Test
public void itShouldMarkSlavesFromInactiveHostAsDecommissioned() {
inactiveSlaveManager.deactivateSlave("host1");

resourceOffers();
SingularitySlave slave = slaveManager.getObject("slave1").get();
Assert.assertTrue(slave.getCurrentState().getState().isDecommissioning());
}
}
Expand Up @@ -74,6 +74,7 @@
import com.hubspot.singularity.config.SingularityConfiguration;
import com.hubspot.singularity.config.SingularityTaskMetadataConfiguration;
import com.hubspot.singularity.data.DeployManager;
import com.hubspot.singularity.data.InactiveSlaveManager;
import com.hubspot.singularity.data.PriorityManager;
import com.hubspot.singularity.data.RackManager;
import com.hubspot.singularity.data.RequestManager;
Expand Down Expand Up @@ -111,6 +112,8 @@ public class SingularitySchedulerTestBase extends SingularityCuratorTestBase {
@Inject
protected RackManager rackManager;
@Inject
protected InactiveSlaveManager inactiveSlaveManager;
@Inject
protected SchedulerDriverSupplier driverSupplier;
protected SchedulerDriver driver;
@Inject
Expand Down
22 changes: 22 additions & 0 deletions SingularityUI/app/actions/api/inactive.es6
@@ -0,0 +1,22 @@
import { buildApiAction } from './base';

export const DeactivateHost = buildApiAction(
'DEACTIVATE_HOST',
(host) => ({
method: 'POST',
url: `/inactive?host=${host}`
})
);

export const ReactivateHost = buildApiAction(
'REACTIVATE_HOST',
(host) => ({
method: 'DELETE',
url: `/inactive?host=${host}`
})
);

export const FetchInactiveHosts = buildApiAction(
'FETCH_INACTIVE_HOST',
{url: '/inactive'}
);