Don't move Dev GameServer back to Ready always (#2545)

* 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

* Review updates.

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 {

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{

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,58 @@ 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 = 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)
 
-	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)
+
+	// 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) {