From 2df8b8e8138f367ee1913519ca5158efe8ccc4f0 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Fri, 15 Apr 2022 13:33:34 -0700 Subject: [PATCH 1/2] Don't move Dev GameServer back to Ready always There was a bug in the implementation of the [local development GameServer](https://agones.dev/site/docs/guides/local-game-server/) that would always move a `GameServer` to `Ready` any time the state was not currently `Ready`. This meant if you tried to Allocate the `GameServer`, the controller would immediately move it back to being `Ready`. This fix locks down that the only path that the controller should affect is moving from `Creating` state to `Ready`. This allows the end user to manually manage state on the `GameServer` as they desire. Closes #2536 --- pkg/gameservers/controller.go | 7 +++++-- pkg/gameservers/controller_test.go | 27 +++++++++++++++++++++++++ test/e2e/gameserver_test.go | 32 ++++++++++++++++++++++++------ 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index 82166b14d2..ef1f4474a3 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -520,10 +520,13 @@ func (c *Controller) syncDevelopmentGameServer(ctx context.Context, gs *agonesv1 return gs, nil } - if !(gs.Status.State == agonesv1.GameServerStateReady) { - c.loggerForGameServer(gs).Debug("GS is a development game server and will not be managed by Agones.") + // Only move from Creating -> Ready. Other manual state changes are up to the end user. + // We also don't want to move from Allocated -> Ready every time someone allocates a GameServer. + if !(gs.Status.State == agonesv1.GameServerStateCreating) { + return gs, nil } + c.loggerForGameServer(gs).Debug("GS is a development game server and will not be managed by Agones.") gsCopy := gs.DeepCopy() var ports []agonesv1.GameServerStatusPort for _, p := range gs.Spec.Ports { diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index 670a9e576e..eec2460f77 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -223,6 +223,33 @@ func TestControllerSyncGameServerWithDevIP(t *testing.T) { assert.Equal(t, 1, updateCount, "update reactor should fire once") }) + t.Run("Allocated GameServer", func(t *testing.T) { + c, mocks := newFakeController() + + fixture := templateDevGs.DeepCopy() + + fixture.ApplyDefaults() + fixture.Status.State = agonesv1.GameServerStateAllocated + + mocks.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gameServers := &agonesv1.GameServerList{Items: []agonesv1.GameServer{*fixture}} + return true, gameServers, nil + }) + mocks.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + require.Fail(t, "should not update") + return true, nil, nil + }) + + ctx, cancel := agtesting.StartInformers(mocks, c.gameServerSynced, c.portAllocator.nodeSynced) + defer cancel() + + err := c.portAllocator.syncAll() + require.NoError(t, err) + + err = c.syncGameServer(ctx, "default/test") + require.NoError(t, err) + }) + t.Run("When a GameServer has been deleted, the sync operation should be a noop", func(t *testing.T) { runReconcileDeleteGameServer(t, &agonesv1.GameServer{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/e2e/gameserver_test.go b/test/e2e/gameserver_test.go index f048f78f8b..e36b5d5274 100644 --- a/test/e2e/gameserver_test.go +++ b/test/e2e/gameserver_test.go @@ -27,6 +27,7 @@ import ( "time" agonesv1 "agones.dev/agones/pkg/apis/agones/v1" + allocationv1 "agones.dev/agones/pkg/apis/allocation/v1" "agones.dev/agones/pkg/util/runtime" e2eframework "agones.dev/agones/test/e2e/framework" "github.com/sirupsen/logrus" @@ -417,11 +418,14 @@ func TestGameServerUnhealthyAfterReadyCrash(t *testing.T) { func TestDevelopmentGameServerLifecycle(t *testing.T) { t.Parallel() ctx := context.Background() + + labels := map[string]string{"development": "true"} gs := &agonesv1.GameServer{ ObjectMeta: metav1.ObjectMeta{ GenerateName: "udp-server", Namespace: framework.Namespace, Annotations: map[string]string{agonesv1.DevAddressAnnotation: fakeIPAddress}, + Labels: labels, }, Spec: agonesv1.GameServerSpec{ Container: "udp-server", @@ -442,28 +446,44 @@ func TestDevelopmentGameServerLifecycle(t *testing.T) { }, }, } - readyGs, err := framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs) + readyGs, err := framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs.DeepCopy()) if err != nil { t.Fatalf("Could not get a GameServer ready: %v", err) } - - assert.Equal(t, readyGs.Status.State, agonesv1.GameServerStateReady) + require.Equal(t, readyGs.Status.State, agonesv1.GameServerStateReady) // confirm delete works, because if the finalisers don't get removed, this won't work. err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, readyGs.ObjectMeta.Name, metav1.DeleteOptions{}) - assert.NoError(t, err) + require.NoError(t, err) err = wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { _, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, readyGs.ObjectMeta.Name, metav1.GetOptions{}) - if k8serrors.IsNotFound(err) { return true, nil } return false, err }) + require.NoError(t, err) - assert.NoError(t, err) + // let's make sure we can allocate a dev gameserver + readyGs, err = framework.CreateGameServerAndWaitUntilReady(t, framework.Namespace, gs.DeepCopy()) + require.NoError(t, err) + + gsa := &allocationv1.GameServerAllocation{ + Spec: allocationv1.GameServerAllocationSpec{ + Selectors: []allocationv1.GameServerSelector{{ + LabelSelector: metav1.LabelSelector{MatchLabels: labels}, + }}, + }, + } + gsa, err = framework.AgonesClient.AllocationV1().GameServerAllocations(framework.Namespace).Create(ctx, gsa, metav1.CreateOptions{}) + require.NoError(t, err) + + require.Equal(t, readyGs.ObjectMeta.Name, gsa.Status.GameServerName) + + _, err = framework.WaitForGameServerState(t, readyGs, agonesv1.GameServerStateAllocated, time.Minute) + require.NoError(t, err) } func TestGameServerSelfAllocate(t *testing.T) { From d93c763e575fa6e3446614bdd2033e8f66866527 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Thu, 21 Apr 2022 07:50:19 -0700 Subject: [PATCH 2/2] Review updates. --- pkg/gameservers/controller.go | 2 +- test/e2e/gameserver_test.go | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index ef1f4474a3..410f8fc19f 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -522,7 +522,7 @@ func (c *Controller) syncDevelopmentGameServer(ctx context.Context, gs *agonesv1 // Only move from Creating -> Ready. Other manual state changes are up to the end user. // We also don't want to move from Allocated -> Ready every time someone allocates a GameServer. - if !(gs.Status.State == agonesv1.GameServerStateCreating) { + if gs.Status.State != agonesv1.GameServerStateCreating { return gs, nil } diff --git a/test/e2e/gameserver_test.go b/test/e2e/gameserver_test.go index e36b5d5274..825dc3b736 100644 --- a/test/e2e/gameserver_test.go +++ b/test/e2e/gameserver_test.go @@ -456,7 +456,7 @@ func TestDevelopmentGameServerLifecycle(t *testing.T) { err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, readyGs.ObjectMeta.Name, metav1.DeleteOptions{}) require.NoError(t, err) - err = wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) { + err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { _, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, readyGs.ObjectMeta.Name, metav1.GetOptions{}) if k8serrors.IsNotFound(err) { return true, nil @@ -484,6 +484,20 @@ func TestDevelopmentGameServerLifecycle(t *testing.T) { _, err = framework.WaitForGameServerState(t, readyGs, agonesv1.GameServerStateAllocated, time.Minute) require.NoError(t, err) + + // Also confirm that delete works for Allocated state, it should be fine but let's be sure. + err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Delete(ctx, readyGs.ObjectMeta.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + + err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { + _, err = framework.AgonesClient.AgonesV1().GameServers(framework.Namespace).Get(ctx, readyGs.ObjectMeta.Name, metav1.GetOptions{}) + if k8serrors.IsNotFound(err) { + return true, nil + } + + return false, err + }) + require.NoError(t, err) } func TestGameServerSelfAllocate(t *testing.T) {