Rolling updates for Fleets #213
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
markmandel
commented
May 18, 2018
| // rollingUpdateDeployment will do the rolling update of the old GameServers | ||
| // through to the new ones, based on the fleet.Spec.Strategy.RollingUpdate configuration | ||
| // and return the replica count for the active GameServerSet | ||
| func (c *Controller) rollingUpdateDeployment(fleet *stablev1alpha1.Fleet, active *stablev1alpha1.GameServerSet, rest []*stablev1alpha1.GameServerSet) (int32, error) { |
There was a problem hiding this comment.
@enocom particularly love your eyes on this function, and the functions it calls - it's a bit complicated, so if you can see anything that could improve the code / readability, would definitely be interested.
markmandel
force-pushed
the
feature/fleet-rolling
branch
from
c87fd20
to
678d5ed
Compare
|
Build Succeeded 👏 Build Id: afe860aa-044d-48d9-91d6-26c692369fb1 The following development artifacts have been built, and will exist for the next 30 days:
|
enocom
reviewed
May 19, 2018
pkg/fleets/controller.go
Outdated
| func (c *Controller) rollingUpdateActive(fleet *stablev1alpha1.Fleet, active *stablev1alpha1.GameServerSet, rest []*stablev1alpha1.GameServerSet) (int32, error) { | ||
| replicas := active.Spec.Replicas | ||
|
|
||
| if active.Spec.Replicas < fleet.Spec.Replicas && active.Spec.Replicas == active.Status.Replicas { |
There was a problem hiding this comment.
I would invert this boolean expression such that the body becomes return replicas, nil as on line 381 below. That way the control flow of the function handles errors within if statements and happy paths outside. Does that make sense?
pkg/fleets/controller.go
Outdated
| unavailable := int32(r) | ||
|
|
||
| for _, gsSet := range rest { | ||
| if gsSet.Status.Replicas > 0 && gsSet.Status.Replicas == gsSet.Spec.Replicas { |
There was a problem hiding this comment.
Same thing here as above. If you invert the boolean check, your if statement can become much smaller with a continue in the body, and the code outside the if statement can be moved below, which makes understanding the flow easier.
markmandel
force-pushed
the
feature/fleet-rolling
branch
from
678d5ed
to
b2d8fd0
Compare
markmandel
commented
May 21, 2018
| func (c *Controller) rollingUpdateActive(fleet *stablev1alpha1.Fleet, active *stablev1alpha1.GameServerSet, rest []*stablev1alpha1.GameServerSet) (int32, error) { | ||
| replicas := active.Spec.Replicas | ||
|
|
||
| // if the active spec replicas are greater than or equal the fleet spec replicas, then we don't |
There was a problem hiding this comment.
@enocom made the inversion here, and also put in some text to help explain the logic. WDYT?
There was a problem hiding this comment.
Very nice. It takes a bit of getting used to, but the resulting code has a better flow with errors handled to the right.
| for _, gsSet := range rest { | ||
| // if the status.Replicas are less than or equal to 0, then that means we are done | ||
| // scaling this GameServerSet down, and can therefore exit/move to the next one. | ||
| if gsSet.Status.Replicas <= 0 { |
There was a problem hiding this comment.
@enocom the inversion here breaks this out into two sections, with slightly different logic - which I think is much easier to read 👍 and parse. WDYT?
There was a problem hiding this comment.
I think it reads better for sure. And the comments are very helpful.
|
Build Succeeded 👏 Build Id: 53a0801a-9358-4780-9643-ae87a11ceb48 The following development artifacts have been built, and will exist for the next 30 days:
|
EricFortin
reviewed
May 22, 2018
pkg/fleets/controller.go
Outdated
| if gsSet.Status.Replicas <= 0 { | ||
| continue | ||
| } | ||
| // If the Status.Replicas does not equal the Status.Replicas for this GameServerSet, this means |
There was a problem hiding this comment.
If the Spec.Replicas does not equal ...
This implements a configurable rolling update strategy for Fleets that also ensures that Allocated GameServers are not interuppted. Also includes updates to documentation. Closes googleforgames#70
markmandel
force-pushed
the
feature/fleet-rolling
branch
from
b2d8fd0
to
d409691
Compare
|
Build Failed 😱 Build Id: 37ec19e1-73e1-4aa5-b516-fe707fade650 Build Logs |
|
Build Succeeded 👏 Build Id: a9267b94-3a73-4d49-9642-169ba958de16 The following development artifacts have been built, and will exist for the next 30 days:
|
EricFortin
approved these changes
May 23, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements a configurable rolling update strategy for Fleets that also ensures that Allocated GameServers are not interrupted.
Also includes updates to documentation.
Closes #70