From 9dbb6bc6cf2fab5067c4c77c0a89539fe8abe207 Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Wed, 20 May 2020 01:31:29 +0300 Subject: [PATCH] Fix Local SDK nil Players with test (#1572) * Fix Local SDK nil Players with test There was a situation when additional call in testMode leads to SegFault. --- build/includes/sdk.mk | 3 +- pkg/sdkserver/localsdk.go | 17 +- pkg/sdkserver/localsdk_test.go | 297 +++++++++++++++++++++------------ 3 files changed, 210 insertions(+), 107 deletions(-) diff --git a/build/includes/sdk.mk b/build/includes/sdk.mk index db078a6296..95d31005e2 100644 --- a/build/includes/sdk.mk +++ b/build/includes/sdk.mk @@ -126,9 +126,10 @@ ensure-build-sdk-image: # SDK client test against it. Useful for test development run-sdk-conformance-local: TIMEOUT ?= 30 run-sdk-conformance-local: TESTS ?= ready,allocate,setlabel,setannotation,gameserver,health,shutdown,watch,reserve +run-sdk-conformance-local: FEATURE_GATES ?= run-sdk-conformance-local: ensure-agones-sdk-image docker run -e "ADDRESS=" -p 9357:9357 -p 9358:9358 \ - -e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" $(sidecar_tag) + -e "TEST=$(TESTS)" -e "TIMEOUT=$(TIMEOUT)" -e "FEATURE_GATES=$(FEATURE_GATES)" $(sidecar_tag) # Run SDK conformance test, previously built, for a specific SDK_FOLDER # Sleeps the start of the sidecar to test that the SDK blocks on connection correctly diff --git a/pkg/sdkserver/localsdk.go b/pkg/sdkserver/localsdk.go index 35ca3b9c51..b8c2cac7ba 100644 --- a/pkg/sdkserver/localsdk.go +++ b/pkg/sdkserver/localsdk.go @@ -40,8 +40,10 @@ var ( _ sdk.SDKServer = &LocalSDKServer{} _ alpha.SDKServer = &LocalSDKServer{} _ beta.SDKServer = &LocalSDKServer{} +) - defaultGs = &sdk.GameServer{ +func defaultGs() *sdk.GameServer { + return &sdk.GameServer{ ObjectMeta: &sdk.GameServer_ObjectMeta{ Name: "local", Namespace: "default", @@ -66,7 +68,7 @@ var ( Ports: []*sdk.GameServer_Status_Port{{Name: "default", Port: 7777}}, }, } -) +} // LocalSDKServer type is the SDKServer implementation for when the sidecar // is being run for local development, and doesn't connect to the @@ -91,7 +93,7 @@ type LocalSDKServer struct { func NewLocalSDKServer(filePath string) (*LocalSDKServer, error) { l := &LocalSDKServer{ gsMutex: sync.RWMutex{}, - gs: defaultGs, + gs: defaultGs(), update: make(chan struct{}), updateObservers: sync.Map{}, testMutex: sync.Mutex{}, @@ -134,6 +136,9 @@ func NewLocalSDKServer(filePath string) (*LocalSDKServer, error) { l.logger.WithError(err).WithField("filePath", filePath).Error("error adding watcher") } } + if runtime.FeatureEnabled(runtime.FeaturePlayerTracking) && l.gs.Status.Players == nil { + l.gs.Status.Players = &sdk.GameServer_Status_PlayerStatus{} + } go func() { for value := range l.update { @@ -197,8 +202,14 @@ func (l *LocalSDKServer) recordRequestWithValue(request string, value string, ob case "UID": fieldVal = l.gs.ObjectMeta.Uid case "PlayerCapacity": + if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { + return + } fieldVal = strconv.FormatInt(l.gs.Status.Players.Capacity, 10) case "PlayerIDs": + if !runtime.FeatureEnabled(runtime.FeaturePlayerTracking) { + return + } fieldVal = strings.Join(l.gs.Status.Players.Ids, ",") default: l.logger.Error("unexpected Field to compare") diff --git a/pkg/sdkserver/localsdk_test.go b/pkg/sdkserver/localsdk_test.go index 3d0ecf9dcc..e33729fa0f 100644 --- a/pkg/sdkserver/localsdk_test.go +++ b/pkg/sdkserver/localsdk_test.go @@ -20,6 +20,7 @@ import ( "fmt" "io/ioutil" "os" + "strconv" "sync" "testing" "time" @@ -63,7 +64,11 @@ func TestLocal(t *testing.T) { gs, err := l.GetGameServer(ctx, e) assert.Nil(t, err) - assert.Equal(t, defaultGs, gs) + assert.Equal(t, defaultGs().GetObjectMeta(), gs.GetObjectMeta()) + assert.Equal(t, defaultGs().GetSpec(), gs.GetSpec()) + gsStatus := defaultGs().GetStatus() + gsStatus.State = "Shutdown" + assert.Equal(t, gsStatus, gs.GetStatus()) } func TestLocalSDKWithTestMode(t *testing.T) { @@ -328,125 +333,191 @@ func TestLocalSDKServerPlayerConnectAndDisconnect(t *testing.T) { defer runtime.FeatureTestMutex.Unlock() assert.NoError(t, runtime.ParseFeatures(string(runtime.FeaturePlayerTracking)+"=true")) - fixture := &agonesv1.GameServer{ - ObjectMeta: metav1.ObjectMeta{Name: "stuff"}, - Status: agonesv1.GameServerStatus{ - Players: &agonesv1.PlayerStatus{ - Capacity: 1, - }, - }, + gs := func() *agonesv1.GameServer { + return &agonesv1.GameServer{ + ObjectMeta: metav1.ObjectMeta{Name: "stuff"}, + Status: agonesv1.GameServerStatus{ + Players: &agonesv1.PlayerStatus{ + Capacity: 1, + }, + }} } e := &alpha.Empty{} - path, err := gsToTmpFile(fixture) - assert.NoError(t, err) - l, err := NewLocalSDKServer(path) - assert.Nil(t, err) - - stream := newGameServerMockStream() - go func() { - err := l.WatchGameServer(&sdk.Empty{}, stream) - assert.Nil(t, err) - }() - // wait for watching to begin - err = wait.Poll(time.Second, 10*time.Second, func() (bool, error) { - found := false - l.updateObservers.Range(func(_, _ interface{}) bool { - found = true - return false - }) - return found, nil - }) - assert.NoError(t, err) - - count, err := l.GetPlayerCount(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, int64(0), count.Count) - - list, err := l.GetConnectedPlayers(context.Background(), e) - assert.NoError(t, err) - assert.Empty(t, list.List) + // nolint: maligned + fixtures := map[string]struct { + testMode bool + gs *agonesv1.GameServer + useFile bool + }{ + "test mode on, gs with Status.Players": { + testMode: true, + gs: gs(), + useFile: true, + }, + "test mode off, gs with Status.Players": { + testMode: false, + gs: gs(), + useFile: true, + }, + "test mode on, gs without Status.Players": { + testMode: true, + useFile: true, + }, + "test mode off, gs without Status.Players": { + testMode: false, + useFile: true, + }, + "test mode on, no filePath": { + testMode: true, + useFile: false, + }, + "test mode off, no filePath": { + testMode: false, + useFile: false, + }, + } - id := &alpha.PlayerID{PlayerID: "one"} - // connect a player - ok, err := l.PlayerConnect(context.Background(), id) - assert.NoError(t, err) - assert.True(t, ok.Bool, "Player should not exist yet") + for k, v := range fixtures { + t.Run(k, func(t *testing.T) { + t.Parallel() + + var l *LocalSDKServer + var err error + if v.useFile { + path, pathErr := gsToTmpFile(v.gs) + assert.NoError(t, pathErr) + l, err = NewLocalSDKServer(path) + } else { + l, err = NewLocalSDKServer("") + } + assert.Nil(t, err) + l.SetTestMode(v.testMode) - count, err = l.GetPlayerCount(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, int64(1), count.Count) + stream := newGameServerMockStream() + go func() { + err := l.WatchGameServer(&sdk.Empty{}, stream) + assert.Nil(t, err) + }() - expected := &sdk.GameServer_Status_PlayerStatus{ - Count: 1, - Capacity: 1, - Ids: []string{id.PlayerID}, - } - assertWatchUpdate(t, stream, expected, func(gs *sdk.GameServer) interface{} { - return gs.Status.Players - }) + // wait for watching to begin + err = wait.Poll(time.Second, 10*time.Second, func() (bool, error) { + found := false + l.updateObservers.Range(func(_, _ interface{}) bool { + found = true + return false + }) + return found, nil + }) + assert.NoError(t, err) - ok, err = l.IsPlayerConnected(context.Background(), id) - assert.NoError(t, err) - assert.True(t, ok.Bool, "player should be connected") + if !v.useFile || v.gs == nil { + _, err := l.SetPlayerCapacity(context.Background(), &alpha.Count{ + Count: 1, + }) + assert.NoError(t, err) + expected := &sdk.GameServer_Status_PlayerStatus{ + Capacity: 1, + } + assertWatchUpdate(t, stream, expected, func(gs *sdk.GameServer) interface{} { + return gs.Status.Players + }) + } - list, err = l.GetConnectedPlayers(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, []string{id.PlayerID}, list.List) + id := &alpha.PlayerID{PlayerID: "one"} + ok, err := l.IsPlayerConnected(context.Background(), id) + assert.NoError(t, err) + assert.False(t, ok.Bool, "player should not be connected") - // add same player - ok, err = l.PlayerConnect(context.Background(), id) - assert.NoError(t, err) - assert.False(t, ok.Bool, "Player already exists") + count, err := l.GetPlayerCount(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, int64(0), count.Count) - count, err = l.GetPlayerCount(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, int64(1), count.Count) - assertNoWatchUpdate(t, stream) + list, err := l.GetConnectedPlayers(context.Background(), e) + assert.NoError(t, err) + assert.Empty(t, list.List) - list, err = l.GetConnectedPlayers(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, []string{id.PlayerID}, list.List) + // connect a player + ok, err = l.PlayerConnect(context.Background(), id) + assert.NoError(t, err) + assert.True(t, ok.Bool, "Player should not exist yet") - // should return an error if we try to add another, since we're at capacity - nopePlayer := &alpha.PlayerID{PlayerID: "nope"} - _, err = l.PlayerConnect(context.Background(), nopePlayer) - assert.EqualError(t, err, "Players are already at capacity") + count, err = l.GetPlayerCount(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, int64(1), count.Count) - ok, err = l.IsPlayerConnected(context.Background(), nopePlayer) - assert.NoError(t, err) - assert.False(t, ok.Bool) + expected := &sdk.GameServer_Status_PlayerStatus{ + Count: 1, + Capacity: 1, + Ids: []string{id.PlayerID}, + } + assertWatchUpdate(t, stream, expected, func(gs *sdk.GameServer) interface{} { + return gs.Status.Players + }) - // disconnect a player - ok, err = l.PlayerDisconnect(context.Background(), id) - assert.NoError(t, err) - assert.True(t, ok.Bool, "Player should be removed") - count, err = l.GetPlayerCount(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, int64(0), count.Count) + ok, err = l.IsPlayerConnected(context.Background(), id) + assert.NoError(t, err) + assert.True(t, ok.Bool, "player should be connected") + + list, err = l.GetConnectedPlayers(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, []string{id.PlayerID}, list.List) + + // add same player + ok, err = l.PlayerConnect(context.Background(), id) + assert.NoError(t, err) + assert.False(t, ok.Bool, "Player already exists") + + count, err = l.GetPlayerCount(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, int64(1), count.Count) + assertNoWatchUpdate(t, stream) + + list, err = l.GetConnectedPlayers(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, []string{id.PlayerID}, list.List) + + // should return an error if we try to add another, since we're at capacity + nopePlayer := &alpha.PlayerID{PlayerID: "nope"} + _, err = l.PlayerConnect(context.Background(), nopePlayer) + assert.EqualError(t, err, "Players are already at capacity") + + ok, err = l.IsPlayerConnected(context.Background(), nopePlayer) + assert.NoError(t, err) + assert.False(t, ok.Bool) + + // disconnect a player + ok, err = l.PlayerDisconnect(context.Background(), id) + assert.NoError(t, err) + assert.True(t, ok.Bool, "Player should be removed") + count, err = l.GetPlayerCount(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, int64(0), count.Count) + + expected = &sdk.GameServer_Status_PlayerStatus{ + Count: 0, + Capacity: 1, + Ids: []string{}, + } + assertWatchUpdate(t, stream, expected, func(gs *sdk.GameServer) interface{} { + return gs.Status.Players + }) - expected = &sdk.GameServer_Status_PlayerStatus{ - Count: 0, - Capacity: 1, - Ids: []string{}, + list, err = l.GetConnectedPlayers(context.Background(), e) + assert.NoError(t, err) + assert.Empty(t, list.List) + + // remove same player + ok, err = l.PlayerDisconnect(context.Background(), id) + assert.NoError(t, err) + assert.False(t, ok.Bool, "Player already be gone") + count, err = l.GetPlayerCount(context.Background(), e) + assert.NoError(t, err) + assert.Equal(t, int64(0), count.Count) + assertNoWatchUpdate(t, stream) + }) } - assertWatchUpdate(t, stream, expected, func(gs *sdk.GameServer) interface{} { - return gs.Status.Players - }) - - list, err = l.GetConnectedPlayers(context.Background(), e) - assert.NoError(t, err) - assert.Empty(t, list.List) - - // remove same player - ok, err = l.PlayerDisconnect(context.Background(), id) - assert.NoError(t, err) - assert.False(t, ok.Bool, "Player already be gone") - count, err = l.GetPlayerCount(context.Background(), e) - assert.NoError(t, err) - assert.Equal(t, int64(0), count.Count) - assertNoWatchUpdate(t, stream) } // TestLocalSDKServerStateUpdates verify that SDK functions changes the state of the @@ -524,6 +595,26 @@ func TestSDKConformanceFunctionality(t *testing.T) { assert.True(t, b, "we should receive strings from all go routines %v %v", l.expectedSequence, l.requestSequence) } +func TestAlphaSDKConformanceFunctionality(t *testing.T) { + t.Parallel() + lStable, err := NewLocalSDKServer("") + assert.Nil(t, err) + v := int64(0) + lStable.recordRequestWithValue("setplayercapacity", strconv.FormatInt(v, 10), "PlayerCapacity") + lStable.recordRequestWithValue("isplayerconnected", "", "PlayerIDs") + + runtime.FeatureTestMutex.Lock() + defer runtime.FeatureTestMutex.Unlock() + + assert.NoError(t, runtime.ParseFeatures(string(runtime.FeaturePlayerTracking)+"=true")) + l, err := NewLocalSDKServer("") + assert.Nil(t, err) + l.testMode = true + l.recordRequestWithValue("setplayercapacity", strconv.FormatInt(v, 10), "PlayerCapacity") + l.recordRequestWithValue("isplayerconnected", "", "PlayerIDs") + +} + func gsToTmpFile(gs *agonesv1.GameServer) (string, error) { file, err := ioutil.TempFile(os.TempDir(), "gameserver-") if err != nil {