From 31b7aae9e49537e212c089dff3c487590b18a4d6 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Fri, 21 Apr 2023 21:09:45 +0000 Subject: [PATCH 1/7] Counts and Lists Actions on Game Server Allocation --- pkg/apis/agones/v1/common.go | 6 +- pkg/apis/agones/v1/gameserver.go | 80 ++++ pkg/apis/agones/v1/gameserver_test.go | 361 ++++++++++++++++++ .../allocation/v1/gameserverallocation.go | 47 +++ .../v1/gameserverallocation_test.go | 185 +++++++++ pkg/gameserverallocations/find.go | 14 + 6 files changed, 692 insertions(+), 1 deletion(-) diff --git a/pkg/apis/agones/v1/common.go b/pkg/apis/agones/v1/common.go index f1a5539dc4..bafe72e999 100644 --- a/pkg/apis/agones/v1/common.go +++ b/pkg/apis/agones/v1/common.go @@ -26,7 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// Block of const Error messages +// Block of const const ( ErrContainerRequired = "Container is required when using multiple containers in the pod template" ErrHostPort = "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy" @@ -34,6 +34,10 @@ const ( ErrContainerPortRequired = "ContainerPort must be defined for Dynamic and Static PortPolicies" ErrContainerPortPassthrough = "ContainerPort cannot be specified with Passthrough PortPolicy" ErrContainerNameInvalid = "Container must be empty or the name of a container in the pod template" + // GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be incremented at Allocation. + GameServerAllocationIncrement string = "Increment" + // GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation. + GameServerAllocationDecrement string = "Decrement" ) // AggregatedPlayerStatus stores total player tracking values diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index 392b250e3f..e5dbc8086e 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -926,3 +926,83 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) { result, err = json.Marshal(patch) return result, errors.Wrapf(err, "error creating json for patch for GameServer %s", gs.ObjectMeta.Name) } + +// UpdateCount increments or decrements a CounterStatus on a Game Server by the given amount. +// TODO: Should we log on noncomplemention or return false? Issue #2716 asks for "silent no-op" +func (gs *GameServer) UpdateCount(name string, action string, amount int64) { + if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) || amount < 0 { + return + } + if counter, ok := gs.Status.Counters[name]; ok { + cnt := counter.Count + if action == GameServerAllocationIncrement { + cnt += amount + } + if action == GameServerAllocationDecrement { + cnt -= amount + } + // TODO: Can we force the Capacity to update first before updating the Count if CounterAction updates both? + if (cnt < 0) || (cnt > counter.Capacity) { + return + } + counter.Count = cnt + gs.Status.Counters[name] = counter + } +} + +// UpdateCounterCapacity updates the CounterStatus Capacity to the given capacity. Silent no-op. +// TODO: Do anything if Capacity is updated to be less than the current Count? +func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) { + if capacity < 0 { + return + } + if counter, ok := gs.Status.Counters[name]; ok { + counter.Capacity = capacity + gs.Status.Counters[name] = counter + } +} + +// UpdateListCapacity updates the ListStatus Capacity to the given capacity. Silent no-op. +func (gs *GameServer) UpdateListCapacity(name string, capacity int64) { + if capacity < 0 || capacity > 1000 { + return + } + if list, ok := gs.Status.Lists[name]; ok { + list.Capacity = capacity + gs.Status.Lists[name] = list + } +} + +// AppendListValues adds given the values to the ListStatus Values list. Any duplicates are silently +// dropped. Silent no-op. +func (gs *GameServer) AppendListValues(name string, values []string) { + if len(values) == 0 { + return + } + if list, ok := gs.Status.Lists[name]; ok { + vals := []string{} + // Maintain ordering so new values are appended to the end of list.Values + vals = append(vals, list.Values...) + vals = append(vals, values...) + vals = removeDuplicates(vals) + if (len(vals) > int(list.Capacity)) || (len(vals) == len(list.Values)) { + return + } + // TODO: Do we need deepcopy here? + list.Values = vals + gs.Status.Lists[name] = list + } +} + +// removeDuplicates removes any duplicate values from a list. Returns new list with unique values only. +func removeDuplicates(values []string) []string { + listMap := make(map[string]bool) + uniqueValues := []string{} + for _, v := range values { + if _, ok := listMap[v]; !ok { + uniqueValues = append(uniqueValues, v) + listMap[v] = true + } + } + return uniqueValues +} diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index 0123df37dd..526e04216e 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -1635,3 +1635,364 @@ func defaultGameServer() *GameServer { }, }, Status: GameServerStatus{State: GameServerStateCreating}} } + +func TestGameServerUpdateCount(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + gs GameServer + name string + action string + amount int64 + want CounterStatus + }{ + "counter not in game server silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "foos": { + Count: 0, + Capacity: 100, + }}}}, + name: "foo", + action: "Increment", + amount: 1, + }, + "amount less than zero silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "foos": { + Count: 1, + Capacity: 100, + }}}}, + name: "foos", + action: "Decrement", + amount: -1, + want: CounterStatus{ + Count: 1, + Capacity: 100, + }, + }, + "increment by 1": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "players": { + Count: 0, + Capacity: 100, + }}}}, + name: "players", + action: "Increment", + amount: 1, + want: CounterStatus{ + Count: 1, + Capacity: 100, + }, + }, + "decrement by 10": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "bars": { + Count: 99, + Capacity: 100, + }}}}, + name: "bars", + action: "Decrement", + amount: 10, + want: CounterStatus{ + Count: 89, + Capacity: 100, + }, + }, + "incorrect action silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "bazes": { + Count: 99, + Capacity: 100, + }}}}, + name: "bazes", + action: "decrement", + amount: 10, + want: CounterStatus{ + Count: 99, + Capacity: 100, + }, + }, + "decrement beyond count silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "baz": { + Count: 99, + Capacity: 100, + }}}}, + name: "baz", + action: "Decrement", + amount: 100, + want: CounterStatus{ + Count: 99, + Capacity: 100, + }, + }, + "increment beyond capacity silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "splayers": { + Count: 99, + Capacity: 100, + }}}}, + name: "splayers", + action: "Increment", + amount: 2, + want: CounterStatus{ + Count: 99, + Capacity: 100, + }, + }, + } + + for test, testCase := range testCases { + t.Run(test, func(t *testing.T) { + testCase.gs.UpdateCount(testCase.name, testCase.action, testCase.amount) + if counter, ok := testCase.gs.Status.Counters[testCase.name]; ok { + assert.Equal(t, testCase.want, counter) + } + }) + } +} + +func TestGameServerUpdateCounterCapacity(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + gs GameServer + name string + capacity int64 + want CounterStatus + }{ + "counter not in game server silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "foos": { + Count: 0, + Capacity: 100, + }}}}, + name: "foo", + capacity: 1000, + }, + "capacity less than zero silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "foos": { + Count: 0, + Capacity: 100, + }}}}, + name: "foos", + capacity: -1000, + want: CounterStatus{ + Count: 0, + Capacity: 100, + }, + }, + "update capacity": { + gs: GameServer{Status: GameServerStatus{ + Counters: map[string]CounterStatus{ + "sessions": { + Count: 0, + Capacity: 100, + }}}}, + name: "sessions", + capacity: 9223372036854775807, + want: CounterStatus{ + Count: 0, + Capacity: 9223372036854775807, + }, + }, + } + + for test, testCase := range testCases { + t.Run(test, func(t *testing.T) { + testCase.gs.UpdateCounterCapacity(testCase.name, testCase.capacity) + if counter, ok := testCase.gs.Status.Counters[testCase.name]; ok { + assert.Equal(t, testCase.want, counter) + } + }) + } +} + +func TestGameServerUpdateListCapacity(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + gs GameServer + name string + capacity int64 + want ListStatus + }{ + "list not in game server silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "things": { + Values: []string{}, + Capacity: 100, + }}}}, + name: "thing", + capacity: 1000, + }, + "update list capacity": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "things": { + Values: []string{}, + Capacity: 100, + }}}}, + name: "things", + capacity: 1000, + want: ListStatus{ + Values: []string{}, + Capacity: 1000, + }, + }, + "list capacity above max silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "slings": { + Values: []string{}, + Capacity: 100, + }}}}, + name: "slings", + capacity: 10000, + want: ListStatus{ + Values: []string{}, + Capacity: 100, + }, + }, + "list capacity less than zero silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "flings": { + Values: []string{}, + Capacity: 999, + }}}}, + name: "flings", + capacity: -100, + want: ListStatus{ + Values: []string{}, + Capacity: 999, + }, + }, + } + + for test, testCase := range testCases { + t.Run(test, func(t *testing.T) { + testCase.gs.UpdateListCapacity(testCase.name, testCase.capacity) + if list, ok := testCase.gs.Status.Lists[testCase.name]; ok { + assert.Equal(t, testCase.want, list) + } + }) + } +} + +func TestGameServerAppendListValues(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + gs GameServer + name string + values []string + want ListStatus + }{ + "list not in game server silent no-op": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "things": { + Values: []string{}, + Capacity: 100, + }}}}, + name: "thing", + values: []string{"thing1", "thing2", "thing3"}, + }, + "append values": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "things": { + Values: []string{"thing1"}, + Capacity: 100, + }}}}, + name: "things", + values: []string{"thing2", "thing3"}, + want: ListStatus{ + Values: []string{"thing1", "thing2", "thing3"}, + Capacity: 100, + }, + }, + "append values with duplicates": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "games": { + Values: []string{"game0"}, + Capacity: 10, + }}}}, + name: "games", + values: []string{"game1", "game2", "game2", "game1"}, + want: ListStatus{ + Values: []string{"game0", "game1", "game2"}, + Capacity: 10, + }, + }, + "append values with duplicates in original list": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "objects": { + Values: []string{"object1", "object2"}, + Capacity: 10, + }}}}, + name: "objects", + values: []string{"object2", "object1", "object3", "object3"}, + want: ListStatus{ + Values: []string{"object1", "object2", "object3"}, + Capacity: 10, + }, + }, + } + + for test, testCase := range testCases { + t.Run(test, func(t *testing.T) { + testCase.gs.AppendListValues(testCase.name, testCase.values) + if list, ok := testCase.gs.Status.Lists[testCase.name]; ok { + assert.Equal(t, testCase.want, list) + } + }) + } +} + +func TestRemoveDuplicates(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + values []string + want []string + }{ + "empty string array": { + values: []string{}, + want: []string{}, + }, + "no duplicates": { + values: []string{"one", "two", "three"}, + want: []string{"one", "two", "three"}, + }, + "remove one duplicate": { + values: []string{"one", "one", "one"}, + want: []string{"one"}, + }, + "remove multiple duplicates": { + values: []string{"one", "two", "one", "one", "two"}, + want: []string{"one", "two"}, + }, + } + + for test, testCase := range testCases { + t.Run(test, func(t *testing.T) { + got := removeDuplicates(testCase.values) + assert.Equal(t, testCase.want, got) + }) + } +} diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index 7a98c36d4b..4ae83540c8 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -109,6 +109,12 @@ type GameServerAllocationSpec struct { // MetaPatch is optional custom metadata that is added to the game server at allocation // You can use this to tell the server necessary session data MetaPatch MetaPatch `json:"metadata,omitempty"` + + // (Alpha, CountsAndLists feature flag) Counters and Lists provide a set of actions to perform + // on Counters and Lists during allocation. + // +optional + Counters map[string]CounterAction `json:"counters,omitempty"` + Lists map[string]ListAction `json:"lists,omitempty"` } // GameServerSelector contains all the filter options for selecting @@ -166,6 +172,24 @@ type ListSelector struct { MaxAvailable int64 `json:"maxAvailable"` } +// CounterAction is an optional action that can be performed on a Counter at allocation. +// Action: "Increment" or "Decrement" the Counter's Count (optional). Must also define the Amount. +// Amount: The amount to increment or decrement the Count (optional). Must be a positive integer. +// Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max int64. +type CounterAction struct { + Action *string `json:"action,omitempty"` + Amount *int64 `json:"amount,omitempty"` + Capacity *int64 `json:"capacity,omitempty"` +} + +// ListAction is an optional action that can be performed on a List at allocation. +// AddValues: Append values to a List's Values array (optional). Any duplicate values will be ignored. +// Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000. +type ListAction struct { + AddValues *[]string `json:"addValues,omitempty"` + Capacity *int64 `json:"capacity,omitempty"` +} + // ApplyDefaults applies default values func (s *GameServerSelector) ApplyDefaults() { if runtime.FeatureEnabled(runtime.FeatureStateAllocationFilter) { @@ -270,6 +294,29 @@ func (s *GameServerSelector) matchCounters(gs *agonesv1.GameServer) bool { return true } +// CounterActions attempts to peform any actions from the CounterAction on the GameServer Counter. +// TODO: Silent no-op on all actions if one cannot be performed? Or silent no-op on the action that can't be performed? +// Silent no-op if unable to perform the action. +func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) { + if ca.Capacity != nil { + gs.UpdateCounterCapacity(counter, *ca.Capacity) + } + if ca.Action != nil && ca.Amount != nil { + gs.UpdateCount(counter, *ca.Action, *ca.Amount) + } +} + +// ListActions attempts to peform any actions from the ListAction on the GameServer List. +// Silent no-op if unable to perform the action. +func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) { + if la.Capacity != nil { + gs.UpdateListCapacity(list, *la.Capacity) + } + if la.AddValues != nil { + gs.AppendListValues(list, *la.AddValues) + } +} + // matchLists returns true if there is a match for the ListSelector in the GameServerStatus func (s *GameServerSelector) matchLists(gs *agonesv1.GameServer) bool { if gs.Status.Lists == nil { diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go index b412efa7f1..7a9d9f1837 100644 --- a/pkg/apis/allocation/v1/gameserverallocation_test.go +++ b/pkg/apis/allocation/v1/gameserverallocation_test.go @@ -1016,6 +1016,191 @@ func TestGameServerSelectorMatches(t *testing.T) { } } +// Helper function for creating int64 pointers +func int64Pointer(x int64) *int64 { + return &x +} + +func TestGameServerCounterActions(t *testing.T) { + t.Parallel() + + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists))) + + DECREMENT := "Decrement" + INCREMENT := "Increment" + + testScenarios := map[string]struct { + ca CounterAction + counter string + gs *agonesv1.GameServer + want *agonesv1.GameServer + }{ + "update counter capacity": { + ca: CounterAction{ + Capacity: int64Pointer(0), + }, + counter: "mages", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "mages": { + Count: 1, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "mages": { + Count: 1, + Capacity: 0, + }}}}, + }, + "update counter count": { + ca: CounterAction{ + Action: &INCREMENT, + Amount: int64Pointer(10), + }, + counter: "baddies", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "baddies": { + Count: 1, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "baddies": { + Count: 11, + Capacity: 100, + }}}}, + }, + "update counter count and capacity": { + ca: CounterAction{ + Action: &DECREMENT, + Amount: int64Pointer(10), + Capacity: int64Pointer(10), + }, + counter: "heroes", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "heroes": { + Count: 11, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "heroes": { + Count: 1, + Capacity: 10, + }}}}, + }, + } + + for test, testScenario := range testScenarios { + t.Run(test, func(t *testing.T) { + testScenario.ca.CounterActions(testScenario.counter, testScenario.gs) + assert.Equal(t, testScenario.want, testScenario.gs) + }) + } +} + +func TestGameServerListActions(t *testing.T) { + t.Parallel() + + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists))) + + testScenarios := map[string]struct { + la ListAction + list string + gs *agonesv1.GameServer + want *agonesv1.GameServer + }{ + "update list capacity": { + la: ListAction{ + Capacity: int64Pointer(0), + }, + list: "pages", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "pages": { + Values: []string{"page1", "page2"}, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "pages": { + Values: []string{"page1", "page2"}, + Capacity: 0, + }}}}, + }, + "update list values": { + la: ListAction{ + AddValues: &[]string{"sage1", "sage3"}, + }, + list: "sages", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "sages": { + Values: []string{"sage1", "sage2"}, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "sages": { + Values: []string{"sage1", "sage2", "sage3"}, + Capacity: 100, + }}}}, + }, + "update list values and capacity": { + la: ListAction{ + AddValues: &[]string{"magician1", "magician3"}, + Capacity: int64Pointer(42), + }, + list: "magicians", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "magicians": { + Values: []string{"magician1", "magician2"}, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "magicians": { + Values: []string{"magician1", "magician2", "magician3"}, + Capacity: 42, + }}}}, + }, + "update list values and capacity - value add fails": { + la: ListAction{ + AddValues: &[]string{"fairy1", "fairy3"}, + Capacity: int64Pointer(2), + }, + list: "fairies", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "fairies": { + Values: []string{"fairy1", "fairy2"}, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Lists: map[string]agonesv1.ListStatus{ + "fairies": { + Values: []string{"fairy1", "fairy2"}, + Capacity: 2, + }}}}, + }, + } + + for test, testScenario := range testScenarios { + t.Run(test, func(t *testing.T) { + testScenario.la.ListActions(testScenario.list, testScenario.gs) + assert.Equal(t, testScenario.want, testScenario.gs) + }) + } +} + func TestGameServerAllocationValidate(t *testing.T) { t.Parallel() diff --git a/pkg/gameserverallocations/find.go b/pkg/gameserverallocations/find.go index d0c68aeb05..b278f26dcc 100644 --- a/pkg/gameserverallocations/find.go +++ b/pkg/gameserverallocations/find.go @@ -20,6 +20,7 @@ import ( "agones.dev/agones/pkg/apis" agonesv1 "agones.dev/agones/pkg/apis/agones/v1" allocationv1 "agones.dev/agones/pkg/apis/allocation/v1" + "agones.dev/agones/pkg/util/runtime" "github.com/pkg/errors" ) @@ -77,6 +78,19 @@ func findGameServerForAllocation(gsa *allocationv1.GameServerAllocation, list [] for j, sel := range gsa.Spec.Selectors { if selectors[j] == nil && sel.Matches(gs) { selectors[j] = &result{gs: gs, index: i} + // Once we have found a matching game server for allocation, perfom any Counter or List actions. + if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { + if gsa.Spec.Counters != nil { + for c, ca := range gsa.Spec.Counters { + ca.CounterActions(c, gs) + } + } + if gsa.Spec.Lists != nil { + for l, la := range gsa.Spec.Lists { + la.ListActions(l, gs) + } + } + } } } }) From 923f9ec03d64f8e261620f4826310326bd193484 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Tue, 25 Apr 2023 21:15:15 +0000 Subject: [PATCH 2/7] Adds test for findGameServerForAllocation --- pkg/gameserverallocations/find_test.go | 116 +++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/pkg/gameserverallocations/find_test.go b/pkg/gameserverallocations/find_test.go index 2613bbca97..d95b7ef8c0 100644 --- a/pkg/gameserverallocations/find_test.go +++ b/pkg/gameserverallocations/find_test.go @@ -395,3 +395,119 @@ func TestFindGameServerForAllocationDistributed(t *testing.T) { assert.FailNow(t, "We should get a different gameserver by now") } + +func TestFindGameServerForAllocationCountListActions(t *testing.T) { + t.Parallel() + + ONE := int64(1) + FORTY := int64(40) + INCREMENT := "Increment" + READY := agonesv1.GameServerStateReady + + gs1 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gs1", Namespace: defaultNs, UID: "1"}, + Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady, + Lists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{}, + Capacity: 100, + }, + }, + Counters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 101, + Capacity: 1000, + }}}} + + testScenarios := map[string]struct { + features string + gsa *allocationv1.GameServerAllocation + list []agonesv1.GameServer + listPointer []*agonesv1.GameServer + wantCounters map[string]agonesv1.CounterStatus + wantLists map[string]agonesv1.ListStatus + }{ + "CounterActions increment and ListActions append and update capacity": { + features: fmt.Sprintf("%s=true&%s=true", runtime.FeatureCountsAndLists, runtime.FeatureStateAllocationFilter), + gsa: &allocationv1.GameServerAllocation{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs}, + Spec: allocationv1.GameServerAllocationSpec{ + Selectors: []allocationv1.GameServerSelector{{ + GameServerState: &READY, + }}, + Scheduling: apis.Packed, + Counters: map[string]allocationv1.CounterAction{ + "rooms": { + Action: &INCREMENT, + Amount: &ONE, + }}, + Lists: map[string]allocationv1.ListAction{ + "players": { + AddValues: &[]string{"x7un", "8inz"}, + Capacity: &FORTY, + }}}}, + list: []agonesv1.GameServer{gs1}, + listPointer: []*agonesv1.GameServer{&gs1}, + wantCounters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 102, + Capacity: 1000, + }}, + wantLists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{"x7un", "8inz"}, + Capacity: 40, + }}, + }, + } + + for test, testScenario := range testScenarios { + t.Run(test, func(t *testing.T) { + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + // we always set the feature flag in all these tests, so always process it. + require.NoError(t, runtime.ParseFeatures(testScenario.features)) + + testScenario.gsa.ApplyDefaults() + _, ok := testScenario.gsa.Validate() + require.True(t, ok) + + controller, m := newFakeController() + c := controller.allocator.allocationCache + + m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, &agonesv1.GameServerList{Items: testScenario.list}, nil + }) + + ctx, cancel := agtesting.StartInformers(m, c.gameServerSynced) + defer cancel() + + // This call initializes the cache + err := c.syncCache() + assert.Nil(t, err) + + err = c.counter.Run(ctx, 0) + assert.Nil(t, err) + + // This test assumes only one matching game server in the list (findGameServerForAllocation + // will always return the first match and does not remove it from the list). + foundGs, _, err := findGameServerForAllocation(testScenario.gsa, testScenario.listPointer) + assert.NoError(t, err) + changes := false + for counter, counterStatus := range testScenario.wantCounters { + if gsCounter, ok := foundGs.Status.Counters[counter]; ok { + changes = true + assert.Equal(t, counterStatus, gsCounter) + } + } + for list, listStatus := range testScenario.wantLists { + if gsList, ok := foundGs.Status.Lists[list]; ok { + changes = true + assert.Equal(t, listStatus, gsList) + } + } + if changes == false { + assert.Equal(t, testScenario.list[0], foundGs) + } + }) + } +} From 24ca606e1716e09d7b9881d1d73709c28f808639 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Thu, 27 Apr 2023 19:55:39 +0000 Subject: [PATCH 3/7] Updates ListAction field to avoid pointer to a list --- pkg/apis/allocation/v1/gameserverallocation.go | 6 +++--- pkg/apis/allocation/v1/gameserverallocation_test.go | 6 +++--- pkg/gameserverallocations/find_test.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index 4ae83540c8..9d2351ac07 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -186,7 +186,7 @@ type CounterAction struct { // AddValues: Append values to a List's Values array (optional). Any duplicate values will be ignored. // Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000. type ListAction struct { - AddValues *[]string `json:"addValues,omitempty"` + AddValues []string `json:"addValues,omitempty"` Capacity *int64 `json:"capacity,omitempty"` } @@ -312,8 +312,8 @@ func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) { if la.Capacity != nil { gs.UpdateListCapacity(list, *la.Capacity) } - if la.AddValues != nil { - gs.AppendListValues(list, *la.AddValues) + if la.AddValues != nil && len(la.AddValues) > 0 { + gs.AppendListValues(list, la.AddValues) } } diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go index 7a9d9f1837..6f9c57da9b 100644 --- a/pkg/apis/allocation/v1/gameserverallocation_test.go +++ b/pkg/apis/allocation/v1/gameserverallocation_test.go @@ -1137,7 +1137,7 @@ func TestGameServerListActions(t *testing.T) { }, "update list values": { la: ListAction{ - AddValues: &[]string{"sage1", "sage3"}, + AddValues: []string{"sage1", "sage3"}, }, list: "sages", gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ @@ -1155,7 +1155,7 @@ func TestGameServerListActions(t *testing.T) { }, "update list values and capacity": { la: ListAction{ - AddValues: &[]string{"magician1", "magician3"}, + AddValues: []string{"magician1", "magician3"}, Capacity: int64Pointer(42), }, list: "magicians", @@ -1174,7 +1174,7 @@ func TestGameServerListActions(t *testing.T) { }, "update list values and capacity - value add fails": { la: ListAction{ - AddValues: &[]string{"fairy1", "fairy3"}, + AddValues: []string{"fairy1", "fairy3"}, Capacity: int64Pointer(2), }, list: "fairies", diff --git a/pkg/gameserverallocations/find_test.go b/pkg/gameserverallocations/find_test.go index d95b7ef8c0..f9e4538a71 100644 --- a/pkg/gameserverallocations/find_test.go +++ b/pkg/gameserverallocations/find_test.go @@ -442,7 +442,7 @@ func TestFindGameServerForAllocationCountListActions(t *testing.T) { }}, Lists: map[string]allocationv1.ListAction{ "players": { - AddValues: &[]string{"x7un", "8inz"}, + AddValues: []string{"x7un", "8inz"}, Capacity: &FORTY, }}}}, list: []agonesv1.GameServer{gs1}, From 85ff20b1415269dc7508fa4e7f038e9641669e41 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Thu, 27 Apr 2023 20:03:10 +0000 Subject: [PATCH 4/7] Generated Files --- .../allocation/v1/gameserverallocation.go | 2 +- .../allocation/v1/zz_generated.deepcopy.go | 71 +++++++++ .../Reference/agones_crd_api_reference.html | 147 ++++++++++++++++++ 3 files changed, 219 insertions(+), 1 deletion(-) diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index 9d2351ac07..f2609406fc 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -187,7 +187,7 @@ type CounterAction struct { // Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000. type ListAction struct { AddValues []string `json:"addValues,omitempty"` - Capacity *int64 `json:"capacity,omitempty"` + Capacity *int64 `json:"capacity,omitempty"` } // ApplyDefaults applies default values diff --git a/pkg/apis/allocation/v1/zz_generated.deepcopy.go b/pkg/apis/allocation/v1/zz_generated.deepcopy.go index e532bb88a6..f08064e317 100644 --- a/pkg/apis/allocation/v1/zz_generated.deepcopy.go +++ b/pkg/apis/allocation/v1/zz_generated.deepcopy.go @@ -26,6 +26,37 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CounterAction) DeepCopyInto(out *CounterAction) { + *out = *in + if in.Action != nil { + in, out := &in.Action, &out.Action + *out = new(string) + **out = **in + } + if in.Amount != nil { + in, out := &in.Amount, &out.Amount + *out = new(int64) + **out = **in + } + if in.Capacity != nil { + in, out := &in.Capacity, &out.Capacity + *out = new(int64) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CounterAction. +func (in *CounterAction) DeepCopy() *CounterAction { + if in == nil { + return nil + } + out := new(CounterAction) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CounterSelector) DeepCopyInto(out *CounterSelector) { *out = *in @@ -128,6 +159,20 @@ func (in *GameServerAllocationSpec) DeepCopyInto(out *GameServerAllocationSpec) } } in.MetaPatch.DeepCopyInto(&out.MetaPatch) + if in.Counters != nil { + in, out := &in.Counters, &out.Counters + *out = make(map[string]CounterAction, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } + if in.Lists != nil { + in, out := &in.Lists, &out.Lists + *out = make(map[string]ListAction, len(*in)) + for key, val := range *in { + (*out)[key] = *val.DeepCopy() + } + } return } @@ -203,6 +248,32 @@ func (in *GameServerSelector) DeepCopy() *GameServerSelector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ListAction) DeepCopyInto(out *ListAction) { + *out = *in + if in.AddValues != nil { + in, out := &in.AddValues, &out.AddValues + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Capacity != nil { + in, out := &in.Capacity, &out.Capacity + *out = new(int64) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ListAction. +func (in *ListAction) DeepCopy() *ListAction { + if in == nil { + return nil + } + out := new(ListAction) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ListSelector) DeepCopyInto(out *ListSelector) { *out = *in diff --git a/site/content/en/docs/Reference/agones_crd_api_reference.html b/site/content/en/docs/Reference/agones_crd_api_reference.html index 45c685a2f2..df62bde126 100644 --- a/site/content/en/docs/Reference/agones_crd_api_reference.html +++ b/site/content/en/docs/Reference/agones_crd_api_reference.html @@ -5704,6 +5704,33 @@

