Finalizer for GameServer until backing Pods are Terminated

Because we allocate a port to a GameSever, we cannot
delete the GameServer until that port is free again.

This means that a GameServer must not be hard deleted until
the backing Pod is successfully Terminated - and the port is
now free again.

This implements a finalizer and backing controller code to manage
this.

This is work that need to occur for #14 (Dynamic Port Allocation)
to be completed.

gameservers/controller/controller.go

@@ -97,12 +97,23 @@ func NewController(sidecarImage string,
 			// no point in processing unless there is a State change
 			oldGs := oldObj.(*stablev1alpha1.GameServer)
 			newGs := newObj.(*stablev1alpha1.GameServer)
-			if oldGs.Status.State != newGs.Status.State {
+			if oldGs.Status.State != newGs.Status.State || oldGs.ObjectMeta.DeletionTimestamp != newGs.ObjectMeta.DeletionTimestamp {
 				c.enqueueGameServer(newGs)
 			}
 		},
 	})
 
+	// track pod deletions, for when GameServers are deleted
+	kubeInformerFactory.Core().V1().Pods().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		DeleteFunc: func(obj interface{}) {
+			pod := obj.(*corev1.Pod)
+			if stablev1alpha1.GameServerRolePodSelector.Matches(labels.Set(pod.ObjectMeta.Labels)) {
+				owner := metav1.GetControllerOf(pod)
+				c.enqueueGameServer(cache.ExplicitKey(pod.ObjectMeta.Namespace + "/" + owner.Name))
+			}
+		},
+	})
+
 	c.syncHandler = c.syncGameServer
 
 	mux := http.NewServeMux()
@@ -223,8 +234,10 @@ func (c *Controller) syncGameServer(key string) error {
 		return errors.Wrapf(err, "error retrieving GameServer %s from namespace %s", name, namespace)
 	}
 
