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

allow deploy to specify port index for healthchecks and LB api #887

Merged
merged 3 commits into from Feb 10, 2016

Conversation

Projects
None yet
2 participants
@ssalinas
Member

ssalinas commented Feb 9, 2016

On the deploy you can specify loadBalancerPortIndex or healthcheckPortIndex (e.g. 0 for first allocated port, 1 for second allocated port). Defaults to 0 if not specified

/fixes #886

@ssalinas ssalinas added the hs_staging label Feb 9, 2016

Show outdated Hide outdated ...ularityBase/src/main/java/com/hubspot/singularity/SingularityDeploy.java
@@ -281,6 +289,11 @@ public String getId() {
return healthcheckTimeoutSeconds;
}
@ApiModelProperty(required=false, value="Perform healthcheck on this dynamically allocated port (e.g. 0 for first port)")

This comment has been minimized.

@tpetr

tpetr Feb 9, 2016

Member

should probably indicate that it defaults to the first port

@tpetr

tpetr Feb 9, 2016

Member

should probably indicate that it defaults to the first port

Show outdated Hide outdated ...ain/java/com/hubspot/singularity/scheduler/SingularityHealthchecker.java
@@ -167,9 +169,9 @@ public void reEnqueueOrAbort(SingularityTask task) {
final String hostname = task.getOffer().getHostname();
Optional<Long> firstPort = task.getFirstPort();
Optional<Long> healthecheckPort = task.getPortByIndex(task.getTaskRequest().getDeploy().getHealthcheckPortIndex().or(0));

This comment has been minimized.

@tpetr

tpetr Feb 9, 2016

Member

typo

@tpetr

tpetr Feb 9, 2016

Member

typo

Show outdated Hide outdated SingularityBase/src/main/java/com/hubspot/singularity/SingularityTask.java
}
public Optional<Long> getPortByIndex(int index) {
List<Long> ports = MesosUtils.getAllPorts(mesosTask.getResourcesList());
if (index >= ports.size()) {

This comment has been minimized.

@tpetr

tpetr Feb 9, 2016

Member

should check for index < 0 too

@tpetr

tpetr Feb 9, 2016

Member

should check for index < 0 too

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Feb 9, 2016

Member

Can you also add checks for these new fields to SingularityValidator.checkDeploy()?

Member

tpetr commented Feb 9, 2016

Can you also add checks for these new fields to SingularityValidator.checkDeploy()?

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Feb 9, 2016

Member

Worth adding and/or updating tests to verify this works

Member

tpetr commented Feb 9, 2016

Worth adding and/or updating tests to verify this works

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Feb 9, 2016

Member

(LGTM once all this is addressed)

Member

tpetr commented Feb 9, 2016

(LGTM once all this is addressed)

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Feb 9, 2016

Member

Added validator pieces, just need to add tests

Member

ssalinas commented Feb 9, 2016

Added validator pieces, just need to add tests

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Feb 9, 2016

Member

LGTM, thanks

Member

tpetr commented Feb 9, 2016

LGTM, thanks

tpetr added a commit that referenced this pull request Feb 10, 2016

Merge pull request #887 from HubSpot/port_indices
allow deploy to specify port index for healthchecks and LB api

@tpetr tpetr merged commit 4468c7d into master Feb 10, 2016

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 added this to the 0.4.10 milestone Mar 2, 2016

@ssalinas ssalinas deleted the port_indices branch Apr 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment