-
Notifications
You must be signed in to change notification settings - Fork 188
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
Report currentActiveInstances on SingularityDeployProgress #1377
Conversation
5a60114
to
cca3d4f
Compare
cca3d4f
to
006ea40
Compare
"Pending deploy already in progress for %s - cancel it or wait for it to complete (%s)", requestId, deployManager.getPendingDeploy(requestId).orNull()); | ||
boolean deployExists = deployManager.createPendingDeploy(pendingDeployObj) == SingularityCreateResult.EXISTED; | ||
if (deployExists && forceDeploy.or(false)) { | ||
cancelDeploy(requestId, deployManager.getInUseDeployId(requestId).orNull()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if there is a restart method or if I need to do something after hitting the cancel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we implement force deploy in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah agreed on leaving force deploy for a separate PR, we'd need to cancel + make sure the deploy checker successfully cancelled before starting a new one for things to work properly.
} | ||
|
||
public SingularityDeployProgress withCompletedStep() { | ||
return new SingularityDeployProgress(targetActiveInstances, deployInstanceCountPerStep, deployStepWaitTimeMs, true, autoAdvanceDeploySteps, failedDeployTasks, System.currentTimeMillis()); | ||
return new SingularityDeployProgress(targetActiveInstances, currentActiveInstances + 1, deployInstanceCountPerStep, deployStepWaitTimeMs, true, autoAdvanceDeploySteps, failedDeployTasks, System.currentTimeMillis()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is currentActiveInstances + 1
correct here? wouldn't it be currentActiveInstances + deployInstanceCountPerStep
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was originally intended to just be the current deploy progress with stepCompleted
flipped to true. If we want to update the active instance count I'd suggest getting it from the deployActiveTasks
variable that gets passed around instead. If we pass that to the calls of markStepFinished
, we can construct the new deploy progress with the actual count of active tasks after each step. withCompletedStep
should do just what I mentioned above and flip the value of that bool, keeping the rest the same
"Pending deploy already in progress for %s - cancel it or wait for it to complete (%s)", requestId, deployManager.getPendingDeploy(requestId).orNull()); | ||
boolean deployExists = deployManager.createPendingDeploy(pendingDeployObj) == SingularityCreateResult.EXISTED; | ||
if (deployExists && forceDeploy.or(false)) { | ||
cancelDeploy(requestId, deployManager.getInUseDeployId(requestId).orNull()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we implement force deploy in a separate PR?
67ec24b
to
006ea40
Compare
4ec1ed6
to
b4282bf
Compare
@tpetr any changes necessary on this one? |
Looks good but could you add unit tests for this? Or if there are already tests flexing this functionality, update them to ensure that |
Just added a test following the logic of existing tests, but I can't say I'm super confident that it's the best way to test the counter. e.g. I couldn't figure out how to deploy with a target of 4 instances, and check the |
@darcatron apologies for letting this one sit so long. I think having that current active instances value will still be valuable. In terms of the test, I would check out the example of
You could likely extend that test to do three instances instead of just two and add a few more assert statements in there to adequately test the new counter |
@ssalinas gave the test another look based on your suggestions and it seems that was the original test I followed. Instead of adding a check for these changes by extending an existing test, this one is separate. I think that's probably better anyway so unless you want to combine them, I'm going to keep it as is. Cool if I get this into staging? |
I think that's fair, we can try this out in staging |
This reports that a canary deploy has completed the first step