-	gs, err = c.syncGameServerBlankState(gs)
-	if err != nil {
+	if gs, err = c.syncGameServerDeletionTimestamp(gs); err != nil {
+		return err
+	}
+	if gs, err = c.syncGameServerBlankState(gs); err != nil {
 		return err
 	}
 	if gs, err = c.syncGameServerCreatingState(gs); err != nil {
@@ -240,10 +253,49 @@ func (c *Controller) syncGameServer(key string) error {
 	return nil
 }
 
+// syncGameServerDeletionTimestamp if the deletion timestamp is non-zero
+// then do one of two things:
+// - if the GameServer has Pods running, delete them
+// - if there no pods, remove the finalizer
+func (c *Controller) syncGameServerDeletionTimestamp(gs *stablev1alpha1.GameServer) (*stablev1alpha1.GameServer, error) {
+	if !gs.ObjectMeta.DeletionTimestamp.IsZero() {
+		logrus.WithField("gs", gs).Info("Syncing with Deletion Timestamp")
+		pods, err := c.listGameServerPods(gs)
+		if err != nil {
+			return gs, err
+		}
+
+		if len(pods) > 0 {
+			logrus.WithField("pods", pods).WithField("gsName", gs.ObjectMeta.Name).Info("Found pods, deleting")
+			for _, p := range pods {
+				err := c.podGetter.Pods(p.ObjectMeta.Namespace).Delete(p.ObjectMeta.Name, nil)
+				if err != nil {
+					return gs, errors.Wrapf(err, "error deleting pod for GameServer %s, %s", gs.ObjectMeta.Name, p.ObjectMeta.Name)
+				}
+			}
+		} else {
+			gsCopy := gs.DeepCopy()
+			// remove the finalizer for this controller
+			var fin []string
+			for _, f := range gsCopy.ObjectMeta.Finalizers {
+				if f != stable.GroupName {
+					fin = append(fin, f)
+				}
+			}
+			gsCopy.ObjectMeta.Finalizers = fin
+			logrus.WithField("gs", gsCopy).Infof("No pods found, removing finalizer %s", stable.GroupName)
+			gs, err := c.gameServerGetter.GameServers(gsCopy.ObjectMeta.Namespace).Update(gsCopy)
+			return gs, errors.Wrapf(err, "error removing finalizer for GameServer %s", gsCopy.ObjectMeta.Name)
+		}
+	}
+
+	return gs, nil
+}
+
 // syncGameServerBlankState applies default values to the the GameServer if its state is "" (blank)
 // returns an updated GameServer
 func (c *Controller) syncGameServerBlankState(gs *stablev1alpha1.GameServer) (*stablev1alpha1.GameServer, error) {
-	if gs.Status.State == "" {
+	if gs.Status.State == "" && gs.ObjectMeta.DeletionTimestamp.IsZero() {
 		gsCopy := gs.DeepCopy()
 		gsCopy.ApplyDefaults()
 		logrus.WithField("gs", gsCopy).Info("Syncing Blank State")
@@ -256,7 +308,7 @@ func (c *Controller) syncGameServerBlankState(gs *stablev1alpha1.GameServer) (*s
 // syncGameServerCreatingState checks if the GameServer is in the Creating state, and if so
 // creates a Pod for the GameServer and moves the state to Starting
 func (c *Controller) syncGameServerCreatingState(gs *stablev1alpha1.GameServer) (*stablev1alpha1.GameServer, error) {
-	if gs.Status.State == stablev1alpha1.Creating {
+	if gs.Status.State == stablev1alpha1.Creating && gs.ObjectMeta.DeletionTimestamp.IsZero() {
 		logrus.WithField("gs", gs).Info("Syncing Create State")
 
 		// Maybe something went wrong, and the pod was created, but the state was never moved to Starting, so let's check
@@ -318,7 +370,7 @@ func (c *Controller) syncGameServerCreatingState(gs *stablev1alpha1.GameServer)
 // and then adds the IP and Port information to the Status and marks the GameServer
 // as Ready
 func (c *Controller) syncGameServerRequestReadyState(gs *stablev1alpha1.GameServer) (*stablev1alpha1.GameServer, error) {
-	if gs.Status.State == stablev1alpha1.RequestReady {
+	if gs.Status.State == stablev1alpha1.RequestReady && gs.ObjectMeta.DeletionTimestamp.IsZero() {
 		logrus.WithField("gs", gs).Info("Syncing RequestReady State")
 		pod, err := c.gameServerPod(gs)
 		if err != nil {
@@ -344,7 +396,7 @@ func (c *Controller) syncGameServerRequestReadyState(gs *stablev1alpha1.GameServ
 
 // syncGameServerShutdownState deletes the game server (and therefore the backing Pod) if it is in shutdown state
 func (c *Controller) syncGameServerShutdownState(gs *stablev1alpha1.GameServer) (*stablev1alpha1.GameServer, error) {
-	if gs.Status.State == stablev1alpha1.Shutdown {
+	if gs.Status.State == stablev1alpha1.Shutdown && gs.ObjectMeta.DeletionTimestamp.IsZero() {
 		logrus.WithField("gs", gs).Info("Syncing Shutdown State")
 		// let's be explicit about how we want to shut things down
 		p := metav1.DeletePropagationBackground

gameservers/controller/controller_test.go

@@ -162,9 +162,16 @@ func TestSyncGameServer(t *testing.T) {
 
 func TestWatchGameServers(t *testing.T) {
 	c, mocks := newFakeController()
-	fixture := v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}}
-	fakeWatch := watch.NewFake()
-	mocks.agonClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(fakeWatch, nil))
+	fixture := v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingeContainerSpec()}
+	fixture.ApplyDefaults()
+	pod, err := fixture.Pod()
+	assert.Nil(t, err)
+	pod.ObjectMeta.Name = pod.ObjectMeta.GenerateName + "-pod"
+
+	gsWatch := watch.NewFake()
+	podWatch := watch.NewFake()
+	mocks.agonClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(gsWatch, nil))
+	mocks.kubeClient.AddWatchReactor("pods", k8stesting.DefaultWatchReactor(podWatch, nil))
 	mocks.extClient.AddReactor("get", "customresourcedefinitions", func(action k8stesting.Action) (bool, runtime.Object, error) {
 		return true, newEstablishedCRD(), nil
 	})
@@ -187,15 +194,24 @@ func TestWatchGameServers(t *testing.T) {
 	}()
 
 	logrus.Info("Adding first fixture")
-	fakeWatch.Add(&fixture)
+	gsWatch.Add(&fixture)
 	assert.Equal(t, "default/test", <-received)
+	podWatch.Add(pod)
 
 	// no state change
-	fakeWatch.Modify(&fixture)
+	gsWatch.Modify(&fixture)
+	select {
+	case <-received:
+		assert.Fail(t, "Should not be queued")
+	case <-time.After(time.Second):
+	}
 	copyFixture := fixture.DeepCopy()
 	copyFixture.Status.State = v1alpha1.Starting
 	logrus.Info("modify copyFixture")
-	fakeWatch.Modify(copyFixture)
+	gsWatch.Modify(copyFixture)
+	assert.Equal(t, "default/test", <-received)
+
+	podWatch.Delete(pod)
 	assert.Equal(t, "default/test", <-received)
 }
 
@@ -225,6 +241,68 @@ func TestHealthCheck(t *testing.T) {
 	assert.Equal(t, []byte("ok"), body, "response body should be 'ok'")
 }
 
+func TestSyncGameServerDeletionTimestamp(t *testing.T) {
+	t.Parallel()
+
+	t.Run("GameServer has a Pod", func(t *testing.T) {
+		c, mocks := newFakeController()
+		now := metav1.Now()
+		fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", DeletionTimestamp: &now},
+			Spec: newSingeContainerSpec()}
+		fixture.ApplyDefaults()
+		pod, err := fixture.Pod()
+		assert.Nil(t, err)
+		pod.ObjectMeta.Name = pod.ObjectMeta.GenerateName
+
+		deleted := false
+		mocks.kubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
+			return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil
+		})
+		mocks.kubeClient.AddReactor("delete", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) {
+			deleted = true
+			da := action.(k8stesting.DeleteAction)
+			assert.Equal(t, pod.ObjectMeta.Name, da.GetName())
+			return true, nil, nil
+		})
+
+		stop := startInformers(c, mocks)
+		defer close(stop)
+
+		result, err := c.syncGameServerDeletionTimestamp(fixture)
+		assert.Nil(t, err)
+		assert.True(t, deleted, "pod should be deleted")
+		assert.Equal(t, fixture, result)
+	})
+
+	t.Run("GameServer's Pods have been deleted", func(t *testing.T) {
+		c, mocks := newFakeController()
+		now := metav1.Now()
+		fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", DeletionTimestamp: &now},
+			Spec: newSingeContainerSpec()}
+		fixture.ApplyDefaults()
+
+		updated := false
+		mocks.agonClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {
+			updated = true
+
+			ua := action.(k8stesting.UpdateAction)
+			gs := ua.GetObject().(*v1alpha1.GameServer)
+			assert.Equal(t, fixture.ObjectMeta.Name, gs.ObjectMeta.Name)
+			assert.Empty(t, gs.ObjectMeta.Finalizers)
+
+			return true, gs, nil
+		})
+		stop := startInformers(c, mocks)
+		defer close(stop)
+
+		result, err := c.syncGameServerDeletionTimestamp(fixture)
+		assert.Nil(t, err)
+		assert.True(t, updated, "gameserver should be updated, to remove the finaliser")
+		assert.Equal(t, fixture.ObjectMeta.Name, result.ObjectMeta.Name)
+		assert.Empty(t, result.ObjectMeta.Finalizers)
+	})
+}
+
 func TestSyncGameServerBlankState(t *testing.T) {
 
 	t.Run("GameServer with a blank initial state", func(t *testing.T) {
@@ -254,6 +332,12 @@ func TestSyncGameServerBlankState(t *testing.T) {
 			return c.syncGameServerBlankState(fixture)
 		})
 	})
+
+	t.Run("GameServer with non zero deletion datetime", func(t *testing.T) {
+		testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) {
+			return c.syncGameServerRequestReadyState(fixture)
+		})
+	})
 }
 
 func TestSyncGameServerCreatingState(t *testing.T) {
@@ -376,6 +460,12 @@ func TestSyncGameServerCreatingState(t *testing.T) {
 			return c.syncGameServerCreatingState(fixture)
 		})
 	})
+
+	t.Run("GameServer with non zero deletion datetime", func(t *testing.T) {
+		testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) {
+			return c.syncGameServerRequestReadyState(fixture)
+		})
+	})
 }
 
 func TestSyncGameServerRequestReadyState(t *testing.T) {
@@ -427,6 +517,12 @@ func TestSyncGameServerRequestReadyState(t *testing.T) {
 			return c.syncGameServerRequestReadyState(fixture)
 		})
 	})
