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

Overrides for run now requests #1650

Merged
merged 27 commits into from Dec 7, 2017
Merged

Overrides for run now requests #1650

merged 27 commits into from Dec 7, 2017

Conversation

pschoenfelder
Copy link
Contributor

No description provided.

@pschoenfelder
Copy link
Contributor Author

Full disclosure - I have no idea if this touches all the things it needs to. I added a unit test to check that environment variable overrides specified in a run-now request are propagated through to a Mesos task, but I don't know if this covers all the workflows (I doubt it does). Do you know if/what other things need to pick up on the overrides?

@pschoenfelder
Copy link
Contributor Author

Also, I added a builder for run-now requests for the unit tests since there was a lot of Optional.absent() boilerplate going on, but I only added it to the test package. Would it be useful elsewhere?

import com.hubspot.mesos.Resources;
import com.hubspot.singularity.api.SingularityRunNowRequest;

public class SingularityRunNowRequestBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to add this to SingularityBase 👍 . For context, we've debated moving to immutables to get more of our builders/etc generated automatically, but haven't made the move yet. For any classes whose constructors start to get larger, it's worth making a builder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! Will move it

@@ -15,16 +16,18 @@
private final Optional<List<String>> commandLineArgs;
private final Optional<Boolean> skipHealthchecks;
private final Optional<Resources> resources;
private final Optional<Map<String, String>> environmentVariables;
Copy link
Member

Choose a reason for hiding this comment

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

maybe environmentOverrides instead? I think we would want these to take precedence over the data in the SingularityDeploy in the case where there are two keys with different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - definitely more clear

Copy link
Contributor

Choose a reason for hiding this comment

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

SingularityTask uses env for the env var map -- should we follow that convention?

Copy link
Member

Choose a reason for hiding this comment

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

Good point, SingularityDeploy does as well.

