Merge pull request #2275 from HubSpot/deploy-validation

Prevent accidental large scale downs

Author: pschoenfelder

SingularityClient/src/main/java/com/hubspot/singularity/client/SingularityClient.java

@@ -1160,7 +1160,25 @@ public SingularityRequestParent createDeployForSingularityRequest(
     SingularityDeploy pendingDeploy,
     Optional<Boolean> deployUnpause,
     Optional<String> message,
-    Optional<SingularityRequest> updatedRequest
+    Optional<Boolean> largeScaleDownAcknowledged
+  ) {
+    return createDeployForSingularityRequest(
+      requestId,
+      pendingDeploy,
+      deployUnpause,
+      message,
+      Optional.empty(),
+      largeScaleDownAcknowledged
+    );
+  }
+
+  public SingularityRequestParent createDeployForSingularityRequest(
+    String requestId,
+    SingularityDeploy pendingDeploy,
+    Optional<Boolean> deployUnpause,
+    Optional<String> message,
+    Optional<SingularityRequest> updatedRequest,
+    Optional<Boolean> largeScaleDownAcknowledged
   ) {
     final Function<String, String> requestUri = (String host) ->
       String.format(DEPLOYS_FORMAT, getApiBase(host));
@@ -1178,6 +1196,10 @@ public SingularityRequestParent createDeployForSingularityRequest(
           message,
           updatedRequest
         )
+      ),
+      Collections.singletonMap(
+        "largeScaleDownAcknowledged",
+        largeScaleDownAcknowledged.orElse(false)
       )
     );
 

SingularityService/src/main/java/com/hubspot/singularity/config/SingularityConfiguration.java

@@ -237,6 +237,8 @@ public class SingularityConfiguration extends Configuration {
 
   private int maxUserIdSize = 100;
 
+  private int maxScaleDownWithoutAcknowledgement = 10;
+
   private boolean storeAllMesosTaskInfoForDebugging = false;
 
   @JsonProperty("historyPurging")
@@ -832,6 +834,10 @@ public int getMaxUserIdSize() {
     return maxUserIdSize;
   }
 
+  public int getMaxScaleDownWithoutAcknowledgement() {
+    return maxScaleDownWithoutAcknowledgement;
+  }
+
   public int getMaxTasksPerOffer() {
     return maxTasksPerOffer;
   }
@@ -1245,6 +1251,12 @@ public void setMaxTasksPerOfferPerRequest(int maxTasksPerOfferPerRequest) {
     this.maxTasksPerOfferPerRequest = maxTasksPerOfferPerRequest;
   }
 
+  public void setMaxScaleDownWithoutAcknowledgement(
+    int maxScaleDownWithoutAcknowledgement
+  ) {
+    this.maxScaleDownWithoutAcknowledgement = maxScaleDownWithoutAcknowledgement;
+  }
+
   public void setMesosConfiguration(MesosConfiguration mesosConfiguration) {
     this.mesosConfiguration = mesosConfiguration;
   }

SingularityService/src/main/java/com/hubspot/singularity/data/SingularityValidator.java

@@ -1209,7 +1209,11 @@ private boolean isAllowBounceToSameHost(SingularityRequest request) {
     }
   }
 
