ability to update request data upon successful deploy #971

Merged
merged 13 commits into from Jun 9, 2016

Conversation

Projects
None yet
2 participants
@ssalinas
Member

ssalinas commented Mar 24, 2016

Currently, if you want to change the scale of a service (or any other request-level setting) as well as deploy new code, you need to do this in two separate steps. This PR adds an optional newRequestData field to the SingularityDeployRequest. If present, it will be added to the pending deploy so that the scheduler and deploy checker will use this new request data for the new deploy. If the deploy succeeds the request is then updated. No other updates to request data are allowed if there is currently a pending deploy that also updates request data.

@tpetr @wsorenson

@@ -10,14 +10,16 @@
private final Optional<SingularityLoadBalancerUpdate> lastLoadBalancerUpdate;
private final DeployState currentDeployState;
private final Optional<SingularityDeployProgress> deployProgress;
+ private final Optional<SingularityRequest> newRequestData;

This comment has been minimized.

@tpetr

tpetr Mar 24, 2016

Member

naming seems a little awkward in my opinion -- updatedRequest might make more sense

@tpetr

tpetr Mar 24, 2016

Member

naming seems a little awkward in my opinion -- updatedRequest might make more sense

This comment has been minimized.

@ssalinas

ssalinas Mar 24, 2016

Member

yeah, went back and forth on the name like 10 times already. Open to changing for sure, updatedRequest sounds good

@ssalinas

ssalinas Mar 24, 2016

Member

yeah, went back and forth on the name like 10 times already. Open to changing for sure, updatedRequest sounds good

@JsonCreator
public SingularityDeployRequest(
@JsonProperty("deploy") SingularityDeploy deploy,
@JsonProperty("unpauseOnSuccessfulDeploy") Optional<Boolean> unpauseOnSuccessfulDeploy,
- @JsonProperty("message") Optional<String> message) {
+ @JsonProperty("message") Optional<String> message,
+ @JsonProperty("newRequestData") Optional<SingularityRequest> newRequestData) {

This comment has been minimized.

@tpetr

tpetr Mar 24, 2016

Member

is there a reason this should be here, as opposed to on the SingularityDeploy object?

@tpetr

tpetr Mar 24, 2016

Member

is there a reason this should be here, as opposed to on the SingularityDeploy object?

This comment has been minimized.

@ssalinas

ssalinas Mar 24, 2016

Member

I had it in the deploy initially. But, the usage of this new field would only be at deploy time. After the deploy fails or succeeds we are back to using the normal request data. Having request data that stuck around with the deploy seemed like it could be more confusing. Made more sense to put it outside the deploy so it didn't stick around

@ssalinas

ssalinas Mar 24, 2016

Member

I had it in the deploy initially. But, the usage of this new field would only be at deploy time. After the deploy fails or succeeds we are back to using the normal request data. Having request data that stuck around with the deploy seemed like it could be more confusing. Made more sense to put it outside the deploy so it didn't stick around

authorizationHelper.checkForAuthorization(requestWithState.getRequest(), user, SingularityAuthorizationScope.WRITE);
+ SingularityRequest newRequestData = requestWithState.getRequest();
+ boolean hasUpdatedRequest = false;

This comment has been minimized.

@tpetr

tpetr Mar 24, 2016

Member

i think you can short-circuit this to final boolean hasUpdatedRequest = deployRequest.getNewRequestData().isPresent(), since an invalid updated request will cause an exception

@tpetr

tpetr Mar 24, 2016

Member

i think you can short-circuit this to final boolean hasUpdatedRequest = deployRequest.getNewRequestData().isPresent(), since an invalid updated request will cause an exception

This comment has been minimized.

@ssalinas

ssalinas Mar 24, 2016

Member

good point, I'll update

@ssalinas

ssalinas Mar 24, 2016

Member

good point, I'll update

authorizationHelper.checkForAuthorization(requestWithState.getRequest(), user, SingularityAuthorizationScope.WRITE);
+ SingularityRequest newRequestData = requestWithState.getRequest();

This comment has been minimized.

@tpetr

tpetr Mar 24, 2016

Member

naming seems awkward here since it's not always a new request

@tpetr

tpetr Mar 24, 2016

Member

naming seems awkward here since it's not always a new request

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Mar 24, 2016

Member

@tpetr updated the field names and such here. Took another look and I didn't see any more places where we would get conflicts between the two sources of request data.

  • deploy checker -> uses the new request data if it is set when checking the deploy
  • scheduler -> uses the pending deploy request data if it is set and if the pending request has a matching deploy id
  • cleaner -> relevant cases only deal with the active deploy, so it always should use the current request data not pending deploy one

Would like to get @wsorenson 's thoughts as well to make sure I'm not missing anything.

Member

ssalinas commented Mar 24, 2016

@tpetr updated the field names and such here. Took another look and I didn't see any more places where we would get conflicts between the two sources of request data.

  • deploy checker -> uses the new request data if it is set when checking the deploy
  • scheduler -> uses the pending deploy request data if it is set and if the pending request has a matching deploy id
  • cleaner -> relevant cases only deal with the active deploy, so it always should use the current request data not pending deploy one

Would like to get @wsorenson 's thoughts as well to make sure I'm not missing anything.

@ssalinas ssalinas modified the milestone: 0.4.12 Apr 1, 2016

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Apr 2, 2016

Member

What are your thoughts on moving the updatedRequest field from SingularityDeployParent to SingularityDeploy? I'm trying to debug a possible issue with this PR and it's a pain to not be able to get the updated request data over the API.

Member

tpetr commented Apr 2, 2016

What are your thoughts on moving the updatedRequest field from SingularityDeployParent to SingularityDeploy? I'm trying to debug a possible issue with this PR and it's a pain to not be able to get the updated request data over the API.

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Apr 2, 2016

Member

My initial concern was that we then have possibly stale request data that hangs around and isn't correct. The deploy data is saved in history, but if the request is updated, we have out of date data hanging around. The deploy parent data is only around during the time the deploy is actually running as part of the pending deploy.

Member

ssalinas commented Apr 2, 2016

My initial concern was that we then have possibly stale request data that hangs around and isn't correct. The deploy data is saved in history, but if the request is updated, we have out of date data hanging around. The deploy parent data is only around during the time the deploy is actually running as part of the pending deploy.

@ssalinas ssalinas modified the milestones: 0.5.0, 0.6.0 Apr 5, 2016

@ssalinas ssalinas added the hs_stable label Apr 7, 2016

@ssalinas ssalinas modified the milestones: 0.6.0, 0.8.0 May 13, 2016

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Jun 9, 2016

Member

@tpetr after the update we made back in April this has been stable, good to merge?

Member

ssalinas commented Jun 9, 2016

@tpetr after the update we made back in April this has been stable, good to merge?

@tpetr

This comment has been minimized.

Show comment
Hide comment
@tpetr

tpetr Jun 9, 2016

Member

LGTM

Member

tpetr commented Jun 9, 2016

LGTM

@ssalinas ssalinas merged commit 510078a into master Jun 9, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the update_on_deploy branch Jun 9, 2016

@ssalinas ssalinas removed the hs_staging label Jun 9, 2016

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