GameServerAllocation You can use this to tell the server necessary session data

+ + +counters
+ + +map[string]agones.dev/agones/pkg/apis/allocation/v1.CounterAction + + + + +(Optional) +

(Alpha, CountsAndLists feature flag) Counters and Lists provide a set of actions to perform +on Counters and Lists during allocation.

+ + + + +lists
+ + +map[string]agones.dev/agones/pkg/apis/allocation/v1.ListAction + + + + + + @@ -5721,6 +5748,58 @@

GameServerAllocation +

CounterAction +

+

+(Appears on: +GameServerAllocationSpec) +

+

+

CounterAction is an optional action that can be performed on a Counter at allocation. +Action: “Increment” or “Decrement” the Counter’s Count (optional). Must also define the Amount. +Amount: The amount to increment or decrement the Count (optional). Must be a positive integer. +Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max int64.

+

+ + + + + + + + + + + + + + + + + + + + + +
FieldDescription
+action
+ +string + +		 +
+amount
+ +int64 + +		 +
+capacity
+ +int64 + +		 +

CounterSelector

@@ -5903,6 +5982,33 @@

GameServerAllocationS You can use this to tell the server necessary session data

+ + +counters
+ + +map[string]agones.dev/agones/pkg/apis/allocation/v1.CounterAction + + + + +(Optional) +

(Alpha, CountsAndLists feature flag) Counters and Lists provide a set of actions to perform +on Counters and Lists during allocation.

+ + + + +lists
+ + +map[string]agones.dev/agones/pkg/apis/allocation/v1.ListAction + + + + + +

GameServerAllocationState @@ -6103,6 +6209,47 @@

GameServerSelector +

ListAction +

+

+(Appears on: +GameServerAllocationSpec) +

+

+

ListAction is an optional action that can be performed on a List at allocation. +AddValues: Append values to a List’s Values array (optional). Any duplicate values will be ignored. +Capacity: Update the maximum capacity of the Counter to this number (optional). Min 0, Max 1000.

+

+ + + + + + + + + + + + + + + + + +
FieldDescription
+addValues
+ +[]string + +		 +
+capacity
+ +int64 + +		 +

ListSelector

From 7707ce6705002a755ededdb53cd31728f7823c44 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Tue, 2 May 2023 16:41:02 +0000 Subject: [PATCH 5/7] Moves counter and list action kick to the Allocator.goAdds logging of any counter or list actions as warning events --- pkg/apis/agones/v1/common.go | 2 +- pkg/apis/agones/v1/gameserver.go | 48 ++++-- pkg/apis/agones/v1/gameserver_test.go | 148 +++++++++++++---- .../allocation/v1/gameserverallocation.go | 33 +++- .../v1/gameserverallocation_test.go | 52 ++++-- pkg/gameserverallocations/allocator.go | 25 ++- pkg/gameserverallocations/allocator_test.go | 156 +++++++++++++++++- pkg/gameserverallocations/find.go | 14 -- pkg/gameserverallocations/find_test.go | 116 ------------- 9 files changed, 389 insertions(+), 205 deletions(-) diff --git a/pkg/apis/agones/v1/common.go b/pkg/apis/agones/v1/common.go index bafe72e999..c38e546c57 100644 --- a/pkg/apis/agones/v1/common.go +++ b/pkg/apis/agones/v1/common.go @@ -26,7 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// Block of const +// nolint:stylecheck const ( ErrContainerRequired = "Container is required when using multiple containers in the pod template" ErrHostPort = "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy" diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index e5dbc8086e..4d791c224b 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -928,56 +928,67 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) { } // UpdateCount increments or decrements a CounterStatus on a Game Server by the given amount. -// TODO: Should we log on noncomplemention or return false? Issue #2716 asks for "silent no-op" -func (gs *GameServer) UpdateCount(name string, action string, amount int64) { +func (gs *GameServer) UpdateCount(name string, action string, amount int64) error { + err := fmt.Errorf("unable to UpdateCount: Name %s, Action %s, Amount %d", name, action, amount) if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) || amount < 0 { - return + return err } if counter, ok := gs.Status.Counters[name]; ok { cnt := counter.Count if action == GameServerAllocationIncrement { cnt += amount + // only check for Count > Capacity when incrementing + if cnt > counter.Capacity { + return err + } } if action == GameServerAllocationDecrement { cnt -= amount } - // TODO: Can we force the Capacity to update first before updating the Count if CounterAction updates both? - if (cnt < 0) || (cnt > counter.Capacity) { - return + if cnt < 0 { + return err } counter.Count = cnt gs.Status.Counters[name] = counter + return nil } + return err } -// UpdateCounterCapacity updates the CounterStatus Capacity to the given capacity. Silent no-op. -// TODO: Do anything if Capacity is updated to be less than the current Count? -func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) { +// UpdateCounterCapacity updates the CounterStatus Capacity to the given capacity. +func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) error { + err := fmt.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d", name, capacity) if capacity < 0 { - return + return err } if counter, ok := gs.Status.Counters[name]; ok { counter.Capacity = capacity gs.Status.Counters[name] = counter + return nil } + return err } -// UpdateListCapacity updates the ListStatus Capacity to the given capacity. Silent no-op. -func (gs *GameServer) UpdateListCapacity(name string, capacity int64) { +// UpdateListCapacity updates the ListStatus Capacity to the given capacity. +func (gs *GameServer) UpdateListCapacity(name string, capacity int64) error { + err := fmt.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d", name, capacity) if capacity < 0 || capacity > 1000 { - return + return err } if list, ok := gs.Status.Lists[name]; ok { list.Capacity = capacity gs.Status.Lists[name] = list + return nil } + return err } // AppendListValues adds given the values to the ListStatus Values list. Any duplicates are silently -// dropped. Silent no-op. -func (gs *GameServer) AppendListValues(name string, values []string) { +// dropped. +func (gs *GameServer) AppendListValues(name string, values []string) error { + err := fmt.Errorf("unable to AppendListValues: Name %s, Values %s", name, values) if len(values) == 0 { - return + return err } if list, ok := gs.Status.Lists[name]; ok { vals := []string{} @@ -986,12 +997,13 @@ func (gs *GameServer) AppendListValues(name string, values []string) { vals = append(vals, values...) vals = removeDuplicates(vals) if (len(vals) > int(list.Capacity)) || (len(vals) == len(list.Values)) { - return + return err } - // TODO: Do we need deepcopy here? list.Values = vals gs.Status.Lists[name] = list + return nil } + return err } // removeDuplicates removes any duplicate values from a list. Returns new list with unique values only. diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index 526e04216e..0f3fedcb9d 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -1640,24 +1640,26 @@ func TestGameServerUpdateCount(t *testing.T) { t.Parallel() testCases := map[string]struct { - gs GameServer - name string - action string - amount int64 - want CounterStatus + gs GameServer + name string + action string + amount int64 + want CounterStatus + wantErr bool }{ - "counter not in game server silent no-op": { + "counter not in game server no-op and error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "foos": { Count: 0, Capacity: 100, }}}}, - name: "foo", - action: "Increment", - amount: 1, + name: "foo", + action: "Increment", + amount: 1, + wantErr: true, }, - "amount less than zero silent no-op": { + "amount less than zero no-op and error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "foos": { @@ -1671,6 +1673,7 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 1, Capacity: 100, }, + wantErr: true, }, "increment by 1": { gs: GameServer{Status: GameServerStatus{ @@ -1686,6 +1689,7 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 1, Capacity: 100, }, + wantErr: false, }, "decrement by 10": { gs: GameServer{Status: GameServerStatus{ @@ -1701,8 +1705,9 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 89, Capacity: 100, }, + wantErr: false, }, - "incorrect action silent no-op": { + "incorrect action no-op and error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "bazes": { @@ -1716,8 +1721,9 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 99, Capacity: 100, }, + wantErr: true, }, - "decrement beyond count silent no-op": { + "decrement beyond count no-op and error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "baz": { @@ -1731,8 +1737,9 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 99, Capacity: 100, }, + wantErr: true, }, - "increment beyond capacity silent no-op": { + "increment beyond capacity no-op and error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "splayers": { @@ -1746,12 +1753,18 @@ func TestGameServerUpdateCount(t *testing.T) { Count: 99, Capacity: 100, }, + wantErr: true, }, } for test, testCase := range testCases { t.Run(test, func(t *testing.T) { - testCase.gs.UpdateCount(testCase.name, testCase.action, testCase.amount) + err := testCase.gs.UpdateCount(testCase.name, testCase.action, testCase.amount) + if err != nil { + assert.True(t, testCase.wantErr) + } else { + assert.False(t, testCase.wantErr) + } if counter, ok := testCase.gs.Status.Counters[testCase.name]; ok { assert.Equal(t, testCase.want, counter) } @@ -1767,8 +1780,9 @@ func TestGameServerUpdateCounterCapacity(t *testing.T) { name string capacity int64 want CounterStatus + wantErr bool }{ - "counter not in game server silent no-op": { + "counter not in game server no-op with error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "foos": { @@ -1777,8 +1791,9 @@ func TestGameServerUpdateCounterCapacity(t *testing.T) { }}}}, name: "foo", capacity: 1000, + wantErr: true, }, - "capacity less than zero silent no-op": { + "capacity less than zero no-op with error": { gs: GameServer{Status: GameServerStatus{ Counters: map[string]CounterStatus{ "foos": { @@ -1791,6 +1806,7 @@ func TestGameServerUpdateCounterCapacity(t *testing.T) { Count: 0, Capacity: 100, }, + wantErr: true, }, "update capacity": { gs: GameServer{Status: GameServerStatus{ @@ -1810,7 +1826,12 @@ func TestGameServerUpdateCounterCapacity(t *testing.T) { for test, testCase := range testCases { t.Run(test, func(t *testing.T) { - testCase.gs.UpdateCounterCapacity(testCase.name, testCase.capacity) + err := testCase.gs.UpdateCounterCapacity(testCase.name, testCase.capacity) + if err != nil { + assert.True(t, testCase.wantErr) + } else { + assert.False(t, testCase.wantErr) + } if counter, ok := testCase.gs.Status.Counters[testCase.name]; ok { assert.Equal(t, testCase.want, counter) } @@ -1826,8 +1847,9 @@ func TestGameServerUpdateListCapacity(t *testing.T) { name string capacity int64 want ListStatus + wantErr bool }{ - "list not in game server silent no-op": { + "list not in game server no-op with error": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "things": { @@ -1836,6 +1858,7 @@ func TestGameServerUpdateListCapacity(t *testing.T) { }}}}, name: "thing", capacity: 1000, + wantErr: true, }, "update list capacity": { gs: GameServer{Status: GameServerStatus{ @@ -1850,8 +1873,9 @@ func TestGameServerUpdateListCapacity(t *testing.T) { Values: []string{}, Capacity: 1000, }, + wantErr: false, }, - "list capacity above max silent no-op": { + "list capacity above max no-op with error": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "slings": { @@ -1864,8 +1888,9 @@ func TestGameServerUpdateListCapacity(t *testing.T) { Values: []string{}, Capacity: 100, }, + wantErr: true, }, - "list capacity less than zero silent no-op": { + "list capacity less than zero no-op with error": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "flings": { @@ -1878,12 +1903,18 @@ func TestGameServerUpdateListCapacity(t *testing.T) { Values: []string{}, Capacity: 999, }, + wantErr: true, }, } for test, testCase := range testCases { t.Run(test, func(t *testing.T) { - testCase.gs.UpdateListCapacity(testCase.name, testCase.capacity) + err := testCase.gs.UpdateListCapacity(testCase.name, testCase.capacity) + if err != nil { + assert.True(t, testCase.wantErr) + } else { + assert.False(t, testCase.wantErr) + } if list, ok := testCase.gs.Status.Lists[testCase.name]; ok { assert.Equal(t, testCase.want, list) } @@ -1895,20 +1926,22 @@ func TestGameServerAppendListValues(t *testing.T) { t.Parallel() testCases := map[string]struct { - gs GameServer - name string - values []string - want ListStatus + gs GameServer + name string + values []string + want ListStatus + wantErr bool }{ - "list not in game server silent no-op": { + "list not in game server no-op with error": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "things": { Values: []string{}, Capacity: 100, }}}}, - name: "thing", - values: []string{"thing1", "thing2", "thing3"}, + name: "thing", + values: []string{"thing1", "thing2", "thing3"}, + wantErr: true, }, "append values": { gs: GameServer{Status: GameServerStatus{ @@ -1923,8 +1956,9 @@ func TestGameServerAppendListValues(t *testing.T) { Values: []string{"thing1", "thing2", "thing3"}, Capacity: 100, }, + wantErr: false, }, - "append values with duplicates": { + "append values with silent drop of duplicates": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "games": { @@ -1937,8 +1971,9 @@ func TestGameServerAppendListValues(t *testing.T) { Values: []string{"game0", "game1", "game2"}, Capacity: 10, }, + wantErr: false, }, - "append values with duplicates in original list": { + "append values with silent drop of duplicates in original list": { gs: GameServer{Status: GameServerStatus{ Lists: map[string]ListStatus{ "objects": { @@ -1951,12 +1986,63 @@ func TestGameServerAppendListValues(t *testing.T) { Values: []string{"object1", "object2", "object3"}, Capacity: 10, }, + wantErr: false, + }, + "append no values no-op with error": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "blings": { + Values: []string{"bling1"}, + Capacity: 10, + }}}}, + name: "blings", + values: []string{}, + want: ListStatus{ + Values: []string{"bling1"}, + Capacity: 10, + }, + wantErr: true, + }, + "append only duplicates no-op with error": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "slings": { + Values: []string{"slings1", "sling2"}, + Capacity: 4, + }}}}, + name: "blings", + values: []string{}, + want: ListStatus{ + Values: []string{"slings1", "sling2"}, + Capacity: 4, + }, + wantErr: true, + }, + "append too many values no-op with error": { + gs: GameServer{Status: GameServerStatus{ + Lists: map[string]ListStatus{ + "bananaslugs": { + Values: []string{"bananaslugs1", "bananaslug2", "bananaslug3"}, + Capacity: 5, + }}}}, + name: "bananaslugs", + values: []string{"bananaslug4", "bananaslug5", "bananaslug6"}, + want: ListStatus{ + Values: []string{"bananaslugs1", "bananaslug2", "bananaslug3"}, + Capacity: 5, + }, + wantErr: true, }, } for test, testCase := range testCases { t.Run(test, func(t *testing.T) { - testCase.gs.AppendListValues(testCase.name, testCase.values) + err := testCase.gs.AppendListValues(testCase.name, testCase.values) + if err != nil { + assert.True(t, testCase.wantErr) + } else { + assert.False(t, testCase.wantErr) + } if list, ok := testCase.gs.Status.Lists[testCase.name]; ok { assert.Equal(t, testCase.want, list) } diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index f2609406fc..279ad2b789 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -295,26 +295,41 @@ func (s *GameServerSelector) matchCounters(gs *agonesv1.GameServer) bool { } // CounterActions attempts to peform any actions from the CounterAction on the GameServer Counter. -// TODO: Silent no-op on all actions if one cannot be performed? Or silent no-op on the action that can't be performed? -// Silent no-op if unable to perform the action. -func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) { +// Returns a string list of any actions that could not be performed. +func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) []string { + errors := []string{} if ca.Capacity != nil { - gs.UpdateCounterCapacity(counter, *ca.Capacity) + capErr := gs.UpdateCounterCapacity(counter, *ca.Capacity) + if capErr != nil { + errors = append(errors, capErr.Error()) + } } if ca.Action != nil && ca.Amount != nil { - gs.UpdateCount(counter, *ca.Action, *ca.Amount) + cntErr := gs.UpdateCount(counter, *ca.Action, *ca.Amount) + if cntErr != nil { + errors = append(errors, cntErr.Error()) + } } + return errors } // ListActions attempts to peform any actions from the ListAction on the GameServer List. -// Silent no-op if unable to perform the action. -func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) { +// Returns a string list of any actions that could not be performed. +func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) []string { + errors := []string{} if la.Capacity != nil { - gs.UpdateListCapacity(list, *la.Capacity) + capErr := gs.UpdateListCapacity(list, *la.Capacity) + if capErr != nil { + errors = append(errors, capErr.Error()) + } } if la.AddValues != nil && len(la.AddValues) > 0 { - gs.AppendListValues(list, la.AddValues) + cntErr := gs.AppendListValues(list, la.AddValues) + if cntErr != nil { + errors = append(errors, cntErr.Error()) + } } + return errors } // matchLists returns true if there is a match for the ListSelector in the GameServerStatus diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go index 6f9c57da9b..8a0d81d93c 100644 --- a/pkg/apis/allocation/v1/gameserverallocation_test.go +++ b/pkg/apis/allocation/v1/gameserverallocation_test.go @@ -1032,10 +1032,11 @@ func TestGameServerCounterActions(t *testing.T) { INCREMENT := "Increment" testScenarios := map[string]struct { - ca CounterAction - counter string - gs *agonesv1.GameServer - want *agonesv1.GameServer + ca CounterAction + counter string + gs *agonesv1.GameServer + want *agonesv1.GameServer + errLength int }{ "update counter capacity": { ca: CounterAction{ @@ -1054,6 +1055,28 @@ func TestGameServerCounterActions(t *testing.T) { Count: 1, Capacity: 0, }}}}, + errLength: 0, + }, + "fail update counter capacity and count": { + ca: CounterAction{ + Action: &INCREMENT, + Amount: int64Pointer(10), + Capacity: int64Pointer(-1), + }, + counter: "sages", + gs: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "sages": { + Count: 99, + Capacity: 100, + }}}}, + want: &agonesv1.GameServer{Status: agonesv1.GameServerStatus{ + Counters: map[string]agonesv1.CounterStatus{ + "sages": { + Count: 99, + Capacity: 100, + }}}}, + errLength: 2, }, "update counter count": { ca: CounterAction{ @@ -1073,6 +1096,7 @@ func TestGameServerCounterActions(t *testing.T) { Count: 11, Capacity: 100, }}}}, + errLength: 0, }, "update counter count and capacity": { ca: CounterAction{ @@ -1093,13 +1117,15 @@ func TestGameServerCounterActions(t *testing.T) { Count: 1, Capacity: 10, }}}}, + errLength: 0, }, } for test, testScenario := range testScenarios { t.Run(test, func(t *testing.T) { - testScenario.ca.CounterActions(testScenario.counter, testScenario.gs) + errs := testScenario.ca.CounterActions(testScenario.counter, testScenario.gs) assert.Equal(t, testScenario.want, testScenario.gs) + assert.Equal(t, testScenario.errLength, len(errs)) }) } } @@ -1112,10 +1138,11 @@ func TestGameServerListActions(t *testing.T) { assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists))) testScenarios := map[string]struct { - la ListAction - list string - gs *agonesv1.GameServer - want *agonesv1.GameServer + la ListAction + list string + gs *agonesv1.GameServer + want *agonesv1.GameServer + errLength int }{ "update list capacity": { la: ListAction{ @@ -1134,6 +1161,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"page1", "page2"}, Capacity: 0, }}}}, + errLength: 0, }, "update list values": { la: ListAction{ @@ -1152,6 +1180,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"sage1", "sage2", "sage3"}, Capacity: 100, }}}}, + errLength: 0, }, "update list values and capacity": { la: ListAction{ @@ -1171,6 +1200,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"magician1", "magician2", "magician3"}, Capacity: 42, }}}}, + errLength: 0, }, "update list values and capacity - value add fails": { la: ListAction{ @@ -1190,13 +1220,15 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"fairy1", "fairy2"}, Capacity: 2, }}}}, + errLength: 1, }, } for test, testScenario := range testScenarios { t.Run(test, func(t *testing.T) { - testScenario.la.ListActions(testScenario.list, testScenario.gs) + errs := testScenario.la.ListActions(testScenario.list, testScenario.gs) assert.Equal(t, testScenario.want, testScenario.gs) + assert.Equal(t, testScenario.errLength, len(errs)) }) } } diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 1b7ad87521..69895f384d 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/pkg/gameserverallocations/allocator.go @@ -553,7 +553,7 @@ func (c *Allocator) allocationUpdateWorkers(ctx context.Context, workerCount int for { select { case res := <-updateQueue: - gs, err := c.applyAllocationToGameServer(ctx, res.request.gsa.Spec.MetaPatch, res.gs) + gs, err := c.applyAllocationToGameServer(ctx, res.request.gsa.Spec.MetaPatch, res.gs, res.request.gsa) if err != nil { if !k8serrors.IsConflict(errors.Cause(err)) { // since we could not allocate, we should put it back @@ -580,7 +580,7 @@ func (c *Allocator) allocationUpdateWorkers(ctx context.Context, workerCount int // applyAllocationToGameServer patches the inputted GameServer with the allocation metadata changes, and updates it to the Allocated State. // Returns the updated GameServer. -func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocationv1.MetaPatch, gs *agonesv1.GameServer) (*agonesv1.GameServer, error) { +func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocationv1.MetaPatch, gs *agonesv1.GameServer, gsa *allocationv1.GameServerAllocation) (*agonesv1.GameServer, error) { // patch ObjectMeta labels if mp.Labels != nil { if gs.ObjectMeta.Labels == nil { @@ -607,6 +607,27 @@ func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocati gs.ObjectMeta.Annotations[LastAllocatedAnnotationKey] = string(ts) gs.Status.State = agonesv1.GameServerStateAllocated + // perfom any Counter or List actions + if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { + actionErrors := []string{} + if gsa.Spec.Counters != nil { + for counter, ca := range gsa.Spec.Counters { + errs := ca.CounterActions(counter, gs) + actionErrors = append(actionErrors, errs...) + } + } + if gsa.Spec.Lists != nil { + for list, la := range gsa.Spec.Lists { + errs := la.ListActions(list, gs) + actionErrors = append(actionErrors, errs...) + } + } + // record any Counter or List action errors as a warning + if len(actionErrors) != 0 { + c.recorder.Event(gs, corev1.EventTypeWarning, string(runtime.FeatureCountsAndLists), fmt.Sprint(actionErrors)) + } + } + return c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gs, metav1.UpdateOptions{}) } diff --git a/pkg/gameserverallocations/allocator_test.go b/pkg/gameserverallocations/allocator_test.go index 0f4a9ae3f8..48e17b7cf5 100644 --- a/pkg/gameserverallocations/allocator_test.go +++ b/pkg/gameserverallocations/allocator_test.go @@ -17,6 +17,7 @@ package gameserverallocations import ( "context" "errors" + "fmt" "testing" "time" @@ -26,6 +27,7 @@ import ( multiclusterv1 "agones.dev/agones/pkg/apis/multicluster/v1" "agones.dev/agones/pkg/gameservers" agtesting "agones.dev/agones/pkg/testing" + "agones.dev/agones/pkg/util/runtime" "github.com/heptiolabs/healthcheck" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" @@ -245,14 +247,14 @@ func TestAllocatorApplyAllocationToGameServer(t *testing.T) { time.Second, 5*time.Second, 500*time.Millisecond, ) - gs, err := allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{}, &agonesv1.GameServer{}) + gs, err := allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{}, &agonesv1.GameServer{}, &allocationv1.GameServerAllocation{}) assert.NoError(t, err) assert.Equal(t, agonesv1.GameServerStateAllocated, gs.Status.State) assert.NotNil(t, gs.ObjectMeta.Annotations["agones.dev/last-allocated"]) var ts time.Time assert.NoError(t, ts.UnmarshalText([]byte(gs.ObjectMeta.Annotations[LastAllocatedAnnotationKey]))) - gs, err = allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{Labels: map[string]string{"foo": "bar"}}, &agonesv1.GameServer{}) + gs, err = allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{Labels: map[string]string{"foo": "bar"}}, &agonesv1.GameServer{}, &allocationv1.GameServerAllocation{}) assert.NoError(t, err) assert.Equal(t, agonesv1.GameServerStateAllocated, gs.Status.State) assert.Equal(t, "bar", gs.ObjectMeta.Labels["foo"]) @@ -260,7 +262,7 @@ func TestAllocatorApplyAllocationToGameServer(t *testing.T) { gs, err = allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{Labels: map[string]string{"foo": "bar"}, Annotations: map[string]string{"bar": "foo"}}, - &agonesv1.GameServer{}) + &agonesv1.GameServer{}, &allocationv1.GameServerAllocation{}) assert.NoError(t, err) assert.Equal(t, agonesv1.GameServerStateAllocated, gs.Status.State) assert.Equal(t, "bar", gs.ObjectMeta.Labels["foo"]) @@ -268,6 +270,152 @@ func TestAllocatorApplyAllocationToGameServer(t *testing.T) { assert.NotNil(t, gs.ObjectMeta.Annotations[LastAllocatedAnnotationKey]) } +func TestAllocatorApplyAllocationToGameServerCountsListsActions(t *testing.T) { + t.Parallel() + m := agtesting.NewMocks() + ctx := context.Background() + mp := allocationv1.MetaPatch{} + + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, k8sruntime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*agonesv1.GameServer) + return true, gs, nil + }) + + allocator := NewAllocator(m.AgonesInformerFactory.Multicluster().V1().GameServerAllocationPolicies(), + m.KubeInformerFactory.Core().V1().Secrets(), + m.AgonesClient.AgonesV1(), m.KubeClient, + NewAllocationCache(m.AgonesInformerFactory.Agones().V1().GameServers(), gameservers.NewPerNodeCounter(m.KubeInformerFactory, m.AgonesInformerFactory), healthcheck.NewHandler()), + time.Second, 5*time.Second, 500*time.Millisecond, + ) + + ONE := int64(1) + FORTY := int64(40) + THOUSAND := int64(1000) + INCREMENT := "Increment" + READY := agonesv1.GameServerStateReady + + gs1 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gs1", Namespace: defaultNs, UID: "1"}, + Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady, + Lists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{}, + Capacity: 100, + }, + }, + Counters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 101, + Capacity: 1000, + }}}} + gs2 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gs2", Namespace: defaultNs, UID: "2"}, + Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady, + Lists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{}, + Capacity: 100, + }, + }, + Counters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 101, + Capacity: 1000, + }}}} + + testScenarios := map[string]struct { + features string + gs *agonesv1.GameServer + gsa *allocationv1.GameServerAllocation + wantCounters map[string]agonesv1.CounterStatus + wantLists map[string]agonesv1.ListStatus + }{ + "CounterActions increment and ListActions append and update capacity": { + features: fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists), + gs: &gs1, + gsa: &allocationv1.GameServerAllocation{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs}, + Spec: allocationv1.GameServerAllocationSpec{ + Selectors: []allocationv1.GameServerSelector{{ + GameServerState: &READY, + }}, + Scheduling: apis.Packed, + Counters: map[string]allocationv1.CounterAction{ + "rooms": { + Action: &INCREMENT, + Amount: &ONE, + }}, + Lists: map[string]allocationv1.ListAction{ + "players": { + AddValues: []string{"x7un", "8inz"}, + Capacity: &FORTY, + }}}}, + wantCounters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 102, + Capacity: 1000, + }}, + wantLists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{"x7un", "8inz"}, + Capacity: 40, + }}, + }, + "CounterActions and ListActions only update list capacity": { + features: fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists), + gs: &gs2, + gsa: &allocationv1.GameServerAllocation{ + ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs}, + Spec: allocationv1.GameServerAllocationSpec{ + Selectors: []allocationv1.GameServerSelector{{ + GameServerState: &READY, + }}, + Scheduling: apis.Packed, + Counters: map[string]allocationv1.CounterAction{ + "rooms": { + Action: &INCREMENT, + Amount: &THOUSAND, + }}, + Lists: map[string]allocationv1.ListAction{ + "players": { + AddValues: []string{"x7un", "8inz"}, + Capacity: &ONE, + }}}}, + wantCounters: map[string]agonesv1.CounterStatus{ + "rooms": { + Count: 101, + Capacity: 1000, + }}, + wantLists: map[string]agonesv1.ListStatus{ + "players": { + Values: []string{}, + Capacity: 1, + }}, + }, + } + + for test, testScenario := range testScenarios { + t.Run(test, func(t *testing.T) { + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + // we always set the feature flag in all these tests, so always process it. + require.NoError(t, runtime.ParseFeatures(testScenario.features)) + + foundGs, err := allocator.applyAllocationToGameServer(ctx, mp, testScenario.gs, testScenario.gsa) + assert.NoError(t, err) + for counter, counterStatus := range testScenario.wantCounters { + if gsCounter, ok := foundGs.Status.Counters[counter]; ok { + assert.Equal(t, counterStatus, gsCounter) + } + } + for list, listStatus := range testScenario.wantLists { + if gsList, ok := foundGs.Status.Lists[list]; ok { + assert.Equal(t, listStatus, gsList) + } + } + }) + } +} + func TestAllocationApplyAllocationError(t *testing.T) { t.Parallel() m := agtesting.NewMocks() @@ -284,7 +432,7 @@ func TestAllocationApplyAllocationError(t *testing.T) { time.Second, 5*time.Second, 500*time.Millisecond, ) - gsa, err := allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{}, &agonesv1.GameServer{}) + gsa, err := allocator.applyAllocationToGameServer(ctx, allocationv1.MetaPatch{}, &agonesv1.GameServer{}, &allocationv1.GameServerAllocation{}) logrus.WithError(err).WithField("gsa", gsa).WithField("test", t.Name()).Info("Allocation should fail") assert.Error(t, err) } diff --git a/pkg/gameserverallocations/find.go b/pkg/gameserverallocations/find.go index b278f26dcc..d0c68aeb05 100644 --- a/pkg/gameserverallocations/find.go +++ b/pkg/gameserverallocations/find.go @@ -20,7 +20,6 @@ import ( "agones.dev/agones/pkg/apis" agonesv1 "agones.dev/agones/pkg/apis/agones/v1" allocationv1 "agones.dev/agones/pkg/apis/allocation/v1" - "agones.dev/agones/pkg/util/runtime" "github.com/pkg/errors" ) @@ -78,19 +77,6 @@ func findGameServerForAllocation(gsa *allocationv1.GameServerAllocation, list [] for j, sel := range gsa.Spec.Selectors { if selectors[j] == nil && sel.Matches(gs) { selectors[j] = &result{gs: gs, index: i} - // Once we have found a matching game server for allocation, perfom any Counter or List actions. - if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { - if gsa.Spec.Counters != nil { - for c, ca := range gsa.Spec.Counters { - ca.CounterActions(c, gs) - } - } - if gsa.Spec.Lists != nil { - for l, la := range gsa.Spec.Lists { - la.ListActions(l, gs) - } - } - } } } }) diff --git a/pkg/gameserverallocations/find_test.go b/pkg/gameserverallocations/find_test.go index f9e4538a71..2613bbca97 100644 --- a/pkg/gameserverallocations/find_test.go +++ b/pkg/gameserverallocations/find_test.go @@ -395,119 +395,3 @@ func TestFindGameServerForAllocationDistributed(t *testing.T) { assert.FailNow(t, "We should get a different gameserver by now") } - -func TestFindGameServerForAllocationCountListActions(t *testing.T) { - t.Parallel() - - ONE := int64(1) - FORTY := int64(40) - INCREMENT := "Increment" - READY := agonesv1.GameServerStateReady - - gs1 := agonesv1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gs1", Namespace: defaultNs, UID: "1"}, - Status: agonesv1.GameServerStatus{NodeName: "node1", State: agonesv1.GameServerStateReady, - Lists: map[string]agonesv1.ListStatus{ - "players": { - Values: []string{}, - Capacity: 100, - }, - }, - Counters: map[string]agonesv1.CounterStatus{ - "rooms": { - Count: 101, - Capacity: 1000, - }}}} - - testScenarios := map[string]struct { - features string - gsa *allocationv1.GameServerAllocation - list []agonesv1.GameServer - listPointer []*agonesv1.GameServer - wantCounters map[string]agonesv1.CounterStatus - wantLists map[string]agonesv1.ListStatus - }{ - "CounterActions increment and ListActions append and update capacity": { - features: fmt.Sprintf("%s=true&%s=true", runtime.FeatureCountsAndLists, runtime.FeatureStateAllocationFilter), - gsa: &allocationv1.GameServerAllocation{ - ObjectMeta: metav1.ObjectMeta{Namespace: defaultNs}, - Spec: allocationv1.GameServerAllocationSpec{ - Selectors: []allocationv1.GameServerSelector{{ - GameServerState: &READY, - }}, - Scheduling: apis.Packed, - Counters: map[string]allocationv1.CounterAction{ - "rooms": { - Action: &INCREMENT, - Amount: &ONE, - }}, - Lists: map[string]allocationv1.ListAction{ - "players": { - AddValues: []string{"x7un", "8inz"}, - Capacity: &FORTY, - }}}}, - list: []agonesv1.GameServer{gs1}, - listPointer: []*agonesv1.GameServer{&gs1}, - wantCounters: map[string]agonesv1.CounterStatus{ - "rooms": { - Count: 102, - Capacity: 1000, - }}, - wantLists: map[string]agonesv1.ListStatus{ - "players": { - Values: []string{"x7un", "8inz"}, - Capacity: 40, - }}, - }, - } - - for test, testScenario := range testScenarios { - t.Run(test, func(t *testing.T) { - runtime.FeatureTestMutex.Lock() - defer runtime.FeatureTestMutex.Unlock() - // we always set the feature flag in all these tests, so always process it. - require.NoError(t, runtime.ParseFeatures(testScenario.features)) - - testScenario.gsa.ApplyDefaults() - _, ok := testScenario.gsa.Validate() - require.True(t, ok) - - controller, m := newFakeController() - c := controller.allocator.allocationCache - - m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, k8sruntime.Object, error) { - return true, &agonesv1.GameServerList{Items: testScenario.list}, nil - }) - - ctx, cancel := agtesting.StartInformers(m, c.gameServerSynced) - defer cancel() - - // This call initializes the cache - err := c.syncCache() - assert.Nil(t, err) - - err = c.counter.Run(ctx, 0) - assert.Nil(t, err) - - // This test assumes only one matching game server in the list (findGameServerForAllocation - // will always return the first match and does not remove it from the list). - foundGs, _, err := findGameServerForAllocation(testScenario.gsa, testScenario.listPointer) - assert.NoError(t, err) - changes := false - for counter, counterStatus := range testScenario.wantCounters { - if gsCounter, ok := foundGs.Status.Counters[counter]; ok { - changes = true - assert.Equal(t, counterStatus, gsCounter) - } - } - for list, listStatus := range testScenario.wantLists { - if gsList, ok := foundGs.Status.Lists[list]; ok { - changes = true - assert.Equal(t, listStatus, gsList) - } - } - if changes == false { - assert.Equal(t, testScenario.list[0], foundGs) - } - }) - } -} From 17398eb9640fccfc46cf4264ae2a31c047d416af Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Fri, 5 May 2023 18:25:21 +0000 Subject: [PATCH 6/7] Updates actions to return single error --- pkg/apis/agones/v1/common.go | 4 +- pkg/apis/agones/v1/gameserver.go | 83 ++++++++++--------- pkg/apis/agones/v1/gameserver_test.go | 31 ++++--- .../allocation/v1/gameserverallocation.go | 23 ++--- .../v1/gameserverallocation_test.go | 48 ++++++----- pkg/gameserverallocations/allocator.go | 17 ++-- 6 files changed, 115 insertions(+), 91 deletions(-) diff --git a/pkg/apis/agones/v1/common.go b/pkg/apis/agones/v1/common.go index c38e546c57..12fcd3d212 100644 --- a/pkg/apis/agones/v1/common.go +++ b/pkg/apis/agones/v1/common.go @@ -26,7 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// nolint:stylecheck +// Block of const Error messages and GameServerAllocation Counter actions const ( ErrContainerRequired = "Container is required when using multiple containers in the pod template" ErrHostPort = "HostPort cannot be specified with a Dynamic or Passthrough PortPolicy" @@ -36,7 +36,7 @@ const ( ErrContainerNameInvalid = "Container must be empty or the name of a container in the pod template" // GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be incremented at Allocation. GameServerAllocationIncrement string = "Increment" - // GameServerAllocationIncrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation. + // GameServerAllocationDecrement is a Counter Action that indiciates the Counter's Count should be decremented at Allocation. GameServerAllocationDecrement string = "Decrement" ) diff --git a/pkg/apis/agones/v1/gameserver.go b/pkg/apis/agones/v1/gameserver.go index 4d791c224b..c300a3bd55 100644 --- a/pkg/apis/agones/v1/gameserver.go +++ b/pkg/apis/agones/v1/gameserver.go @@ -929,9 +929,11 @@ func (gs *GameServer) Patch(delta *GameServer) ([]byte, error) { // UpdateCount increments or decrements a CounterStatus on a Game Server by the given amount. func (gs *GameServer) UpdateCount(name string, action string, amount int64) error { - err := fmt.Errorf("unable to UpdateCount: Name %s, Action %s, Amount %d", name, action, amount) - if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) || amount < 0 { - return err + if !(action == GameServerAllocationIncrement || action == GameServerAllocationDecrement) { + return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Allocation action must be one of %s or %s", name, action, amount, GameServerAllocationIncrement, GameServerAllocationDecrement) + } + if amount < 0 { + return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Amount must be greater than 0", name, action, amount) } if counter, ok := gs.Status.Counters[name]; ok { cnt := counter.Count @@ -939,82 +941,87 @@ func (gs *GameServer) UpdateCount(name string, action string, amount int64) erro cnt += amount // only check for Count > Capacity when incrementing if cnt > counter.Capacity { - return err + return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Incremented Count %d is greater than available capacity %d", name, action, amount, cnt, counter.Capacity) } - } - if action == GameServerAllocationDecrement { + } else { cnt -= amount - } - if cnt < 0 { - return err + // only check for Count < 0 when decrementing + if cnt < 0 { + return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Decremented Count %d is less than 0", name, action, amount, cnt) + } } counter.Count = cnt gs.Status.Counters[name] = counter return nil } - return err + return errors.Errorf("unable to UpdateCount with Name %s, Action %s, Amount %d. Counter not found in GameServer %s", name, action, amount, gs.ObjectMeta.GetName()) } // UpdateCounterCapacity updates the CounterStatus Capacity to the given capacity. func (gs *GameServer) UpdateCounterCapacity(name string, capacity int64) error { - err := fmt.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d", name, capacity) if capacity < 0 { - return err + return errors.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d. Capacity must be greater than or equal to 0", name, capacity) } if counter, ok := gs.Status.Counters[name]; ok { counter.Capacity = capacity gs.Status.Counters[name] = counter return nil } - return err + return errors.Errorf("unable to UpdateCounterCapacity: Name %s, Capacity %d. Counter not found in GameServer %s", name, capacity, gs.ObjectMeta.GetName()) } // UpdateListCapacity updates the ListStatus Capacity to the given capacity. func (gs *GameServer) UpdateListCapacity(name string, capacity int64) error { - err := fmt.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d", name, capacity) if capacity < 0 || capacity > 1000 { - return err + return errors.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d. Capacity must be between 0 and 1000, inclusive", name, capacity) } if list, ok := gs.Status.Lists[name]; ok { list.Capacity = capacity gs.Status.Lists[name] = list return nil } - return err + return errors.Errorf("unable to UpdateListCapacity: Name %s, Capacity %d. List not found in GameServer %s", name, capacity, gs.ObjectMeta.GetName()) } -// AppendListValues adds given the values to the ListStatus Values list. Any duplicates are silently -// dropped. +// AppendListValues adds unique values to the ListStatus Values list. func (gs *GameServer) AppendListValues(name string, values []string) error { - err := fmt.Errorf("unable to AppendListValues: Name %s, Values %s", name, values) if len(values) == 0 { - return err + return errors.Errorf("unable to AppendListValues: Name %s, Values %s. No values to append", name, values) } if list, ok := gs.Status.Lists[name]; ok { - vals := []string{} - // Maintain ordering so new values are appended to the end of list.Values - vals = append(vals, list.Values...) - vals = append(vals, values...) - vals = removeDuplicates(vals) - if (len(vals) > int(list.Capacity)) || (len(vals) == len(list.Values)) { - return err - } - list.Values = vals + mergedList := mergeRemoveDuplicates(list.Values, values) + if len(mergedList) > int(list.Capacity) { + return errors.Errorf("unable to AppendListValues: Name %s, Values %s. Appended list length %d exceeds list capacity %d", name, values, len(mergedList), list.Capacity) + } + // If all given values are duplicates we give an error warning. + if len(mergedList) == len(list.Values) { + return errors.Errorf("unable to AppendListValues: Name %s, Values %s. All appended values are duplicates of the existing list", name, values) + } + // If only some values are duplicates, those duplicate values are silently dropped. + list.Values = mergedList gs.Status.Lists[name] = list return nil } - return err + return errors.Errorf("unable to AppendListValues: Name %s, Values %s. List not found in GameServer %s", name, values, gs.ObjectMeta.GetName()) } -// removeDuplicates removes any duplicate values from a list. Returns new list with unique values only. -func removeDuplicates(values []string) []string { +// mergeRemoveDuplicates merges two lists and removes any duplicate values. +// Maintains ordering, so new values from list2 are appended to the end of list1. +// Returns a new list with unique values only. +func mergeRemoveDuplicates(list1 []string, list2 []string) []string { + uniqueList := []string{} listMap := make(map[string]bool) - uniqueValues := []string{} - for _, v := range values { - if _, ok := listMap[v]; !ok { - uniqueValues = append(uniqueValues, v) - listMap[v] = true + for _, v1 := range list1 { + if _, ok := listMap[v1]; !ok { + uniqueList = append(uniqueList, v1) + listMap[v1] = true + } + } + for _, v2 := range list2 { + if _, ok := listMap[v2]; !ok { + uniqueList = append(uniqueList, v2) + listMap[v2] = true } } - return uniqueValues + return uniqueList } diff --git a/pkg/apis/agones/v1/gameserver_test.go b/pkg/apis/agones/v1/gameserver_test.go index 0f3fedcb9d..a6cec05d15 100644 --- a/pkg/apis/agones/v1/gameserver_test.go +++ b/pkg/apis/agones/v1/gameserver_test.go @@ -2050,34 +2050,39 @@ func TestGameServerAppendListValues(t *testing.T) { } } -func TestRemoveDuplicates(t *testing.T) { +func TestMergeRemoveDuplicates(t *testing.T) { t.Parallel() testCases := map[string]struct { - values []string - want []string + str1 []string + str2 []string + want []string }{ - "empty string array": { - values: []string{}, - want: []string{}, + "empty string arrays": { + str1: []string{}, + str2: []string{}, + want: []string{}, }, "no duplicates": { - values: []string{"one", "two", "three"}, - want: []string{"one", "two", "three"}, + str1: []string{"one"}, + str2: []string{"two", "three"}, + want: []string{"one", "two", "three"}, }, "remove one duplicate": { - values: []string{"one", "one", "one"}, - want: []string{"one"}, + str1: []string{"one", "one", "one"}, + str2: []string{"one", "one", "one"}, + want: []string{"one"}, }, "remove multiple duplicates": { - values: []string{"one", "two", "one", "one", "two"}, - want: []string{"one", "two"}, + str1: []string{"one", "two"}, + str2: []string{"two", "one"}, + want: []string{"one", "two"}, }, } for test, testCase := range testCases { t.Run(test, func(t *testing.T) { - got := removeDuplicates(testCase.values) + got := mergeRemoveDuplicates(testCase.str1, testCase.str2) assert.Equal(t, testCase.want, got) }) } diff --git a/pkg/apis/allocation/v1/gameserverallocation.go b/pkg/apis/allocation/v1/gameserverallocation.go index 279ad2b789..04ca4d294d 100644 --- a/pkg/apis/allocation/v1/gameserverallocation.go +++ b/pkg/apis/allocation/v1/gameserverallocation.go @@ -15,6 +15,7 @@ package v1 import ( + "errors" "fmt" "agones.dev/agones/pkg/apis" @@ -295,41 +296,41 @@ func (s *GameServerSelector) matchCounters(gs *agonesv1.GameServer) bool { } // CounterActions attempts to peform any actions from the CounterAction on the GameServer Counter. -// Returns a string list of any actions that could not be performed. -func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) []string { - errors := []string{} +// Returns the errors of any actions that could not be performed. +func (ca *CounterAction) CounterActions(counter string, gs *agonesv1.GameServer) error { + var errs error if ca.Capacity != nil { capErr := gs.UpdateCounterCapacity(counter, *ca.Capacity) if capErr != nil { - errors = append(errors, capErr.Error()) + errs = errors.Join(errs, capErr) } } if ca.Action != nil && ca.Amount != nil { cntErr := gs.UpdateCount(counter, *ca.Action, *ca.Amount) if cntErr != nil { - errors = append(errors, cntErr.Error()) + errs = errors.Join(errs, cntErr) } } - return errors + return errs } // ListActions attempts to peform any actions from the ListAction on the GameServer List. // Returns a string list of any actions that could not be performed. -func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) []string { - errors := []string{} +func (la *ListAction) ListActions(list string, gs *agonesv1.GameServer) error { + var errs error if la.Capacity != nil { capErr := gs.UpdateListCapacity(list, *la.Capacity) if capErr != nil { - errors = append(errors, capErr.Error()) + errs = errors.Join(errs, capErr) } } if la.AddValues != nil && len(la.AddValues) > 0 { cntErr := gs.AppendListValues(list, la.AddValues) if cntErr != nil { - errors = append(errors, cntErr.Error()) + errs = errors.Join(errs, cntErr) } } - return errors + return errs } // matchLists returns true if there is a match for the ListSelector in the GameServerStatus diff --git a/pkg/apis/allocation/v1/gameserverallocation_test.go b/pkg/apis/allocation/v1/gameserverallocation_test.go index 8a0d81d93c..fdf754917f 100644 --- a/pkg/apis/allocation/v1/gameserverallocation_test.go +++ b/pkg/apis/allocation/v1/gameserverallocation_test.go @@ -1032,11 +1032,11 @@ func TestGameServerCounterActions(t *testing.T) { INCREMENT := "Increment" testScenarios := map[string]struct { - ca CounterAction - counter string - gs *agonesv1.GameServer - want *agonesv1.GameServer - errLength int + ca CounterAction + counter string + gs *agonesv1.GameServer + want *agonesv1.GameServer + wantErr bool }{ "update counter capacity": { ca: CounterAction{ @@ -1055,7 +1055,7 @@ func TestGameServerCounterActions(t *testing.T) { Count: 1, Capacity: 0, }}}}, - errLength: 0, + wantErr: false, }, "fail update counter capacity and count": { ca: CounterAction{ @@ -1076,7 +1076,7 @@ func TestGameServerCounterActions(t *testing.T) { Count: 99, Capacity: 100, }}}}, - errLength: 2, + wantErr: true, }, "update counter count": { ca: CounterAction{ @@ -1096,7 +1096,7 @@ func TestGameServerCounterActions(t *testing.T) { Count: 11, Capacity: 100, }}}}, - errLength: 0, + wantErr: false, }, "update counter count and capacity": { ca: CounterAction{ @@ -1117,15 +1117,19 @@ func TestGameServerCounterActions(t *testing.T) { Count: 1, Capacity: 10, }}}}, - errLength: 0, + wantErr: false, }, } for test, testScenario := range testScenarios { t.Run(test, func(t *testing.T) { errs := testScenario.ca.CounterActions(testScenario.counter, testScenario.gs) + if errs != nil { + assert.True(t, testScenario.wantErr) + } else { + assert.False(t, testScenario.wantErr) + } assert.Equal(t, testScenario.want, testScenario.gs) - assert.Equal(t, testScenario.errLength, len(errs)) }) } } @@ -1138,11 +1142,11 @@ func TestGameServerListActions(t *testing.T) { assert.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true", runtime.FeatureCountsAndLists))) testScenarios := map[string]struct { - la ListAction - list string - gs *agonesv1.GameServer - want *agonesv1.GameServer - errLength int + la ListAction + list string + gs *agonesv1.GameServer + want *agonesv1.GameServer + wantErr bool }{ "update list capacity": { la: ListAction{ @@ -1161,7 +1165,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"page1", "page2"}, Capacity: 0, }}}}, - errLength: 0, + wantErr: false, }, "update list values": { la: ListAction{ @@ -1180,7 +1184,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"sage1", "sage2", "sage3"}, Capacity: 100, }}}}, - errLength: 0, + wantErr: false, }, "update list values and capacity": { la: ListAction{ @@ -1200,7 +1204,7 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"magician1", "magician2", "magician3"}, Capacity: 42, }}}}, - errLength: 0, + wantErr: false, }, "update list values and capacity - value add fails": { la: ListAction{ @@ -1220,15 +1224,19 @@ func TestGameServerListActions(t *testing.T) { Values: []string{"fairy1", "fairy2"}, Capacity: 2, }}}}, - errLength: 1, + wantErr: true, }, } for test, testScenario := range testScenarios { t.Run(test, func(t *testing.T) { errs := testScenario.la.ListActions(testScenario.list, testScenario.gs) + if errs != nil { + assert.True(t, testScenario.wantErr) + } else { + assert.False(t, testScenario.wantErr) + } assert.Equal(t, testScenario.want, testScenario.gs) - assert.Equal(t, testScenario.errLength, len(errs)) }) } } diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 69895f384d..7b906974f1 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/pkg/gameserverallocations/allocator.go @@ -18,6 +18,7 @@ import ( "context" "crypto/tls" "crypto/x509" + goErrors "errors" "fmt" "net/http" "strings" @@ -609,22 +610,24 @@ func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocati // perfom any Counter or List actions if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { - actionErrors := []string{} + var counterErrors error + var listErrors error if gsa.Spec.Counters != nil { for counter, ca := range gsa.Spec.Counters { - errs := ca.CounterActions(counter, gs) - actionErrors = append(actionErrors, errs...) + counterErrors = goErrors.Join(counterErrors, ca.CounterActions(counter, gs)) } } if gsa.Spec.Lists != nil { for list, la := range gsa.Spec.Lists { - errs := la.ListActions(list, gs) - actionErrors = append(actionErrors, errs...) + listErrors = goErrors.Join(listErrors, la.ListActions(list, gs)) } } // record any Counter or List action errors as a warning - if len(actionErrors) != 0 { - c.recorder.Event(gs, corev1.EventTypeWarning, string(runtime.FeatureCountsAndLists), fmt.Sprint(actionErrors)) + if counterErrors != nil { + c.recorder.Event(gs, corev1.EventTypeWarning, "Counter Errors", counterErrors.Error()) + } + if listErrors != nil { + c.recorder.Event(gs, corev1.EventTypeWarning, "List Errors", listErrors.Error()) } } From 6296d8f5bc34d749d3dcf0f295b5c2431cc38638 Mon Sep 17 00:00:00 2001 From: Ivy Gooch Date: Fri, 5 May 2023 19:12:01 +0000 Subject: [PATCH 7/7] Move event recording after successful game server update --- pkg/gameserverallocations/allocator.go | 28 ++++++++++++++++---------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 7b906974f1..58b8703a35 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/pkg/gameserverallocations/allocator.go @@ -565,7 +565,6 @@ func (c *Allocator) allocationUpdateWorkers(ctx context.Context, workerCount int res.err = errors.Wrap(err, "error updating allocated gameserver") } else { res.gs = gs - c.recorder.Event(res.gs, corev1.EventTypeNormal, string(res.gs.Status.State), "Allocated") } res.request.response <- res @@ -609,9 +608,9 @@ func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocati gs.Status.State = agonesv1.GameServerStateAllocated // perfom any Counter or List actions + var counterErrors error + var listErrors error if runtime.FeatureEnabled(runtime.FeatureCountsAndLists) { - var counterErrors error - var listErrors error if gsa.Spec.Counters != nil { for counter, ca := range gsa.Spec.Counters { counterErrors = goErrors.Join(counterErrors, ca.CounterActions(counter, gs)) @@ -622,16 +621,23 @@ func (c *Allocator) applyAllocationToGameServer(ctx context.Context, mp allocati listErrors = goErrors.Join(listErrors, la.ListActions(list, gs)) } } - // record any Counter or List action errors as a warning - if counterErrors != nil { - c.recorder.Event(gs, corev1.EventTypeWarning, "Counter Errors", counterErrors.Error()) - } - if listErrors != nil { - c.recorder.Event(gs, corev1.EventTypeWarning, "List Errors", listErrors.Error()) - } } - return c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gs, metav1.UpdateOptions{}) + gsUpdate, updateErr := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gs, metav1.UpdateOptions{}) + if updateErr != nil { + return gsUpdate, updateErr + } + + // If successful Update record any Counter or List action errors as a warning + if counterErrors != nil { + c.recorder.Event(gsUpdate, corev1.EventTypeWarning, "CounterActionError", counterErrors.Error()) + } + if listErrors != nil { + c.recorder.Event(gsUpdate, corev1.EventTypeWarning, "ListActionError", listErrors.Error()) + } + c.recorder.Event(gsUpdate, corev1.EventTypeNormal, string(gsUpdate.Status.State), "Allocated") + + return gsUpdate, updateErr } // Retry retries fn based on backoff provided.