Skip to content

Commit

Permalink
fix(monitored deploy): don't store any context in the task (#3184)
Browse files Browse the repository at this point in the history 
Somehow... I forgot that tasks are singletons and shouldn't have any state stored...
Simple refactor to pass along the vars that are needed all the time
  • Loading branch information
@marchello2000
marchello2000 committed Sep 20, 2019
1 parent aa5a675 commit dea4a7f
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 35 deletions.
15 changes: 11 additions & 4 deletions ...etflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,14 @@

import com.netflix.spectator.api.Id;
import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.config.DeploymentMonitorServiceProvider;
import com.netflix.spinnaker.orca.ExecutionStatus;
import com.netflix.spinnaker.orca.TaskResult;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.MonitoredDeployStageData;
import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthRequest;
import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse;
import com.netflix.spinnaker.orca.pipeline.model.Stage;
import java.time.Instant;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nonnull;
Expand All @@ -40,15 +43,18 @@ public class EvaluateDeploymentHealthTask extends MonitoredDeployBaseTask {
}

@Override
public @Nonnull TaskResult executeInternal() {
public @Nonnull TaskResult executeInternal(
Stage stage,
MonitoredDeployStageData context,
DeploymentMonitorDefinition monitorDefinition) {
EvaluateHealthRequest request = new EvaluateHealthRequest(stage);

log.info(
"Evaluating health of deployment {} at {}% with {}",
request.getNewServerGroup(), request.getCurrentProgress(), monitorDefinition);

EvaluateHealthResponse response = monitorDefinition.getService().evaluateHealth(request);
sanitizeAndLogResponse(response);
sanitizeAndLogResponse(response, monitorDefinition, stage.getExecution().getId());

EvaluateHealthResponse.NextStepDirective directive = response.getNextStep().getDirective();

Expand All @@ -73,11 +79,12 @@ public class EvaluateDeploymentHealthTask extends MonitoredDeployBaseTask {
registry.timer(timerId).record(duration, TimeUnit.MILLISECONDS);
}

return buildTaskResult(processDirective(directive), response);
return buildTaskResult(processDirective(directive, monitorDefinition), response);
}

private TaskResult.TaskResultBuilder processDirective(
EvaluateHealthResponse.NextStepDirective directive) {
EvaluateHealthResponse.NextStepDirective directive,
DeploymentMonitorDefinition monitorDefinition) {
switch (directive) {
case COMPLETE:
// TODO(mvulfson): Actually implement this case in the stages
Expand Down
58 changes: 32 additions & 26 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,7 @@
public class MonitoredDeployBaseTask implements RetryableTask {
private static final int MAX_RETRY_COUNT = 3;
protected final Logger log = LoggerFactory.getLogger(getClass());
protected DeploymentMonitorDefinition monitorDefinition;
protected Stage stage;
protected Registry registry;

private DeploymentMonitorServiceProvider deploymentMonitorServiceProvider;

MonitoredDeployBaseTask(
Expand All @@ -73,7 +70,7 @@ public long getTimeout() {

@Override
public long getDynamicTimeout(Stage stage) {
ensureMonitorDefinition(stage);
DeploymentMonitorDefinition monitorDefinition = getDeploymentMonitorDefinition(stage);

final Duration defaultTimeout = Duration.ofMinutes(60);
long timeout;
Expand All @@ -95,10 +92,9 @@ public long getDynamicTimeout(Stage stage) {

@Override
public @Nullable TaskResult onTimeout(@Nonnull Stage stage) {
ensureMonitorDefinition(stage);

ExecutionStatus taskStatus;
String message;
DeploymentMonitorDefinition monitorDefinition = getDeploymentMonitorDefinition(stage);

if (monitorDefinition.isFailOnError()) {
message =
Expand All @@ -115,11 +111,11 @@ public long getDynamicTimeout(Stage stage) {

@Override
public @Nonnull TaskResult execute(@Nonnull Stage stage) {
MonitoredDeployStageData context = stage.mapTo(MonitoredDeployStageData.class);
MonitoredDeployStageData context = getStageContext(stage);
DeploymentMonitorDefinition monitorDefinition = getDeploymentMonitorDefinition(stage);

try {
ensureMonitorDefinition(stage);
return executeInternal();
return executeInternal(stage, context, monitorDefinition);
} catch (RetrofitError e) {
log.warn(
"HTTP Error encountered while talking to {}->{}, {}}",
Expand All @@ -128,10 +124,10 @@ public long getDynamicTimeout(Stage stage) {
getRetrofitLogMessage(e.getResponse()),
e);

return handleError(context, e, true);
return handleError(context, e, true, monitorDefinition);
} catch (DeploymentMonitorInvalidDataException e) {

return handleError(context, e, false);
return handleError(context, e, false, monitorDefinition);
} catch (Exception e) {
log.error("Exception while executing {}, aborting deployment", getClass().getSimpleName(), e);

Expand All @@ -140,12 +136,18 @@ public long getDynamicTimeout(Stage stage) {
}
}

public @Nonnull TaskResult executeInternal() {
public @Nonnull TaskResult executeInternal(
Stage stage,
MonitoredDeployStageData context,
DeploymentMonitorDefinition monitorDefinition) {
throw new UnsupportedOperationException("Must implement executeInternal method");
}

private TaskResult handleError(
MonitoredDeployStageData context, Exception e, boolean retryAllowed) {
MonitoredDeployStageData context,
Exception e,
boolean retryAllowed,
DeploymentMonitorDefinition monitorDefinition) {
registry
.counter("deploymentMonitor.errors", "monitorId", monitorDefinition.getId())
.increment();
Expand Down Expand Up @@ -199,16 +201,6 @@ private TaskResult handleError(
return buildTaskResult(TaskResult.builder(ExecutionStatus.SUCCEEDED), userMessage);
}

private void ensureMonitorDefinition(Stage stage) {
if (this.stage == null) {
MonitoredDeployStageData context = stage.mapTo(MonitoredDeployStageData.class);
this.stage = stage;
this.monitorDefinition =
deploymentMonitorServiceProvider.getDefinitionById(
context.getDeploymentMonitor().getId());
}
}

TaskResult buildTaskResult(
TaskResult.TaskResultBuilder taskResultBuilder, EvaluateHealthResponse response) {
List<StatusReason> statusReasons =
Expand All @@ -226,6 +218,17 @@ TaskResult buildTaskResult(TaskResult.TaskResultBuilder taskResultBuilder, Strin
return taskResultBuilder.context("deploymentMonitorReasons", explanation).build();
}

private DeploymentMonitorDefinition getDeploymentMonitorDefinition(Stage stage) {
MonitoredDeployStageData context = getStageContext(stage);

return deploymentMonitorServiceProvider.getDefinitionById(
context.getDeploymentMonitor().getId());
}

private MonitoredDeployStageData getStageContext(Stage stage) {
return stage.mapTo(MonitoredDeployStageData.class);
}

private String getRetrofitLogMessage(Response response) {
if (response == null) {
return "<NO RESPONSE>";
Expand All @@ -250,7 +253,10 @@ private String getRetrofitLogMessage(Response response) {
return String.format("status: %s\nheaders: %s\nresponse body: %s", status, headers, body);
}

void sanitizeAndLogResponse(EvaluateHealthResponse response) {
void sanitizeAndLogResponse(
EvaluateHealthResponse response,
DeploymentMonitorDefinition monitorDefinition,
String executionId) {
if (response.getNextStep() == null) {
log.error("Deployment monitor {}: returned null nextStep", monitorDefinition);

Expand Down Expand Up @@ -278,7 +284,7 @@ void sanitizeAndLogResponse(EvaluateHealthResponse response) {
monitorDefinition,
nextStepDirective,
this.getClass().getSimpleName(),
stage.getExecution().getId());
executionId);
break;

case CONTINUE:
Expand All @@ -287,7 +293,7 @@ void sanitizeAndLogResponse(EvaluateHealthResponse response) {
monitorDefinition,
nextStepDirective,
this.getClass().getSimpleName(),
stage.getExecution().getId());
executionId);
break;

default:
Expand Down
8 changes: 7 additions & 1 deletion ...m/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/NotifyDeployCompletedTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@
package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy;

import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.config.DeploymentMonitorServiceProvider;
import com.netflix.spinnaker.orca.ExecutionStatus;
import com.netflix.spinnaker.orca.TaskResult;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.MonitoredDeployStageData;
import com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentCompletedRequest;
import com.netflix.spinnaker.orca.pipeline.model.Stage;
import javax.annotation.Nonnull;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
Expand All @@ -36,7 +39,10 @@ public class NotifyDeployCompletedTask extends MonitoredDeployBaseTask {
}

@Override
public @Nonnull TaskResult executeInternal() {
public @Nonnull TaskResult executeInternal(
Stage stage,
MonitoredDeployStageData context,
DeploymentMonitorDefinition monitorDefinition) {
// TODO(mvulfson): actually populate the request data
DeploymentCompletedRequest request = new DeploymentCompletedRequest(stage);
monitorDefinition.getService().notifyCompleted(request);
Expand Down
16 changes: 12 additions & 4 deletions ...om/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/NotifyDeployStartingTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@
package com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy;

import com.netflix.spectator.api.Registry;
import com.netflix.spinnaker.config.DeploymentMonitorDefinition;
import com.netflix.spinnaker.config.DeploymentMonitorServiceProvider;
import com.netflix.spinnaker.orca.ExecutionStatus;
import com.netflix.spinnaker.orca.TaskResult;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.strategies.MonitoredDeployStageData;
import com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse;
import com.netflix.spinnaker.orca.deploymentmonitor.models.RequestBase;
import com.netflix.spinnaker.orca.pipeline.model.Stage;
import javax.annotation.Nonnull;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
Expand All @@ -37,17 +40,22 @@ public class NotifyDeployStartingTask extends MonitoredDeployBaseTask {
}

@Override
public @Nonnull TaskResult executeInternal() {
public @Nonnull TaskResult executeInternal(
Stage stage,
MonitoredDeployStageData context,
DeploymentMonitorDefinition monitorDefinition) {
RequestBase request = new RequestBase(stage);
EvaluateHealthResponse response = monitorDefinition.getService().notifyStarting(request);

sanitizeAndLogResponse(response);
sanitizeAndLogResponse(response, monitorDefinition, stage.getExecution().getId());

return buildTaskResult(processDirective(response.getNextStep().getDirective()), response);
return buildTaskResult(
processDirective(response.getNextStep().getDirective(), monitorDefinition), response);
}

private TaskResult.TaskResultBuilder processDirective(
EvaluateHealthResponse.NextStepDirective directive) {
EvaluateHealthResponse.NextStepDirective directive,
DeploymentMonitorDefinition monitorDefinition) {
switch (directive) {
case COMPLETE:
log.warn(
Expand Down

0 comments on commit dea4a7f

Please sign in to comment.