diff --git a/pkg/sdkserver/sdkserver.go b/pkg/sdkserver/sdkserver.go index d71b23cdcd..40f1bea133 100644 --- a/pkg/sdkserver/sdkserver.go +++ b/pkg/sdkserver/sdkserver.go @@ -1055,14 +1055,14 @@ func (s *SDKServer) UpdateList(ctx context.Context, in *alpha.UpdateListRequest) name := in.List.Name s.logger.WithField("name", name).Debug("Update List -- Currently only used for Updating Capacity") - s.gsUpdateMutex.Lock() - defer s.gsUpdateMutex.Unlock() - gs, err := s.gameServer() if err != nil { return nil, err } + s.gsUpdateMutex.Lock() + defer s.gsUpdateMutex.Unlock() + // TODO: Pull in variable Max Capacity from CRD instead of hard-coded number here. if in.List.Capacity < 0 || in.List.Capacity > 1000 { return nil, errors.Errorf("out of range. Capacity must be within range [0,1000]. Found Capacity: %d", in.List.Capacity) @@ -1091,14 +1091,14 @@ func (s *SDKServer) AddListValue(ctx context.Context, in *alpha.AddListValueRequ } s.logger.WithField("name", in.Name).Debug("Add List Value") - s.gsUpdateMutex.Lock() - defer s.gsUpdateMutex.Unlock() - gs, err := s.gameServer() if err != nil { return nil, err } + s.gsUpdateMutex.Lock() + defer s.gsUpdateMutex.Unlock() + if list, ok := gs.Status.Lists[in.Name]; ok { batchList := s.gsListUpdates[in.Name] // Verify room to add another value @@ -1147,14 +1147,14 @@ func (s *SDKServer) RemoveListValue(ctx context.Context, in *alpha.RemoveListVal s.logger.WithField("name", in.Name).Debug("Remove List Value") - s.gsUpdateMutex.Lock() - defer s.gsUpdateMutex.Unlock() - gs, err := s.gameServer() if err != nil { return nil, err } + s.gsUpdateMutex.Lock() + defer s.gsUpdateMutex.Unlock() + if list, ok := gs.Status.Lists[in.Name]; ok { // Verify value exists in the list for _, val := range list.Values { @@ -1163,6 +1163,9 @@ func (s *SDKServer) RemoveListValue(ctx context.Context, in *alpha.RemoveListVal } // Add value to remove to gsListUpdates map. batchList := s.gsListUpdates[in.Name] + if batchList.valuesToDelete == nil { + batchList.valuesToDelete = map[string]bool{} + } batchList.valuesToDelete[in.Value] = true s.gsListUpdates[in.Name] = batchList // Queue up the Update for later batch processing by updateLists. diff --git a/pkg/sdkserver/sdkserver_test.go b/pkg/sdkserver/sdkserver_test.go index a6f3d0c4ee..3f7fcd8673 100644 --- a/pkg/sdkserver/sdkserver_test.go +++ b/pkg/sdkserver/sdkserver_test.go @@ -28,9 +28,11 @@ import ( "agones.dev/agones/pkg/sdk/alpha" agtesting "agones.dev/agones/pkg/testing" agruntime "agones.dev/agones/pkg/util/runtime" + "github.com/google/go-cmp/cmp" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/fieldmaskpb" "google.golang.org/protobuf/types/known/wrapperspb" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -1320,6 +1322,409 @@ func TestSDKServerUpdateCounter(t *testing.T) { } } +func TestSDKServerAddListValue(t *testing.T) { + t.Parallel() + agruntime.FeatureTestMutex.Lock() + defer agruntime.FeatureTestMutex.Unlock() + + err := agruntime.ParseFeatures(string(agruntime.FeatureCountsAndLists) + "=true") + require.NoError(t, err, "Can not parse FeatureCountsAndLists feature") + + lists := map[string]agonesv1.ListStatus{ + "foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + } + + fixtures := map[string]struct { + listName string + request *alpha.AddListValueRequest + want agonesv1.ListStatus + updateErr bool + updated bool + }{ + "Add value": { + listName: "foo", + request: &alpha.AddListValueRequest{Name: "foo", Value: "five"}, + want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four", "five"}, Capacity: int64(100)}, + updateErr: false, + updated: true, + }, + } + + // nolint:dupl // Linter errors on lines are duplicate of TestSDKServerUpdateList, TestSDKServerRemoveListValue + for test, testCase := range fixtures { + t.Run(test, func(t *testing.T) { + m := agtesting.NewMocks() + + m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gs := agonesv1.GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "default", Generation: 1, + }, + Spec: agonesv1.GameServerSpec{ + SdkServer: agonesv1.SdkServer{ + LogLevel: "Debug", + }, + }, + Status: agonesv1.GameServerStatus{ + Lists: lists, + }, + } + gs.ApplyDefaults() + return true, &agonesv1.GameServerList{Items: []agonesv1.GameServer{gs}}, nil + }) + + updated := make(chan map[string]agonesv1.ListStatus, 10) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*agonesv1.GameServer) + gs.ObjectMeta.Generation++ + updated <- gs.Status.Lists + return true, gs, nil + }) + + ctx, cancel := context.WithCancel(context.Background()) + sc, err := defaultSidecar(m) + require.NoError(t, err) + assert.NoError(t, sc.WaitForConnection(ctx)) + sc.informerFactory.Start(ctx.Done()) + assert.True(t, cache.WaitForCacheSync(ctx.Done(), sc.gameServerSynced)) + + wg := sync.WaitGroup{} + wg.Add(1) + + go func() { + err = sc.Run(ctx) + assert.NoError(t, err) + wg.Done() + }() + + // check initial value comes through + require.Eventually(t, func() bool { + list, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 100 && err == nil + }, 10*time.Second, time.Second) + + // Update the List + _, err = sc.AddListValue(context.Background(), testCase.request) + if testCase.updateErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + assert.NoError(t, err) + assert.Equal(t, testCase.want.Values, got.Values) + assert.Equal(t, testCase.want.Capacity, got.Capacity) + + // on an update, confirm that the update hits the K8s api + if testCase.updated { + select { + case value := <-updated: + assert.NotNil(t, value[testCase.listName]) + assert.Equal(t, + agonesv1.ListStatus{Values: testCase.want.Values, Capacity: testCase.want.Capacity}, + value[testCase.listName]) + case <-time.After(10 * time.Second): + assert.Fail(t, "List should have been updated") + } + } + + cancel() + wg.Wait() + }) + } +} + +func TestSDKServerRemoveListValue(t *testing.T) { + t.Parallel() + agruntime.FeatureTestMutex.Lock() + defer agruntime.FeatureTestMutex.Unlock() + + err := agruntime.ParseFeatures(string(agruntime.FeatureCountsAndLists) + "=true") + require.NoError(t, err, "Can not parse FeatureCountsAndLists feature") + + lists := map[string]agonesv1.ListStatus{ + "foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + } + + fixtures := map[string]struct { + listName string + request *alpha.RemoveListValueRequest + want agonesv1.ListStatus + updateErr bool + updated bool + }{ + "Remove value": { + listName: "foo", + request: &alpha.RemoveListValueRequest{Name: "foo", Value: "two"}, + want: agonesv1.ListStatus{Values: []string{"one", "three", "four"}, Capacity: int64(100)}, + updateErr: false, + updated: true, + }, + } + + // nolint:dupl // Linter errors on lines are duplicate of TestSDKServerUpdateList, TestSDKServerAddListValue + for test, testCase := range fixtures { + t.Run(test, func(t *testing.T) { + m := agtesting.NewMocks() + + m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gs := agonesv1.GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "default", Generation: 1, + }, + Spec: agonesv1.GameServerSpec{ + SdkServer: agonesv1.SdkServer{ + LogLevel: "Debug", + }, + }, + Status: agonesv1.GameServerStatus{ + Lists: lists, + }, + } + gs.ApplyDefaults() + return true, &agonesv1.GameServerList{Items: []agonesv1.GameServer{gs}}, nil + }) + + updated := make(chan map[string]agonesv1.ListStatus, 10) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*agonesv1.GameServer) + gs.ObjectMeta.Generation++ + updated <- gs.Status.Lists + return true, gs, nil + }) + + ctx, cancel := context.WithCancel(context.Background()) + sc, err := defaultSidecar(m) + require.NoError(t, err) + assert.NoError(t, sc.WaitForConnection(ctx)) + sc.informerFactory.Start(ctx.Done()) + assert.True(t, cache.WaitForCacheSync(ctx.Done(), sc.gameServerSynced)) + + wg := sync.WaitGroup{} + wg.Add(1) + + go func() { + err = sc.Run(ctx) + assert.NoError(t, err) + wg.Done() + }() + + // check initial value comes through + require.Eventually(t, func() bool { + list, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 100 && err == nil + }, 10*time.Second, time.Second) + + // Update the List + _, err = sc.RemoveListValue(context.Background(), testCase.request) + if testCase.updateErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + assert.NoError(t, err) + assert.Equal(t, testCase.want.Values, got.Values) + assert.Equal(t, testCase.want.Capacity, got.Capacity) + + // on an update, confirm that the update hits the K8s api + if testCase.updated { + select { + case value := <-updated: + assert.NotNil(t, value[testCase.listName]) + assert.Equal(t, + agonesv1.ListStatus{Values: testCase.want.Values, Capacity: testCase.want.Capacity}, + value[testCase.listName]) + case <-time.After(10 * time.Second): + assert.Fail(t, "List should have been updated") + } + } + + cancel() + wg.Wait() + }) + } +} + +func TestSDKServerUpdateList(t *testing.T) { + t.Parallel() + agruntime.FeatureTestMutex.Lock() + defer agruntime.FeatureTestMutex.Unlock() + + err := agruntime.ParseFeatures(string(agruntime.FeatureCountsAndLists) + "=true") + require.NoError(t, err, "Can not parse FeatureCountsAndLists feature") + + lists := map[string]agonesv1.ListStatus{ + "foo": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + "bar": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + "baz": {Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + } + + fixtures := map[string]struct { + listName string + request *alpha.UpdateListRequest + want agonesv1.ListStatus + updateErr bool + updated bool + }{ + "set capacity to max": { + listName: "foo", + request: &alpha.UpdateListRequest{ + List: &alpha.List{ + Name: "foo", + Capacity: int64(1000), + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity"}}, + }, + want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four"}, Capacity: int64(1000)}, + updateErr: false, + updated: true, + }, + "set capacity to min values are truncated": { + listName: "bar", + request: &alpha.UpdateListRequest{ + List: &alpha.List{ + Name: "bar", + Capacity: int64(0), + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity"}}, + }, + want: agonesv1.ListStatus{Values: []string{}, Capacity: int64(0)}, + updateErr: false, + updated: true, + }, + "set capacity past max": { + listName: "baz", + request: &alpha.UpdateListRequest{ + List: &alpha.List{ + Name: "baz", + Capacity: int64(1001), + }, + UpdateMask: &fieldmaskpb.FieldMask{Paths: []string{"capacity"}}, + }, + want: agonesv1.ListStatus{Values: []string{"one", "two", "three", "four"}, Capacity: int64(100)}, + updateErr: true, + updated: false, + }, + } + + // nolint:dupl // Linter errors on lines are duplicate of TestSDKServerAddListValue, TestSDKServerRemoveListValue + for test, testCase := range fixtures { + t.Run(test, func(t *testing.T) { + m := agtesting.NewMocks() + + m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gs := agonesv1.GameServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", Namespace: "default", Generation: 1, + }, + Spec: agonesv1.GameServerSpec{ + SdkServer: agonesv1.SdkServer{ + LogLevel: "Debug", + }, + }, + Status: agonesv1.GameServerStatus{ + Lists: lists, + }, + } + gs.ApplyDefaults() + return true, &agonesv1.GameServerList{Items: []agonesv1.GameServer{gs}}, nil + }) + + updated := make(chan map[string]agonesv1.ListStatus, 10) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*agonesv1.GameServer) + gs.ObjectMeta.Generation++ + updated <- gs.Status.Lists + return true, gs, nil + }) + + ctx, cancel := context.WithCancel(context.Background()) + sc, err := defaultSidecar(m) + require.NoError(t, err) + assert.NoError(t, sc.WaitForConnection(ctx)) + sc.informerFactory.Start(ctx.Done()) + assert.True(t, cache.WaitForCacheSync(ctx.Done(), sc.gameServerSynced)) + + wg := sync.WaitGroup{} + wg.Add(1) + + go func() { + err = sc.Run(ctx) + assert.NoError(t, err) + wg.Done() + }() + + // check initial value comes through + require.Eventually(t, func() bool { + list, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + return cmp.Equal(list.Values, []string{"one", "two", "three", "four"}) && list.Capacity == 100 && err == nil + }, 10*time.Second, time.Second) + + // Update the List + _, err = sc.UpdateList(context.Background(), testCase.request) + if testCase.updateErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + + got, err := sc.GetList(context.Background(), &alpha.GetListRequest{Name: testCase.listName}) + assert.NoError(t, err) + assert.Equal(t, testCase.want.Values, got.Values) + assert.Equal(t, testCase.want.Capacity, got.Capacity) + + // on an update, confirm that the update hits the K8s api + if testCase.updated { + select { + case value := <-updated: + assert.NotNil(t, value[testCase.listName]) + assert.Equal(t, + agonesv1.ListStatus{Values: testCase.want.Values, Capacity: testCase.want.Capacity}, + value[testCase.listName]) + case <-time.After(10 * time.Second): + assert.Fail(t, "List should have been updated") + } + } + + cancel() + wg.Wait() + }) + } +} + +func TestDeleteValues(t *testing.T) { + t.Parallel() + + list := []string{"pDtUOSwMys", "MIaQYdeONT", "ZTwRNgZfxk", "ybtlfzfJau", "JwoYseCCyU", "JQJXhknLeG", + "KDmxroeFvi", "fguLESWvmr", "xRUFzgrtuE", "UwElufBLtA", "jAySktznPe", "JZZRLkAtpQ", "BzHLffHxLd", + "KWOyTiXsGP", "CtHFOMotCK", "SBOFIJBoBu", "gjYoIQLbAk", "krWVhxssxR", "ZTqRMKAqSx", "oDalBXZckY", + "ZxATCXhBHk", "MTwgrrHePq", "KNGxlixHYt", "taZswVczZU", "beoXmuxAHE", "VbiLLJrRVs", "GrIEuiUlkB", + "IPJhGxiKWY", "gYXZtGeFyd", "GYvKpRRsfj", "jRldDqcuEd", "ffPeeHOtMW", "AoEMlXWXVI", "HIjLrcvIqx", + "GztXdbnxqg", "zSyNSIyQbp", "lntxdkIjVt", "jOgkkkaytV", "uHMvVtWKoc", "hetOAzBePn", "KqqkCbGLjS", + "OQHRRtqIlq", "KFyHqLSACF", "nMZTcGlgAz", "iriNEjRLmh", "PRdGOtnyIo", "JDNDFYCIGi", "acalItODHz", + "HJjxJnZWEu", "dmFWypNcDY", "fokGntWpON", "tQLmmXfDNW", "ZvyARYuebj", "ipHGcRmfWt", "MpTXveRDRg", + "xPMoVLWeyj", "tXWeapJxkh", "KCMSWWiPMq", "fwsVKiWLuv", "AkKUUqwaOB", "DDlrgoWHGq", "DHScNuprJo", + "PRMEGliSBU", "kqwktsjCNb", "vDuQZIhUHp", "YoazMkShki", "IwmXsZvlcp", "CJdrVMsjiD", "xNLnNvLRMN", + "nKxDYSOkKx", "MWnrxVVOgK", "YnTHFAunKs", "DzUpkUxpuV", "kNVqCzjRxS", "IzqYWHDloX", "LvlVEniBqp", + "CmdFcgTgzM", "qmORqLRaKv", "MxMnLiGOsY", "vAiAorAIdu", "pfhhTRFcpp", "ByqwQcKJYQ", "mKaeTCghbC", + "eJssFVxVSI", "PGFMEopXax", "pYKCWZzGMf", "wIeRbiOdkf", "EKlxOXvqdF", "qOOorODUsn", "rcVUwlHOME", + "etoDkduCkv", "iqUxYYUfpz", "ALyMkpYnbY", "TwfhVKGaIE", "zWsXruOeOn", "gNEmlDWmnj", "gEvodaSjIJ", + "kOjWgLKjKE", "ATxBnODCKg", "liMbkiUTAs"} + + toDeleteMap := map[string]bool{"pDtUOSwMys": true, "beoXmuxAHE": true, "IPJhGxiKWY": true, + "gYXZtGeFyd": true, "PRMEGliSBU": true, "kqwktsjCNb": true, "mKaeTCghbC": true, + "PGFMEopXax": true, "qOOorODUsn": true, "rcVUwlHOME": true} + + newList := deleteValues(list, toDeleteMap) + assert.Equal(t, len(list)-len(toDeleteMap), len(newList)) +} + func TestSDKServerPlayerCapacity(t *testing.T) { t.Parallel() agruntime.FeatureTestMutex.Lock()