Fix for GameServerAllocation

Add version annotation. Now all E2E tests pass. Some fluctuation on
SDKSetAnnotation.

pkg/apis/stable/v1alpha1/gameserver.go

@@ -189,7 +189,6 @@ func (gs *GameServer) ApplyDefaults() {
 
 	gs.applyContainerDefaults()
 	gs.applyPortDefaults()
-	gs.applyStateDefaults()
 	gs.applyHealthDefaults()
 	gs.applySchedulingDefaults()
 }
@@ -216,16 +215,6 @@ func (gs *GameServer) applyHealthDefaults() {
 	}
 }
 
-// applyStateDefaults applies state defaults
-func (gs *GameServer) applyStateDefaults() {
-	if gs.Status.State == "" {
-		gs.Status.State = GameServerStateCreating
-		if gs.HasPortPolicy(Dynamic) {
-			gs.Status.State = GameServerStatePortAllocation
-		}
-	}
-}
-
 // applyPortDefaults applies default values for all ports
 func (gs *GameServer) applyPortDefaults() {
 	for i, p := range gs.Spec.Ports {

pkg/apis/stable/v1alpha1/gameserver_test.go

@@ -202,7 +202,6 @@ func TestGameServerApplyDefaults(t *testing.T) {
 			assert.Contains(t, test.gameServer.ObjectMeta.Finalizers, stable.GroupName)
 			assert.Equal(t, test.container, spec.Container)
 			assert.Equal(t, test.expected.protocol, spec.Ports[0].Protocol)
-			assert.Equal(t, test.expected.state, test.gameServer.Status.State)
 			assert.Equal(t, test.expected.health, test.gameServer.Spec.Health)
 			assert.Equal(t, test.expected.scheduling, test.gameServer.Spec.Scheduling)
 		})

pkg/gameserverallocations/controller.go

@@ -281,6 +281,14 @@ func (c *Controller) allocate(gsa *v1alpha1.GameServerAllocation) (*v1alpha1.Gam
 	if err != nil {
 		return gs, errors.Wrapf(err, "error updating GameServer %s", gsCopy.ObjectMeta.Name)
 	}
+	if gs.Status.State != v1alpha1.GameServerStateAllocated {
+		gs.Status.State = v1alpha1.GameServerStateAllocated
+		gs, err = c.gameServerGetter.GameServers(gsCopy.ObjectMeta.Namespace).UpdateStatus(gs)
+
+		if err != nil {
+			return gs, errors.Wrapf(err, "error updating GameServer Status %s", gsCopy.ObjectMeta.Name)
+		}
+	}
 	c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Allocated")
 
 	return gs, nil

pkg/gameservers/controller.go

@@ -218,8 +218,6 @@ func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview)
 	// the rest is really just json plumbing
 	gs.ApplyDefaults()
 
-	c.logger.WithField("gs234", gs.ObjectMeta.Name).WithField("Whole review", fmt.Sprintf("%+v", review)).Infof("patch created!")
-
 	newGS, err := json.Marshal(gs)
 	if err != nil {
 		return review, errors.Wrapf(err, "error marshalling default applied GameSever %s to json", gs.ObjectMeta.Name)
@@ -320,8 +318,8 @@ func (c *Controller) Run(workers int, stop <-chan struct{}) error {
 	return nil
 }
 
-// syncGameServer synchronises the Pods for the GameServers.
-// and reacts to status changes that can occur through the client SDK
+// applyStateDefaults applies state defaults and updates the status subresource
+// accordingly
 func (c *Controller) applyStateDefaults(gs *v1alpha1.GameServer, namespace, name string) *v1alpha1.GameServer {
 	if gs.Status.State == "" {
 		var err error
@@ -334,15 +332,16 @@ func (c *Controller) applyStateDefaults(gs *v1alpha1.GameServer, namespace, name
 		if err != nil {
 			if k8serrors.IsNotFound(err) {
 				c.logger.WithField("key", name).Info("GameServer is no longer available for syncing")
+			} else {
+				c.logger.WithField("key", name).Error("Could not update to starting state")
 			}
 		}
-
-		c.logger.WithField("key", name).Error("Here we are")
 	}
 	return gs
-
 }
 
+// syncGameServer synchronises the Pods for the GameServers.
+// and reacts to status changes that can occur through the client SDK
 func (c *Controller) syncGameServer(key string) error {
 	c.logger.WithField("key", key).Info("Synchronising")
 
@@ -363,7 +362,6 @@ func (c *Controller) syncGameServer(key string) error {
 		return errors.Wrapf(err, "error retrieving GameServer %s from namespace %s", name, namespace)
 	}
 	gs = c.applyStateDefaults(gs, namespace, name)
-	//gss.UpdateStatus(gs)
 
 	if gs, err = c.syncGameServerDeletionTimestamp(gs); err != nil {
 		return err
@@ -443,20 +441,18 @@ func (c *Controller) syncGameServerPortAllocationState(gs *v1alpha1.GameServer)
 	c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Port allocated")
 
 	c.logger.WithField("gs", gsCopy).Info("Syncing Port Allocation GameServerState")
-	//gs2, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).GetStatus(gsCopy)
-	//gsCopy.ObjectMeta.ResourceVersion = gs2.ObjectMeta.ResourceVersion
 	gs, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).UpdateStatus(gsCopy)
 	if err != nil {
 		// if the GameServer doesn't get updated with the port data, then put the port
-		// back in the pool, as it will get retried on the next pass
+		// back to the pool, as it will get retried on the next pass
+		c.portAllocator.DeAllocate(gsCopy)
 		return gs, errors.Wrapf(err, "error updating status of GameServer %s to default values", gs.Name)
 	}
 	//Using ResourceVersion from updated gs
 	gs.Spec = gsCopy.Spec
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gs)
 	if err != nil {
-		// if the GameServer doesn't get updated with the port data, then put the port
-		// back in the pool, as it will get retried on the next pass
+		// put the port back to the pool, as it will get retried on the next pass
 		c.portAllocator.DeAllocate(gsCopy)
 		return gs, errors.Wrapf(err, "error updating GameServer %s to default values", gs.Name)
 	}
@@ -496,11 +492,14 @@ func (c *Controller) syncGameServerCreatingState(gs *v1alpha1.GameServer) (*v1al
 	gsCopy := gs.DeepCopy()
 	gsCopy.Status.State = v1alpha1.GameServerStateStarting
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).UpdateStatus(gsCopy)
+	if err != nil {
+		return gs, errors.Wrapf(err, "error updating GameServer Status %s to Starting state", gs.Name)
+	}
 
-	// Also update version annotation of Gameserver which resides in ObjectMeta
-	//gs2, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Get(gsCopy.Name, metav1.GetOptions{})
-	//gs2.ObjectMeta.Annotations = gsCopy.ObjectMeta.Annotations
-	//gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gs2)
+	// add SDK version annotation
+	// update version annotation of Gameserver which resides in ObjectMeta
+	gs.ObjectMeta.Annotations = gsCopy.ObjectMeta.Annotations
+	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gs)
 	if err != nil {
 		return gs, errors.Wrapf(err, "error updating GameServer %s to Starting state", gs.Name)
 	}
@@ -528,13 +527,11 @@ func (c *Controller) syncDevelopmentGameServer(gs *v1alpha1.GameServer) (*v1alph
 	for _, p := range gs.Spec.Ports {
 		ports = append(ports, p.Status())
 	}
-	// TODO: Use UpdateStatus() when it's available.
 	gsCopy.Status.State = v1alpha1.GameServerStateReady
 	gsCopy.Status.Ports = ports
 	gsCopy.Status.Address = devIPAddress
 	gsCopy.Status.NodeName = devIPAddress
 	gs, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).UpdateStatus(gsCopy)
-	// TODO was Update()
 	if err != nil {
 		return gs, errors.Wrapf(err, "error updating GameServer %s to %v status", gs.Name, gs.Status)
 	}
@@ -545,7 +542,6 @@ func (c *Controller) syncDevelopmentGameServer(gs *v1alpha1.GameServer) (*v1alph
 func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.GameServer, error) {
 	sidecar := c.sidecar(gs)
 	var pod *corev1.Pod
-	//TODO add function with annotation to GS only
 	pod, err := gs.Pod(sidecar)
 
 	// this shouldn't happen, but if it does.
@@ -677,15 +673,9 @@ func (c *Controller) syncGameServerStartingState(gs *v1alpha1.GameServer) (*v1al
 	gsCopy.Status.State = v1alpha1.GameServerStateScheduled
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).UpdateStatus(gsCopy)
 	if err != nil {
-		return gs, errors.Wrapf(err, "error updating GameServer %s to Scheduled state", gs.Name)
+		return gs, errors.Wrapf(err, "error updating GameServer Status %s to Scheduled state", gs.Name)
 	}
 
-	/*
-		gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gsCopy)
-		if err != nil {
-			return gs, errors.Wrapf(err, "error updating GameServer %s to Port state", gs.Name)
-		}
-	*/
 	c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Address and port populated")
 
 	return gs, nil
@@ -751,12 +741,6 @@ func (c *Controller) syncGameServerRequestReadyState(gs *v1alpha1.GameServer) (*
 	if err != nil {
 		return gs, errors.Wrapf(err, "error setting Ready, Port and address on GameServer %s Status", gs.ObjectMeta.Name)
 	}
-	/*
-		gs, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gsCopy)
-		if err != nil {
-			return gs, errors.Wrapf(err, "error setting Ready, Port and address on GameServer %s Status", gs.ObjectMeta.Name)
-		}
-	*/
 
 	if addressPopulated {
 		c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Address and port populated")
@@ -788,7 +772,6 @@ func (c *Controller) moveToErrorState(gs *v1alpha1.GameServer, msg string) (*v1a
 	copy.Status.State = v1alpha1.GameServerStateError
 
 	gs, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).UpdateStatus(copy)
-	//gs, err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(copy)
 	if err != nil {
 		return gs, errors.Wrapf(err, "error moving GameServer %s to Error State", gs.ObjectMeta.Name)
 	}

pkg/gameservers/controller_test.go

@@ -56,7 +56,7 @@ func TestControllerSyncGameServer(t *testing.T) {
 
 	t.Run("Creating a new GameServer", func(t *testing.T) {
 		c, mocks := newFakeController()
-		updateCount := 0
+		updateStateCount := 0
 		podCreated := false
 		fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"},
 			Spec: v1alpha1.GameServerSpec{
@@ -94,24 +94,28 @@ func TestControllerSyncGameServer(t *testing.T) {
 			gameServers := &v1alpha1.GameServerList{Items: []v1alpha1.GameServer{*fixture}}
 			return true, gameServers, nil
 		})
+		//Check only GS Status Subresource updates
 		mocks.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {
 			ua := action.(k8stesting.UpdateAction)
 			gs := ua.GetObject().(*v1alpha1.GameServer)
-			updateCount++
 			expectedState := v1alpha1.GameServerState("notastate")
-			switch updateCount {
-			case 1:
-				expectedState = v1alpha1.GameServerStateCreating
-			case 2:
-				expectedState = v1alpha1.GameServerStateStarting
-			case 3:
-				expectedState = v1alpha1.GameServerStateScheduled
-			}
-
-			assert.Equal(t, expectedState, gs.Status.State)
-			if expectedState == v1alpha1.GameServerStateScheduled {
-				assert.Equal(t, ipFixture, gs.Status.Address)
-				assert.NotEmpty(t, gs.Status.Ports[0].Port)
+			// Check only Status Subresource updates
+			if ua.GetSubresource() == "status" {
+				// Check that on last update we have set the Address
+				if updateStateCount == 2 {
+					assert.Equal(t, ipFixture, gs.Status.Address)
+					assert.NotEmpty(t, gs.Status.Ports[0].Port)
+				}
+			} else {
+				assert.Equal(t, ua.GetSubresource(), "")
+				updateStateCount++
+				switch updateStateCount {
+				case 1:
+					expectedState = v1alpha1.GameServerStateCreating
+				case 2:
+					expectedState = v1alpha1.GameServerStateStarting
+				}
+				assert.Equal(t, expectedState, gs.Status.State)
 			}
 
 			return true, gs, nil
@@ -125,7 +129,7 @@ func TestControllerSyncGameServer(t *testing.T) {
 
 		err = c.syncGameServer("default/test")
 		assert.Nil(t, err)
-		assert.Equal(t, 3, updateCount, "update reactor should fire thrice")
+		assert.Equal(t, 2, updateStateCount, "update reactor should fire thrice")
 		assert.True(t, podCreated, "pod should be created")
 	})
 
@@ -158,6 +162,7 @@ func runReconcileDeleteGameServer(t *testing.T, fixture *v1alpha1.GameServer) {
 	assert.Nil(t, err, fmt.Sprintf("Shouldn't be an error from syncGameServer: %+v", err))
 	assert.False(t, podAction, "Nothing should happen to a Pod")
 }
+
 func TestControllerSyncGameServerWithDevIP(t *testing.T) {
 	t.Parallel()
 
@@ -194,6 +199,9 @@ func TestControllerSyncGameServerWithDevIP(t *testing.T) {
 			ua := action.(k8stesting.UpdateAction)
 			gs := ua.GetObject().(*v1alpha1.GameServer)
 			updateCount++
+			if ua.GetSubresource() == "status" {
+				return true, gs, nil
+			}
 			expectedState := v1alpha1.GameServerStateReady
 
 			assert.Equal(t, expectedState, gs.Status.State)
@@ -213,7 +221,7 @@ func TestControllerSyncGameServerWithDevIP(t *testing.T) {
 
 		err = c.syncGameServer("default/test")
 		assert.Nil(t, err)
-		assert.Equal(t, 1, updateCount, "update reactor should fire once")
+		assert.Equal(t, 2, updateCount, "update reactor should fire twice: on Subresource and on Main Resource")
 	})
 
 	t.Run("When a GameServer has been deleted, the sync operation should be a noop", func(t *testing.T) {

pkg/sdkserver/sdkserver.go

@@ -245,13 +245,15 @@ func (s *SDKServer) updateState() error {
 		return nil
 	}
 
-	//UpdateStatus
 	s.gsUpdateMutex.RLock()
 	gs.Status.State = s.gsState
 	s.gsUpdateMutex.RUnlock()
+	// Using Status Endpoint for SDK Server
 	// Status changes are ignored on the main resource endpoint.
-	//_, err = gameServers.Update(gs)
 	_, err = gameServers.UpdateStatus(gs)
+	if err != nil {
+		return err
+	}
 
 	// state specific work here
 	if gs.Status.State == stablev1alpha1.GameServerStateUnhealthy {