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

Incremental deploys #817

Merged
merged 46 commits into from Feb 23, 2016
Merged

Incremental deploys #817

merged 46 commits into from Feb 23, 2016

Conversation

@ssalinas
Copy link
Member

ssalinas commented Dec 21, 2015

Still needs plenty of work, but opening up early for feedback.

Right now I am adding a field to the SingularityPendingDeploy object that will track the progress of an incremental deploy as it goes along. This new field is an object that holds information about the current target number of instances and rate at which the deploy should proceed.

Still to-do:

  • Better scheduling of upcoming tasks. Right now it's just keying off of numMissingInstances, but since we know the time between deploy steps, we can go ahead and schedule them
  • Make it play nice with load balancer add/remove. Regular deploys add everything all at once when the deploy succeeds, this will need to do it per-task
  • Surface more information about what is happening in the deploy, if you aren't watching the logs it's kind of mysterious right now
  • Option to manually move the deploy forward or backward a step (should be fairly straightforward since we have the info in the SingularityPendingDeploy object)
  • Tests, tests, tests, tests, tests....
  • UI

/cc @tpetr @wsorenson

@@ -0,0 +1,5 @@
package com.hubspot.singularity;

public enum DeployStyle {

This comment has been minimized.

Copy link
@tpetr

tpetr Dec 21, 2015

Member

I don't want to overcomplicate things, but I wonder about the merits of an enum vs. being able to define a batch size for the deploy that defaults to the total instance count (i.e. a "normal" deploy)

This comment has been minimized.

Copy link
@ssalinas

ssalinas Dec 21, 2015

Author Member

Yeah, that's fair, might keep it simpler in the end, move towards all deploys working the same, but 'regular' deploys are just everything at once

@ssalinas ssalinas force-pushed the incremental_deploys branch from 478b99a to d5d15c4 Dec 22, 2015
@tpetr tpetr added this to the 0.4.7 milestone Dec 22, 2015
@ssalinas ssalinas force-pushed the incremental_deploys branch 4 times, most recently from 50f69f9 to d5dc040 Dec 24, 2015
@tpetr tpetr modified the milestones: 0.4.7, 0.4.8 Dec 28, 2015
@ssalinas ssalinas force-pushed the incremental_deploys branch from c29aecb to 81d0e93 Dec 29, 2015
@ssalinas ssalinas force-pushed the incremental_deploys branch from 81d0e93 to 036092e Dec 29, 2015
@ssalinas
Copy link
Member Author

ssalinas commented Dec 29, 2015

The basics are working now. All deploys now use a similar system where progress is tracked in the pending deploy object, incrementing a target number of instances for each step of the deploy. By default the number of instances per deploy step is the total instances (i.e. everything at once like before) unless a deployRate is set that then determines how many instances to deploy in each 'step'. Load balancer actions are triggered at the end of each step.

Some info is surfaced in the ui now on the request page, but the wording/design could probably still be a bit cleaner.

@@ -169,7 +177,10 @@ public SingularityDeployBuilder toBuilder() {
.setEnv(copyOfMap(env))
.setUris(copyOfList(uris))
.setExecutorData(executorData)
.setLabels(labels);
.setLabels(labels)

This comment has been minimized.

Copy link
@tpetr

tpetr Dec 29, 2015

Member

^ can you remove the newline? can get confusing in a builder block

@@ -134,6 +139,9 @@ public SingularityDeploy(@JsonProperty("requestId") String requestId,
this.serviceBasePath = serviceBasePath;
this.loadBalancerGroups = loadBalancerGroups;
this.loadBalancerOptions = loadBalancerOptions;

this.deployRate = deployRate;

This comment has been minimized.

Copy link
@tpetr

tpetr Dec 29, 2015

Member

kind of a nitpick but deployRate seems a little confusing to me. maybe deployInstanceCountPerStep? (curious what @wsorenson thinks before you make the change though)

@ssalinas ssalinas force-pushed the incremental_deploys branch from 95c22e8 to e2cb224 Dec 29, 2015
@@ -50,6 +51,7 @@
@Api(description="Manages Singularity Deploys for existing requests", value=DeployResource.PATH, position=2)
public class DeployResource extends AbstractRequestResource {
public static final String PATH = SingularityService.API_BASE_PATH + "/deploys";
public static final int DEFAULT_DEPLOY_STEP_WAIT_TIME_SECONDS = 60;

This comment has been minimized.

Copy link
@ssalinas

ssalinas Dec 29, 2015

Author Member

@tpetr was debating making the default wait time 0, would that make more sense? i.e. move on to the next step as soon as the previous is finished 'rolling deploy', by default

This comment has been minimized.

Copy link
@tpetr

tpetr Dec 29, 2015

Member

yeah i think that jives with the incremental deploy idea

@ssalinas ssalinas mentioned this pull request Feb 9, 2016
@ssalinas
Copy link
Member Author

ssalinas commented Feb 9, 2016

@tpetr @wsorenson finished a bunch of refactoring on this to slim down the methods. Also added more tests for canceled/failed deploy scenarios. Let me know if you see anything else in here that's needs updating

return targetActiveInstances;
}

@Override public String toString() {

This comment has been minimized.

Copy link
@tpetr

tpetr Feb 11, 2016

Member

this is a recent intellij thing that started happening, might not be a setting that the eclipse formatter file sets. i think it'd be good to keep all our @Override's on their own line

deployResult = new SingularityDeployResult(DeployState.FAILED_INTERNAL_STATE, Optional.of(String.format("Deploy had state %s but failed to persist it correctly", deployResult.getDeployState())), deployResult.getLbUpdate(), deployResult.getTimestamp());
deployResult =
new SingularityDeployResult(DeployState.FAILED_INTERNAL_STATE, Optional.of(String.format("Deploy had state %s but failed to persist it correctly", deployResult.getDeployState())),
deployResult.getLbUpdate(), deployResult.getTimestamp());

This comment has been minimized.

Copy link
@tpetr

tpetr Feb 11, 2016

Member

this is obviously a nitpick / not super important, but is this really the kind of formatting we want? this his happening due to a max column width setting?

This comment has been minimized.

Copy link
@ssalinas

ssalinas Feb 11, 2016

Author Member

if we don't care as much about the line length pieces, we should probably update the eclipse formatter settings that are in the repo.

@ssalinas ssalinas added the hs_stable label Feb 16, 2016
@tpetr tpetr modified the milestones: 0.4.10, 0.4.11 Feb 18, 2016
@ssalinas ssalinas modified the milestones: 0.4.11, 0.4.12 Feb 23, 2016
ssalinas added a commit that referenced this pull request Feb 23, 2016
Incremental deploys
@ssalinas ssalinas merged commit 0adc4dc into master Feb 23, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ssalinas ssalinas deleted the incremental_deploys branch Feb 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.