-  public void checkScale(SingularityRequest request, Optional<Integer> previousScale) {
+  public void checkScale(
+    SingularityRequest request,
+    Optional<Integer> previousScale,
+    Optional<Boolean> largeScaleDownAcknowledged
+  ) {
     AgentPlacement placement = request.getAgentPlacement().orElse(defaultAgentPlacement);
 
     if (placement != AgentPlacement.GREEDY && placement != AgentPlacement.OPTIMISTIC) {
@@ -1239,6 +1243,25 @@ public void checkScale(SingularityRequest request, Optional<Integer> previousSca
         );
       }
     }
+
+    if (previousScale.isPresent() && !largeScaleDownAcknowledged.orElse(false)) {
+      int absMaxScaleDown = singularityConfiguration.getMaxScaleDownWithoutAcknowledgement();
+      boolean scaleDownExceedsAbsoluteMax =
+        previousScale.get() - request.getInstancesSafe() > absMaxScaleDown;
+      boolean scaleDownExceedsRelativeMax =
+        request.getInstancesSafe() < (previousScale.get() / 2);
+      checkBadRequest(
+        !scaleDownExceedsAbsoluteMax,
+        "Cannot scale down by more than %s instances at a time without explicit " +
+        "acknowledgement (set the largeScaleDownAcknowledged field in the request)",
+        absMaxScaleDown
+      );
+      checkBadRequest(
+        !(previousScale.get() > absMaxScaleDown && scaleDownExceedsRelativeMax),
+        "Cannot scale down by more than half of current instances without explicit " +
+        "acknowledgement (set the largeScaleDownAcknowledged field in the request)"
+      );
+    }
   }
 
   public void validateExpiringMachineStateChange(

SingularityService/src/main/java/com/hubspot/singularity/resources/DeployResource.java

@@ -5,6 +5,7 @@
 import static com.hubspot.singularity.WebExceptions.checkNotNullBadRequest;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.inject.Inject;
 import com.hubspot.jackson.jaxrs.PropertyFiltering;
 import com.hubspot.singularity.DeployState;
@@ -56,6 +57,7 @@
 import javax.ws.rs.Path;
 import javax.ws.rs.PathParam;
 import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import org.apache.curator.framework.recipes.leader.LeaderLatch;
@@ -131,19 +133,29 @@ public SingularityRequestParent deploy(
     @RequestBody(
       required = true,
       description = "Deploy data"
-    ) SingularityDeployRequest deployRequest
+    ) SingularityDeployRequest deployRequest,
+    @QueryParam("largeScaleDownAcknowledged") Optional<Boolean> largeScaleDownAcknowledged
   ) {
     return maybeProxyToLeader(
       requestContext,
       SingularityRequestParent.class,
       deployRequest,
-      () -> deploy(deployRequest, user)
+      () -> deploy(deployRequest, user, largeScaleDownAcknowledged)
     );
   }
 
+  @VisibleForTesting
   public SingularityRequestParent deploy(
     SingularityDeployRequest deployRequest,
     SingularityUser user
+  ) {
+    return deploy(deployRequest, user, Optional.empty());
+  }
+
+  public SingularityRequestParent deploy(
+    SingularityDeployRequest deployRequest,
+    SingularityUser user,
+    Optional<Boolean> largeScaleDownAcknowledged
   ) {
     validator.checkActionEnabled(SingularityAction.DEPLOY);
     SingularityDeploy deploy = deployRequest.getDeploy();
@@ -197,7 +209,8 @@ public SingularityRequestParent deploy(
 
     validator.checkScale(
       request,
-      Optional.of(taskManager.getActiveTaskIdsForRequest(request.getId()).size())
+      Optional.of(taskManager.getActiveTaskIdsForRequest(request.getId()).size()),
+      largeScaleDownAcknowledged
     );
 
     if (

SingularityService/src/main/java/com/hubspot/singularity/resources/RequestResource.java

@@ -168,7 +168,8 @@ private void submitRequest(
     Optional<Boolean> skipHealthchecks,
     Optional<String> message,
     Optional<SingularityBounceRequest> maybeBounceRequest,
-    SingularityUser user
+    SingularityUser user,
+    Optional<Boolean> largeScaleDownAcknowledged
   ) {
     checkNotNullBadRequest(request.getId(), "Request must have an id");
     checkConflict(
@@ -224,11 +225,14 @@ private void submitRequest(
         request.toBuilder().setInstances(Optional.of(currentActiveAgentCount)).build();
     }
 
-    if (
-      !oldRequest.isPresent() ||
-      !(oldRequest.get().getInstancesSafe() == request.getInstancesSafe())
-    ) {
-      validator.checkScale(request, Optional.empty());
+    if (!oldRequest.isPresent()) {
+      validator.checkScale(request, Optional.empty(), largeScaleDownAcknowledged);
+    } else if (!(oldRequest.get().getInstancesSafe() == request.getInstancesSafe())) {
+      validator.checkScale(
+        request,
+        Optional.of(oldRequest.get().getInstancesSafe()),
+        largeScaleDownAcknowledged
+      );
     }
 
     authorizationHelper.checkForAuthorization(
@@ -383,7 +387,8 @@ public SingularityRequestParent postRequest(
       Optional.empty(),
       Optional.empty(),
       Optional.empty(),
-      user
+      user,
+      Optional.empty()
     );
     return fillEntireRequest(fetchRequestWithState(request.getId(), user));
   }
@@ -463,7 +468,8 @@ private SingularityRequestParent updateAuthorizedGroups(
       Optional.empty(),
       updateGroupsRequest.getMessage(),
       Optional.empty(),
-      user
+      user,
+      Optional.empty()
     );
     return fillEntireRequest(fetchRequestWithState(requestId, user));
   }
@@ -1735,7 +1741,8 @@ public SingularityRequestParent setPriority(
       Optional.of(false),
       Optional.of(message),
       Optional.empty(),
-      user
+      user,
+      Optional.empty()
     );
 
     if (priorityRequest.getDurationMillis().isPresent()) {
@@ -1775,20 +1782,30 @@ public SingularityRequestParent scale(
     @RequestBody(
       required = true,
       description = "Object to hold number of instances to request"
-    ) SingularityScaleRequest scaleRequest
+    ) SingularityScaleRequest scaleRequest,
+    @QueryParam("largeScaleDownAcknowledged") Optional<Boolean> largeScaleDownAcknowledged
   ) {
     return maybeProxyToLeader(
       requestContext,
       SingularityRequestParent.class,
       scaleRequest,
-      () -> scale(requestId, scaleRequest, user)
+      () -> scale(requestId, scaleRequest, user, largeScaleDownAcknowledged)
     );
   }
 
   public SingularityRequestParent scale(
     String requestId,
     SingularityScaleRequest scaleRequest,
     SingularityUser user
+  ) {
+    return scale(requestId, scaleRequest, user, Optional.empty());
+  }
+
+  public SingularityRequestParent scale(
+    String requestId,
+    SingularityScaleRequest scaleRequest,
+    SingularityUser user,
+    Optional<Boolean> largeScaleDownAcknowledged
   ) {
     SingularityRequestWithState oldRequestWithState = fetchRequestWithState(
       requestId,
@@ -1808,7 +1825,11 @@ public SingularityRequestParent scale(
       .toBuilder()
       .setInstances(scaleRequest.getInstances())
       .build();
-    validator.checkScale(newRequest, Optional.<Integer>empty());
+    validator.checkScale(
+      newRequest,
+      oldRequest.getInstances(),
+      largeScaleDownAcknowledged
+    );
 
     checkBadRequest(
       oldRequest.getInstancesSafe() != newRequest.getInstancesSafe(),
@@ -1871,7 +1892,8 @@ public SingularityRequestParent scale(
         scaleRequest.getSkipHealthchecks(),
         Optional.of(scaleMessage),
         Optional.of(bounceRequest),
-        user
+        user,
+        largeScaleDownAcknowledged
       );
     } else {
       submitRequest(
@@ -1881,7 +1903,8 @@ public SingularityRequestParent scale(
         scaleRequest.getSkipHealthchecks(),
         Optional.of(scaleMessage),
         Optional.empty(),
-        user
+        user,
+        largeScaleDownAcknowledged
       );
     }
 
@@ -2117,7 +2140,8 @@ public SingularityRequestParent skipHealthchecks(
       Optional.empty(),
       skipHealthchecksRequest.getMessage(),
       Optional.empty(),
-      user
+      user,
+      Optional.empty()
     );
 
     if (skipHealthchecksRequest.getDurationMillis().isPresent()) {

SingularityService/src/test/java/com/hubspot/singularity/scheduler/SingularitySchedulerTest.java

@@ -2338,7 +2338,7 @@ public void testBounceReleasesLockWithAlternateCleanupType() {
   @Test
   public void testIncrementalBounce() {
     initRequest();
-    resourceOffers(2); // set up agents so scale validate will pass
+    resourceOffers(3); // set up agents so scale validate will pass
 
     SingularityRequest request = requestResource
       .getRequest(requestId, singularityUser)

SingularityUI/app/actions/api/requests.es6

@@ -137,8 +137,8 @@ export const PersistSkipRequestHealthchecks = buildJsonApiAction(
 export const ScaleRequest = buildJsonApiAction(
   'SCALE_REQUEST',
   'PUT',
-  (requestId, {instances, skipHealthchecks, durationMillis, message, actionId, bounce, incremental }) => ({
-    url: `/requests/request/${requestId}/scale`,
+  (requestId, {instances, skipHealthchecks, durationMillis, message, actionId, bounce, incremental, largeScaleDownAcknowledged }) => ({
+    url: `/requests/request/${requestId}/scale?largeScaleDownAcknowledged=${largeScaleDownAcknowledged}`,
     body: { instances, skipHealthchecks, durationMillis, message, actionId, bounce, incremental }
   })
 );

SingularityUI/app/components/common/modal/FormModal.jsx

@@ -45,13 +45,17 @@ export default class FormModal extends React.Component {
   }
 
   static FormItem = (props) => {
-    if ((props.element.dependsOn && props.formState[props.element.dependsOn]) || !props.element.dependsOn) {
+    const noDepends = (!props.element.dependsOn && !props.element.dependsOnFormState);
+    const dependsOnOk = (props.element.dependsOn && props.formState[props.element.dependsOn]);
+    const dependsOnFormStateOk = (props.element.dependsOnFormState && props.element.dependsOnFormState(props.formState));
+    if (noDepends || dependsOnOk || dependsOnFormStateOk) {
       return (
         <div className={classNames(props.className, {'childItem': props.formState[props.element.dependsOn]})}>
           {props.children}
         </div>
       );
     }
+
     return null;
   };
 
@@ -502,6 +506,7 @@ FormModal.propTypes = {
     values: React.PropTypes.array,
     defaultValue: React.PropTypes.oneOfType([React.PropTypes.string, React.PropTypes.bool, React.PropTypes.number, React.PropTypes.array]),
     validateField: React.PropTypes.func, // String -> String, return field validation error or falsey value if valid
-    dependsOn: React.PropTypes.string // Only show this item if the other item (referenced by name) has a truthy value
+    dependsOn: React.PropTypes.string, // Only show this item if the other item (referenced by name) has a truthy value
+    dependsOnFormState: React.PropTypes.func, // Only show this item if this function applied to form state returns a truthy value
   })).isRequired
 };

SingularityUI/app/components/common/modalButtons/ScaleModal.jsx

@@ -39,15 +39,16 @@ class ScaleModal extends Component {
   }
 
   handleScale(data) {
-    const { instances, durationMillis, message, bounce, incremental } = data;
+    const { instances, durationMillis, message, bounce, incremental, largeScaleDownAcknowledged } = data;
     const isIncremental = incremental === 'incremental';
     this.props.scaleRequest(
       {
         instances,
         durationMillis,
         message,
         bounce,
-        incremental: isIncremental
+        incremental: isIncremental,
+        largeScaleDownAcknowledged
       }
     );
   }
@@ -95,6 +96,18 @@ class ScaleModal extends Component {
             name: 'message',
             type: FormModal.INPUT_TYPES.STRING,
             label: 'Message: (optional)'
+          },
+          {
+            name: 'largeScaleDownAcknowledged',
+            type: FormModal.INPUT_TYPES.BOOLEAN,
+            label: 'Explciit acknowledgement of large scale down (less than -10 or 1/2 of previous)',
+            defaultValue: false,
+            dependsOnFormState: data => {
+              const scaleDownExceedsAbsoluteMax = (this.props.currentInstances - data.instances) > 10;
+              const scaleDownExceedsRelativeMax = (this.props.currentInstances > 10) && (data.instances < (this.props.currentInstances / 2));
+              // window.config flag in case the numbers change at some point
+              return scaleDownExceedsAbsoluteMax || scaleDownExceedsRelativeMax || window.config.largeScaleDownAcknowledged;
+            },
           }
         ]}>
         <p>Scaling request:</p>