Prioritise Allocation from Nodes with Allocated/Ready GameServers #370
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
|
Build Failed 😱 Build Id: 168dd0f8-60c3-4c56-b237-b9f8160eb2c4 Build Logs |
markmandel
force-pushed
the
feature/allocate-priority
branch
from
cc07f07 to
9f31570
Compare
|
Build Succeeded 👏 Build Id: 611cccb7-fca1-4904-a320-09d9544103c6 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
IT should work, but I have some performance concerns. |
pkg/fleetallocation/count.go
Outdated
| count.ready++ | ||
| } | ||
|
|
||
| result[gs.Status.NodeName] = count |
There was a problem hiding this comment.
Update only If changed
pkg/fleetallocation/controller.go
Outdated
| nodeCounts := countReadyAndAllocatedPerNode(gsList) | ||
|
|
||
| // prioritise by nodes that already have allocated gameservers, and then by ready | ||
| sort.Slice(gsList, func(i, j int) bool { |
There was a problem hiding this comment.
We are allocating just one server here so we dont need to actually sort the list (nlogn), just to find the best one (O(n)).
So we should combine this new logic with the iteration below.
There was a problem hiding this comment.
Excellent catch. Working on it now.
pkg/fleetallocation/count.go
Outdated
| import "agones.dev/agones/pkg/apis/stable/v1alpha1" | ||
|
|
||
| type nodeCount struct { | ||
| ready int64 |
There was a problem hiding this comment.
Why int64 and not just int?
There was a problem hiding this comment.
Habit? 😄 probably not necessary here.
|
Build Failed 😱 Build Id: be7f9477-4a30-4649-9ae3-1be86c26c908 Build Logs |
markmandel
force-pushed
the
feature/allocate-priority
branch
from
d9e5121 to
b3b93b7
Compare
|
Build Failed 😱 Build Id: 4213a5ef-4142-4361-b460-297245a14ef0 Build Logs |
markmandel
force-pushed
the
feature/allocate-priority
branch
from
b3b93b7 to
dda935f
Compare
|
Build Failed 😱 Build Id: 1a9361e0-c9f0-4395-a2e6-95538cdecbda Build Logs |
markmandel
force-pushed
the
feature/allocate-priority
branch
from
dda935f to
fe798ba
Compare
|
Build Succeeded 👏 Build Id: 349c2e46-3a25-4d6d-9084-259affce73b3 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
| } | ||
|
|
||
| if update { | ||
| bestCount = count |
There was a problem hiding this comment.
bestCount should be udated together with bestGs or else you're building an allocation trap here me thinks
There was a problem hiding this comment.
I don't think that it will - but, it definitely makes the code easier to understand with it together - so change incoming 👍
markmandel
force-pushed
the
feature/allocate-priority
branch
from
fe798ba to
ffef7fe
Compare
|
Build Succeeded 👏 Build Id: 08e55014-04a5-4655-b332-2b19c11d60c0 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
pkg/fleetallocation/find_test.go
Outdated
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestFindReadyGameServer(t *testing.T) { |
There was a problem hiding this comment.
Add this test case: node1 with 4 allocated (first in list), then node 2 with 4 ready.
Node1 would be the trap I mentioned earlier.
Thing is that allocated is the primary sort key, but ready>0 is the veto condition. Actually you could also include this in the 'best' search above.
Its quite late here so I am not sure, am I making sense? 😴
There was a problem hiding this comment.
I added the extra test - seems to be passing -- maybe I got it wrong? 🤷♂️ 😄
There was a problem hiding this comment.
That's right, it was working before because you were setting update=true if bestGs==nil as the first condition. If you were using bestCount==nil, it wouldn't have worked, and for someone who reads the code the reason is not easily seen as they should be an atomic pair.
The rules (which you explained on Slack and in the design ticket) were very clear, but are not transposed into the code clearly imo. For example, the update logic is split into two places: first you set an update bool that is actually half of the truth and then you apply another condition. I will add a few notes inline for some specific things I would do differently.
|
Together with this change I think you should also include an allocation strategy param in the allocation spec: stacked or flat. In my case, runnning a static cluster, i want the load to be distributed evenly on the machines. This change as is might make things go 💥 for me. WDYT? |
|
Ooh interesting - should this be on the allocation, or on the Fleet? I kinda feel like it should be at the Fleet level, because it likely also impacts downscaling/bin packing scheduling/etc as well. Previously, it was essentially a random distribution of allocated game servers (because listing tends to be random) - so that was not an official round-robin -- would you want actual round robin, or is random okay (as it was)? Ditto for scale down, it was essentially random. This is why I think it should be at the fleet level -- the fleet "optimisation" (is that a good name?) would determine how allocation, scaling down and what scheduler the backing Pods would get
Don't know if it's the right names but does that make sense? |
|
Build Failed 😱 Build Id: b44c2332-d2e1-4ae6-ad69-e364cbd77cef Build Logs |
pkg/fleetallocation/find_test.go
Outdated
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestFindReadyGameServer(t *testing.T) { |
There was a problem hiding this comment.
That's right, it was working before because you were setting update=true if bestGs==nil as the first condition. If you were using bestCount==nil, it wouldn't have worked, and for someone who reads the code the reason is not easily seen as they should be an atomic pair.
The rules (which you explained on Slack and in the design ticket) were very clear, but are not transposed into the code clearly imo. For example, the update logic is split into two places: first you set an update bool that is actually half of the truth and then you apply another condition. I will add a few notes inline for some specific things I would do differently.
pkg/fleetallocation/find.go
Outdated
| func findReadyGameServerForAllocation(gsList []*v1alpha1.GameServer) *v1alpha1.GameServer { | ||
| counts := map[string]*nodeCount{} | ||
| // track potential gameservers, one for each node | ||
| gsOptions := map[string]*v1alpha1.GameServer{} |
There was a problem hiding this comment.
I don't understand the name gsOptions. I would rename it to gsInstance, gsAllocatableInstance or gsReadyInstance, because that's what it will contain.
pkg/fleetallocation/find.go
Outdated
| for nodeName, count := range counts { | ||
| update := false | ||
| // if there is no best GameServer, then this node & GameServer is the always the best | ||
| if bestGS == nil { |
There was a problem hiding this comment.
I would change this to bestGS == nil && count.ready > 0, because this is the valid condition to update bestGS imo
pkg/fleetallocation/find.go
Outdated
| update = true | ||
| } else if count.allocated == bestCount.allocated && count.ready > bestCount.ready { | ||
| update = true | ||
| } else if count.allocated > bestCount.allocated { |
There was a problem hiding this comment.
I could change this condition to count.allocated > bestCount.allocated && count.ready > 0 to be more correct.
pkg/fleetallocation/find.go
Outdated
| } | ||
|
|
||
| if update { | ||
| // we may not have any GameServers on this node, so check first! |
There was a problem hiding this comment.
We shouldn't try to update if we know that the current node has no ready nodes. Therefore, I would assert.NotNil(gsOptions[nodeName]).
There was a problem hiding this comment.
Go doesn't really have a runtime assert, all I can do is check if it exists, and return an error if it doesn't (or nil, which is caught later)
There was a problem hiding this comment.
I reworked this a bit, to try and make it more readable -- agreed with lots of what you wrote, tweaked a couple of things. Would love you to take another pass 👍 Thanks for taking all the time to do such a detailed review.
Yes, indeed. Fleet is a better place.
Random is fine, I don't see any advantage in a real round-robin. What I would add would be a flat/evenly distributed allocation, in which you allocate a server on a machine that has the least amount of ready servers (to spread the load evenly).
Yes, it makes sense. I don't agree with the We would have the following operation/allocation strategies:
And maybe it's not a subject for current PR, but I think we should also consider how to handle different node pools/different machine types. Both for WDYT? |
|
I'd like to keep it to at most 2 scheduling strategies (maybe we should call it I think our target is (1) dynamic systems, i.e. cloud providers -- which would be The other fun question - do you think this configuration option should be part of this PR? I'm leaning yes, but could be swayed the other way. Re: fractions -- totally agreed, but I think let's not worry about this for this PR, we can come back to it. |
|
Build Succeeded 👏 Build Id: 1066b557-2247-479e-911c-86a4d53a6e14 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
I am thinking though, that unless we have both strategies in place, we shouldn't push this PR into the next release -- it's a pretty big change, and without an option to backout - I think that would be bad. I'm going to try and get the |
👍
Yes, for the same reason you mentioned yourself- it's a pretty big change, and it could cause surprising side effects. Some kind of off switch is needed in this case.
👍 |
markmandel
changed the title
markmandel
force-pushed
the
feature/allocate-priority
branch
from
0fe47c4 to
c7d524d
Compare
|
Build Succeeded 👏 Build Id: dd8618c5-a383-4d63-bfd8-196d5518aeb1 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
This is ready for review again! We now have two |
victor-prodan
approved these changes
Oct 9, 2018
pkg/apis/stable/v1alpha1/fleet.go
Outdated
| // to bin pack as many Allocated GameServers on a single node. | ||
| // This is most useful for dynamic Kubernetes clusters - such as on Cloud Providers. | ||
| // In future versions, this will also impact Fleet scale down, and Pod Scheduling. | ||
| // TODO: example and document |
| } | ||
| fixtures := []v1alpha1.SchedulingStrategy{v1alpha1.Packed, v1alpha1.Distributed} | ||
|
|
||
| for _, strategy := range fixtures { |
markmandel
force-pushed
the
feature/allocate-priority
branch
from
a63b132 to
713b592
Compare
|
Build Succeeded 👏 Build Id: b91646cd-50af-497c-8b57-3668fa4964c4 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
added
the
feature-freeze-do-not-merge
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
Oct 10, 2018
Moved this out of googleforgames#370 so people can review it during the RC cycle.
markmandel
added a commit
to markmandel/agones
that referenced
this pull request
Oct 10, 2018
Moved this out of googleforgames#370 so people can review it during the RC cycle.
markmandel
added a commit
that referenced
this pull request
Oct 10, 2018
Moved this out of #370 so people can review it during the RC cycle.
markmandel
force-pushed
the
feature/allocate-priority
branch
from
713b592 to
4cbaf3e
Compare
|
Build Succeeded 👏 Build Id: 605f3c17-58af-4778-ad75-8b2b85eefd86 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
force-pushed
the
feature/allocate-priority
branch
from
4cbaf3e to
487388c
Compare
One of the first parts for Node autoscaling (googleforgames#368) - make sure we essentially bin pack our allocated game servers. This change makes allocation first prioritise allocation from `Nodes` that already have the most `Allocated` `GameServers`, and then in the case of a tie, to the `Nodes` that have the most `Ready` `GameServers`. This sets us up for the next part, such that when we scale down a Fleet, it removes `GameServers` from `Nodes` that have the least `GameServers` on them.
markmandel
force-pushed
the
feature/allocate-priority
branch
from
487388c to
8fc3114
Compare
|
Build Succeeded 👏 Build Id: 65403add-4f1c-4212-8fcd-95674bea056e The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
|
Build Succeeded 👏 Build Id: 28d02b68-da65-449f-97c0-09b1543c4513 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
force-pushed
the
feature/allocate-priority
branch
from
fbd01c6 to
8fc3114
Compare
|
Build Succeeded 👏 Build Id: b289ed8f-8044-453e-9a60-31da380fd579 The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
markmandel
removed
feature-freeze-do-not-merge
|
Build Succeeded 👏 Build Id: 9280fcf5-9933-46c1-bebe-5c46f328a63e The following development artifacts have been built, and will exist for the next 30 days:
(experimental) To install this version:
|
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.
One of the first parts for Node autoscaling (#368) - make sure we essentially bin pack our allocated game servers.
This change makes allocation first prioritise allocation from
Nodesthat already have the mostAllocatedGameServers, and then in the case of a tie, to theNodesthat have the mostReadyGameServers.This sets us up for the next part, such that when we scale down a Fleet, it removes
GameServersfromNodesthat have the leastGameServerson them.