+
+	t.Run("GameServer with non zero deletion datetime", func(t *testing.T) {
+		testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) {
+			return c.syncGameServerRequestReadyState(fixture)
+		})
+	})
 }
 
 func TestSyncGameServerShutdownState(t *testing.T) {
@@ -460,6 +556,12 @@ func TestSyncGameServerShutdownState(t *testing.T) {
 			return c.syncGameServerRequestReadyState(fixture)
 		})
 	})
+
+	t.Run("GameServer with non zero deletion datetime", func(t *testing.T) {
+		testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) {
+			return c.syncGameServerRequestReadyState(fixture)
+		})
+	})
 }
 
 func TestControllerExternalIP(t *testing.T) {
@@ -556,6 +658,26 @@ func testWithUnknownState(t *testing.T, f func(*Controller, *v1alpha1.GameServer
 	assert.Equal(t, fixture, result)
 }
 
+// testWithNonZeroDeletionTimestamp runs a test with a given state, but
+// the DeletionTimestamp set to Now()
+func testWithNonZeroDeletionTimestamp(t *testing.T, state v1alpha1.State, f func(*Controller, *v1alpha1.GameServer) (*v1alpha1.GameServer, error)) {
+	c, mocks := newFakeController()
+	now := metav1.Now()
+	fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", DeletionTimestamp: &now},
+		Spec: newSingeContainerSpec(), Status: v1alpha1.GameServerStatus{State: state}}
+	fixture.ApplyDefaults()
+	updated := false
+	mocks.agonClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		updated = true
+		return true, nil, nil
+	})
+
+	result, err := f(c, fixture)
+	assert.Nil(t, err, "sync should not error")
+	assert.False(t, updated, "update should occur")
+	assert.Equal(t, fixture, result)
+}
+
 // holder for all my fakes and mocks
 type mocks struct {
 	kubeClient             *kubefake.Clientset

pkg/apis/stable/v1alpha1/types.go

@@ -19,6 +19,7 @@ import (
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 )
 
 const (
@@ -44,11 +45,21 @@ const (
 	// in the configuration.
 	Static PortPolicy = "static"
 
+	// RoleLabel is the label in which the Agon role is specified.
+	// Pods from a GameServer will have the value "gameserver"
+	RoleLabel = stable.GroupName + "/role"
+	// GameServerLabelRole is the GameServer label value for RoleLabel
+	GameServerLabelRole = "gameserver"
 	// GameServerPodLabel is the label that the name of the GameServer
 	// is set on the Pod the GameServer controls
 	GameServerPodLabel = stable.GroupName + "/gameserver"
 )
 
+var (
+	// GameServerRolePodSelector is the selector to get all GameServer Pods
+	GameServerRolePodSelector = labels.SelectorFromSet(labels.Set{RoleLabel: GameServerLabelRole})
+)
+
 // +genclient
 // +genclient:noStatus
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -108,6 +119,8 @@ type GameServerList struct {
 
 // ApplyDefaults applies default values to the GameServer if they are not already populated
 func (gs *GameServer) ApplyDefaults() {
+	gs.ObjectMeta.Finalizers = append(gs.ObjectMeta.Finalizers, stable.GroupName)
+
 	if len(gs.Spec.Template.Spec.Containers) == 1 {
 		gs.Spec.Container = gs.Spec.Template.Spec.Containers[0].Name
 	}
@@ -153,7 +166,7 @@ func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) {
 	if pod.ObjectMeta.Labels == nil {
 		pod.ObjectMeta.Labels = make(map[string]string, 1)
 	}
-	pod.ObjectMeta.Labels[stable.GroupName+"/role"] = "gameserver"
+	pod.ObjectMeta.Labels[RoleLabel] = GameServerLabelRole
 	// store the GameServer name as a label, for easy lookup later on
 	pod.ObjectMeta.Labels[GameServerPodLabel] = gs.ObjectMeta.Name
 	ref := metav1.NewControllerRef(gs, SchemeGroupVersion.WithKind("GameServer"))

pkg/apis/stable/v1alpha1/types_test.go

@@ -91,6 +91,7 @@ func TestGameServerApplyDefaults(t *testing.T) {
 			test.gameServer.ApplyDefaults()
 
 			spec := test.gameServer.Spec
+			assert.Contains(t, test.gameServer.ObjectMeta.Finalizers, stable.GroupName)
 			assert.Equal(t, test.expectedContainer, spec.Container)
 			assert.Equal(t, test.expectedProtocol, spec.Protocol)
 			assert.Equal(t, test.expectedState, test.gameServer.Status.State)

sdks/cpp/Makefile

@@ -60,7 +60,6 @@ ensure-bin:
 	-mkdir $(build_path)/bin
 
 # build dev and runtime tarballs
-# TODO: add the verrsion to the archive (pass through from main Makefile
 archive: VERSION = "dev"
 archive:
 	-rm $(build_path)/bin/argonsdk-$(VERSION)-dev-linux-arch_64.tar.gz