-
Notifications
You must be signed in to change notification settings - Fork 520
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
fix(app): starting with past units should not increase the process units #2614
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM! To ensure future modifications won't break this again, I would add an extra check on TestStart_StartProcessesUsingReplicasFromLastAppStop
for a subsequent err = a.Start(context.TODO(), &output, "", "")
should return empty output
Sure... lemme improve this test case. |
provision/provision.go
Outdated
@@ -451,6 +451,12 @@ type InterAppProvisioner interface { | |||
InternalAddresses(ctx context.Context, a App) ([]AppInternalAddress, error) | |||
} | |||
|
|||
// CurrentReplicasGetter implements how to get the current (desired) | |||
// number of replicas (units) of an app. | |||
type CurrentReplicasGetter interface { |
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.
@nettoclaudio could you rename to CurrentReplicasProvisioner to be like other interfaces ?
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.
Sure
app/app.go
Outdated
|
||
fmt.Fprintf(w, "Before being stopped, the %q process had %d replicas... re-adding them.\n", p, replicas) | ||
|
||
err := prov.AddUnits(ctx, app, uint(replicas), p, version, w) |
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.
@nettoclaudio to perfectly achieve the number of replicas without risky of race-condition, we should implement the SetUnits method instead of AddUnits, but it is a nice to have.
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.
Although the name, the implementation on provisioner side is set-like already.
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.
Wait... it isn't. So you found a bug. 🎉
lgtm 👍🏻 |
Please don't merge this PR yet. I found a problem when user doesn't pass a specific version - in this case, the PR doesn't do anything at all. |
Prior to this PR, running consecutive app-stop and app-start could increase the number of replicas since we didn't check the status of the pods. This implementation delegates to the provisioner a new method to get the desired replicas number.