Fleet update strategy: Replace #199
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
added
kind/feature
markmandel
force-pushed
the
feature/fleet-replace
branch
from
bc820a0
to
a3eec8b
Compare
|
Build Failed 😱 Build Id: 03705ece-f3ca-4033-8039-af427c17c88d Build Logs |
|
Build Failed 😱 Build Id: 97f15f10-ed60-492d-b963-057fe19878ef Build Logs |
markmandel
force-pushed
the
feature/fleet-replace
branch
2 times, most recently
from
e463233
to
980ad2d
Compare
|
Build Succeeded 👏 Build Id: 7964e4de-35f8-4011-a62c-aa15b13bddda The following development artifacts have been built, and will exist for the next 30 days:
|
EricFortin
reviewed
May 9, 2018
docs/create_fleet.md
Outdated
| We can also change the configuration of the `GameServer` of the running `Fleet`, and have the changes | ||
| roll out, without interrupting the currently `Allocated` `GameServers`. | ||
|
|
||
| Let's take this for a spin! Run `kubectl edit fleet simple-udp` and set the `repicas` field to back to `5`. |
There was a problem hiding this comment.
Typo: replicas field back to 5
EricFortin
reviewed
May 9, 2018
| @@ -173,39 +224,102 @@ func (c *Controller) syncFleet(key string) error { | |||
| return err | |||
| } | |||
|
|
|||
| var activeGsSet *stablev1alpha1.GameServerSet | |||
| activeGsSet, rest := c.filterGameServerSetByActive(fleet, list) | |||
There was a problem hiding this comment.
I want to make sure I understand this piece of code. This method returns the active gameserver set a a list of previous one. We then go on to set the replicas count to 0 on those and then deleting them. What happens if one of those replicas was allocated?
There was a problem hiding this comment.
Excellent question - any previously Allocated GameServers that part of the "inactive" GameServerSet are left alone, and the GameServerSet will stay alive until the Allocated GameServer shuts itself down or is otherwise terminated.
For reference you can see the code for scaling down GameServerSet here, and how it ignores Allocated GameServers.
For a live demo, you can actually see it in action on this twitch video (only live for 13 more days, will push to YouTube at some point today).
There was a problem hiding this comment.
As an interesting note - this is why deleteEmptyGameServerSets (code) only deletes a GameServerSet when the gsSet.Status.Replicas == 0 - because this will never be 0 when there are still Allocated GameServers.
markmandel
force-pushed
the
feature/fleet-replace
branch
2 times, most recently
from
0324093
to
833eb56
Compare
|
Build Succeeded 👏 Build Id: 6ab82750-e6a4-46d5-b2b3-ab6f9a1c4392 The following development artifacts have been built, and will exist for the next 30 days:
|
|
@enocom see anything that needs improvement in my Go code? 😄 |
markmandel
force-pushed
the
feature/fleet-replace
branch
from
833eb56
to
1533a4e
Compare
|
Build Succeeded 👏 Build Id: 5ba15b9c-2f8f-47a7-808f-6455efac4c7e The following development artifacts have been built, and will exist for the next 30 days:
|
enocom
reviewed
May 11, 2018
pkg/fleets/controller.go
Outdated
| if err != nil { | ||
| return errors.Wrapf(err, "error creating gameserverset for fleet %s", fleet.ObjectMeta.Name) | ||
| } | ||
|
|
||
| c.recorder.Eventf(fleet, corev1.EventTypeNormal, "CreatingGameServerSet", | ||
| "Created GameServerSet %s", activeGsSet.ObjectMeta.Name) | ||
| "Created GameServerSet %s", gsSet.ObjectMeta.Name) | ||
| } else if replicas != gsSet.Spec.Replicas { |
There was a problem hiding this comment.
Rather than use an else if statement here, you could add a return nil to the if block above, and a return nil to the else if block. That way the control flow is easier to follow and more idiomatic.
There was a problem hiding this comment.
Updated. That should be better. Definitely seems more readable 👍
When the template for a Fleet is edited, a deployment strategy can be defined. At the moment, this is just the `Replace` strategy, which terminates all current non-allocated GameServers and immeadiately starts up new GameServers to replace them. Includes documentation as well.
markmandel
force-pushed
the
feature/fleet-replace
branch
from
1533a4e
to
5fa785c
Compare
|
Build Succeeded 👏 Build Id: 7896b68f-b00f-47f0-9620-fe7a98f3c73f The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Succeeded 👏 Build Id: 64432286-bef7-4463-9b9c-8554ca97fb43 The following development artifacts have been built, and will exist for the next 30 days:
|
|
@enocom @EricFortin gentle bump 😄 Please let me know if there is anything else to change, otherwise, would love an approval 😊 Thanks! |
|
Build Succeeded 👏 Build Id: 1cdf188e-ff09-476e-bf7b-73cec6ad2193 The following development artifacts have been built, and will exist for the next 30 days:
|
|
Build Failed 😱 Build Id: 5eff556d-6af2-4565-a768-0e8845b7d20c Build Logs |
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.
When the template for a Fleet is edited, a deployment strategy can be defined. At the moment, this is just the
Replacestrategy, which terminates all current non-allocated GameServers and immediately starts up new GameServers to replace them.Includes documentation as well.