@@ -33,12 +36,14 @@ public SingularityRunNowRequest(@JsonProperty("message") Optional<String> messag
@JsonProperty("runId") Optional<String> runId,
@JsonProperty("commandLineArgs") Optional<List<String>> commandLineArgs,
@JsonProperty("resources") Optional<Resources> resources,
@JsonProperty("environmentVariables") Optional<Map<String, String>> environmentVariables,
Copy link
Member

Choose a reason for hiding this comment

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

Rather than having an optional map, it makes more sense to just default it to empty. Maps have a nice default state of empty that we can use to keep things cleaner and cut down on all of the isPresent checks everywhere. So, in the constructor it'd be something like this. environmentOverrides = environmentOverrides == null ? Collections.emptyMap() : environmentOverrides

@@ -163,63 +166,76 @@ private boolean hasLiteralPortMapping(Optional<SingularityContainerInfo> maybeCo
return maybeContainerInfo.isPresent() && maybeContainerInfo.get().getDocker().isPresent() && !maybeContainerInfo.get().getDocker().get().getLiteralHostPorts().isEmpty();
}

private void setEnv(Environment.Builder envBldr, String key, Object value) {
private void setEnv(Environment.Builder envBldr, Object key, Object value) {
Copy link
Member

Choose a reason for hiding this comment

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

Why change this to Object from String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think it was more useful as I was developing. Looking at it now though, no good reason. I'll revert it.

}

Environment.Builder envBldr = Environment.newBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

It seems like duplicate work to do this at the end. The helpful piece of the builder is that we can put it together along the way and not have to iterate through the map again, which is why there are all of the setEnv calls above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My unit test was catching an error scenario where an overridden variable was duplicated in the Environment.Builder. I didn't find anything in the docs to indicate that there's anything preventing this or a clean way to handle dupes, so I stuck everything in a map first. Do you know if there's a better way to do this without the map?

@@ -138,6 +141,40 @@ public void testJobUserPassedAsEnvironmentVariable() {
}

@Test
public void testEnvironmentVariableOverrides() {
Map<String, String> overrideVariables = new HashMap<>();
overrideVariables.put("HS_ACCESS_GROUP", "test");
Copy link
Member

Choose a reason for hiding this comment

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

generally try to keep HubSpot-specific things out of Singularity, even for tests ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good catch - didn't think of that

final SingularityTaskRequest taskRequest = new SingularityTaskRequest(request, deploy, pendingTask);
final SingularityTask task = builder.buildTask(offerHolder, null, taskRequest, taskResources, executorResources);

Map<String, String> environmentVariables = task.getMesosTask()
Copy link
Member

Choose a reason for hiding this comment

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

could probably just do the for loop here. for (Variable var : task.getMesosTask().getCommand().getEnvironment().getVariablesList()) and just do the assert equals against getName/getKey. Saves you streaming to a map, only to then iterate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but the mesos task variables are a superset of the override variables, so the loop would require some kind of checking like this:

    Boolean checkedStartedByUser = false;

    for (Variable var : task.getMesosTask().getCommand().getEnvironment().getVariablesList()) {
      if (overrideVariables.containsKey(var.getName())) {
        if (var.getName().equals("MY_NEW_ENV_VAR")) {
          checkedMyNewEnvVar = true;
        } else if (var.getName().equals("STARTED_BY_USER")) {
          checkedStartedByUser = true;
        }

        assertEquals(
            "Environment variable " + var + " not overridden.",
            var.getValue(),
            overrideVariables.get(var.getName()));
      }
    }
    assertTrue(checkedMyNewEnvVar);
    assertTrue(checkedStartedByUser);

Which do you think is cleaner (or is there an even cleaner alternative?)?

.build();
final SingularityDeploy deploy = new SingularityDeployBuilder("test", "1")
.setCommand(Optional.of("/bin/echo hi"))
.setRunImmediately(Optional.of(runNowRequest))
Copy link
Member

Choose a reason for hiding this comment

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

This particular field specifies to run immediately with these settings upon successful completion of a deploy. In this case it isn't actually getting transferred to your pending task


if (task.getDeploy().getRunImmediately().isPresent()) {
SingularityRunNowRequest runNowRequest = task.getDeploy().getRunImmediately().get();
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned below, this is a field that specifies config to run immediately upon successful deploy. The SingularityPendingTask is where the environment override needs to be trasferred to first. So, something like:

  • New RunNowRequest comes in with environmentOverrides and the SingularityRunNowRequest is saved as part of the SingularityPendingRequest and put into the pending queue (triggered from the RequestResource
  • overrides from RunNowRequest saved as part of the SingularityPendingTask when it is created based on that pending request from the queue (this is the piece that is currently missing)
  • When the task builder is ready to build the task from the pending task, it can then read the environment overrides from the pending task and apply those here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Next diff will have changes that I believe satisfy this, though I'm not sure my implementation is as clean as it could be. Either way, let me know and I'll work through it.

.setCommand(Optional.of("/bin/echo hi"))
.setRunImmediately(Optional.of(runNowRequest))
.build();
final SingularityTaskRequest taskRequest = new SingularityTaskRequest(request, deploy, pendingTask);
Copy link
Member

Choose a reason for hiding this comment

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

The pendingTask being used here is the one created in the Before. Since the environment overrides will be on the pending task (see more detail in task builder comments), we'll need to construct our own pending task to test this here

@ssalinas ssalinas added this to the 0.19.0 milestone Nov 15, 2017
pendingTaskId, cmdLineArgsList, user, runId, skipHealthchecks, message, resources, envOverrides, actionId
);
}

Copy link
Member

Choose a reason for hiding this comment

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

Good to add a toString on these. Ends up being helpful for debugging later if we ever have to log it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know :)

return new SingularityRunNowRequest(
message, skipHealthchecks, runId, commandLineArgs, resources, envOverrides, runAt);
}

Copy link
Member

Choose a reason for hiding this comment

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

same toString comment for this one

@JsonProperty("skipHealthchecks") Optional<Boolean> skipHealthchecks,
@JsonProperty("runId") Optional<String> runId,
@JsonProperty("commandLineArgs") Optional<List<String>> commandLineArgs,
public SingularityRunNowRequest(@JsonProperty("setMessage") Optional<String> message,
Copy link
Member

Choose a reason for hiding this comment

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

the @JsonProperty on these needs to stay as it was before. This is one of the pieces jackson uses to determine the field name in the json. e.g. @JsonProperty("message") becomes {"message": {}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm...think that was my improper use of Intellij's refactoring tools. Going to need to be more careful of that in the future.

@@ -62,11 +68,16 @@ public SingularityRunNowRequest(@JsonProperty("message") Optional<String> messag
return skipHealthchecks;
}

@ApiModelProperty(required=false, value="Override the resources from the active deploy for this run")
@ApiModelProperty(required=false, value="Override the setResources from the active deploy for this run")
Copy link
Member

Choose a reason for hiding this comment

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

similar comment here, don't want to rename these to set...

this.commandLineArgs = Optional.absent();
this.skipHealthchecks = Optional.absent();
this.resources = Optional.absent();
this.envOverrides = null;
Copy link
Member

Choose a reason for hiding this comment

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

empty map instead of null here?

pendingRequest.getSkipHealthchecks(),
pendingRequest.getMessage(),
pendingRequest.getResources(),
null,
Copy link
Member

Choose a reason for hiding this comment

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

this should be the last missing piece of the puzzle to connect it all the way through. The SingularityPendingRequest is the object that will carry the envOverrides from the RunNowRequest to the pending task.

In RequestResource.scheduleImmediately we create a SingularityPendingRequest from the provided SingularityRunNowRequest. That pending request will need to take in the environment overrides, so that we can later use them on this line to put them on the pending task.

The RequestResource calls checkRunNowRequest on the SingularityValidator, which is what actually constructs the object actually running some checks. That is where the envOverrides will need to be put on the SingularityPendingRequest

@ssalinas
Copy link
Member

From what I can see this looks good. Thanks for those builder simplifications as well 🎉

I think we should be good to give this one a go in hs_staging if it was working for you locally

new SingularityPendingTaskId(
request.getId(), deployId, nextRunAt.get(), nextInstanceNumber,
pendingRequest.getPendingType(), pendingRequest.getTimestamp()),
pendingRequest.getCmdLineArgsList(),
Copy link
Member

Choose a reason for hiding this comment

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

indent ended up a bit funny here

private final Optional<Long> runAt;

public SingularityRunNowRequest(
Optional<String> message,
Optional<Boolean> skipHealthchecks,
Optional<String> runId,
Optional<List<String>> commandLineArgs,
Optional<Resources> resources
Optional<Resources> resources,
Map<String, String> envOverrides
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick note here, we don't want to modify existing constructor signatures for classes in a Base module of a project (or more generally, for any classes that are intended for use outside the project itself), because it'd break other projects. Adding "proxy" constructors is one way around this.

}
}
if (pendingType != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be handling the case where pendingType == null here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. This is just a small refactor of the previous code, which would do nothing in the case where the new code has pendingType == null.

@ssalinas ssalinas changed the title Environment variable overrides for run-now requests Overrides for run now requests Dec 1, 2017
@ssalinas
Copy link
Member

ssalinas commented Dec 5, 2017

🚢

@ssalinas
Copy link
Member

ssalinas commented Dec 6, 2017

🚢

@pschoenfelder
Copy link
Contributor Author

🚢

@ssalinas ssalinas merged commit 54616e5 into master Dec 7, 2017
@ssalinas ssalinas deleted the run_now_overrides branch December 7, 2017 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants