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()) } }