From 7ba2552881b4c91223beca224745824d189bd638 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Thu, 13 Sep 2018 15:01:05 +0900 Subject: [PATCH] Fixes race condition bug with Pod not being scheduled before Ready() This fix does several things: 1. Breaks out ip and port setting of the `Status` into its own state, to make implementation and testing easier. 1. Listen for Pod updates, and if a Pod is a `GameServer` pod that has had its `NodeName` set, then queue the backing `GameServer` 1. Finally - if Ready() gets called before any of this, populate the `Status` ip and port values at this time as well. Fixes #354 --- pkg/apis/stable/v1alpha1/gameserver.go | 5 +- pkg/gameservers/controller.go | 108 ++++++++--- pkg/gameservers/controller_test.go | 237 +++++++++++++++++++++---- 3 files changed, 291 insertions(+), 59 deletions(-) diff --git a/pkg/apis/stable/v1alpha1/gameserver.go b/pkg/apis/stable/v1alpha1/gameserver.go index 47d5329605..aab38447bc 100644 --- a/pkg/apis/stable/v1alpha1/gameserver.go +++ b/pkg/apis/stable/v1alpha1/gameserver.go @@ -31,8 +31,11 @@ const ( // Creating is before the Pod for the GameServer is being created Creating State = "Creating" // Starting is for when the Pods for the GameServer are being - // created but have yet to register themselves as Ready + // created but are not yet Scheduled Starting State = "Starting" + // Scheduled is for when we have determined that the Pod has been + // scheduled in the cluster -- basically, we have a NodeName + Scheduled State = "Scheduled" // RequestReady is when the GameServer has declared that it is ready RequestReady State = "RequestReady" // Ready is when a GameServer is ready to take connections diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index f2d3da2795..23812855d4 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -132,13 +132,23 @@ func NewController( // track pod deletions, for when GameServers are deleted pods.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + UpdateFunc: func(oldObj, newObj interface{}) { + oldPod := oldObj.(*corev1.Pod) + if isGameServerPod(oldPod) { + newPod := newObj.(*corev1.Pod) + // node name has changed -- i.e. it has been scheduled + if oldPod.Spec.NodeName != newPod.Spec.NodeName { + owner := metav1.GetControllerOf(newPod) + c.workerqueue.Enqueue(cache.ExplicitKey(newPod.ObjectMeta.Namespace + "/" + owner.Name)) + } + } + }, DeleteFunc: func(obj interface{}) { // Could be a DeletedFinalStateUnknown, in which case, just ignore it pod, ok := obj.(*corev1.Pod) - if ok && v1alpha1.GameServerRolePodSelector.Matches(labels.Set(pod.ObjectMeta.Labels)) { - if owner := metav1.GetControllerOf(pod); owner != nil { - c.workerqueue.Enqueue(cache.ExplicitKey(pod.ObjectMeta.Namespace + "/" + owner.Name)) - } + if ok && isGameServerPod(pod) { + owner := metav1.GetControllerOf(pod) + c.workerqueue.Enqueue(cache.ExplicitKey(pod.ObjectMeta.Namespace + "/" + owner.Name)) } }, }) @@ -283,6 +293,9 @@ func (c *Controller) syncGameServer(key string) error { if gs, err = c.syncGameServerCreatingState(gs); err != nil { return err } + if gs, err = c.syncGameServerStartingState(gs); err != nil { + return err + } if gs, err = c.syncGameServerRequestReadyState(gs); err != nil { return err } @@ -380,34 +393,24 @@ func (c *Controller) syncGameServerCreatingState(gs *v1alpha1.GameServer) (*v1al return nil, err } - var pod *corev1.Pod if len(ret) == 0 { - gs, pod, err = c.createGameServerPod(gs) + gs, err = c.createGameServerPod(gs) if err != nil || gs.Status.State == v1alpha1.Error { return gs, err } - } else { - pod = ret[0] } gsCopy := gs.DeepCopy() - gsCopy, err = c.applyGameServerAddressAndPort(gsCopy, pod) - if err != nil { - return gs, err - } - gsCopy.Status.State = v1alpha1.Starting gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gsCopy) if err != nil { return gs, errors.Wrapf(err, "error updating GameServer %s to Starting state", gs.Name) } - - c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Address and Port populated") return gs, nil } // createGameServerPod creates the backing Pod for a given GameServer -func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.GameServer, *corev1.Pod, error) { +func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { sidecar := c.sidecar(gs) var pod *corev1.Pod pod, err := gs.Pod(sidecar) @@ -416,7 +419,7 @@ func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.Gam if err != nil { c.logger.WithField("gameserver", gs).WithError(err).Error("error creating pod from Game Server") gs, err = c.moveToErrorState(gs, err.Error()) - return gs, pod, err + return gs, err } c.addGameServerHealthCheck(gs, pod) @@ -427,14 +430,14 @@ func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.Gam if k8serrors.IsInvalid(err) { c.logger.WithField("pod", pod).WithField("gameserver", gs).Errorf("Pod created is invalid") gs, err = c.moveToErrorState(gs, err.Error()) - return gs, nil, err + return gs, err } - return gs, pod, errors.Wrapf(err, "error creating Pod for GameServer %s", gs.Name) + return gs, errors.Wrapf(err, "error creating Pod for GameServer %s", gs.Name) } c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), fmt.Sprintf("Pod %s created", pod.ObjectMeta.Name)) - return gs, pod, nil + return gs, nil } // sidecar creates the sidecar container for a given GameServer @@ -498,6 +501,39 @@ func (c *Controller) addGameServerHealthCheck(gs *v1alpha1.GameServer, pod *core } } +// syncGameServerStartingState looks for a pod that has been scheduled for this GameServer +// and then sets the Status > Address and Ports values. +func (c *Controller) syncGameServerStartingState(gs *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + if !(gs.Status.State == v1alpha1.Starting && gs.ObjectMeta.DeletionTimestamp.IsZero()) { + return gs, nil + } + + c.logger.WithField("gs", gs).Info("Syncing Starting State") + + // there should be a pod (although it may not have a scheduled container), + // so if there is an error of any kind, then move this to queue backoff + pod, err := c.gameServerPod(gs) + if err != nil { + return nil, err + } + + gsCopy := gs.DeepCopy() + // if we can't get the address, then go into queue backoff + gsCopy, err = c.applyGameServerAddressAndPort(gsCopy, pod) + if err != nil { + return gs, err + } + + gsCopy.Status.State = v1alpha1.Scheduled + gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(gsCopy) + if err != nil { + return gs, errors.Wrapf(err, "error updating GameServer %s to Scheduled state", gs.Name) + } + c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "Address and port populated") + + return gs, nil +} + // applyGameServerAddressAndPort gets the backing Pod for the GamesServer, // and sets the allocated Address and Port values to it and returns it. func (c *Controller) applyGameServerAddressAndPort(gs *v1alpha1.GameServer, pod *corev1.Pod) (*v1alpha1.GameServer, error) { @@ -530,12 +566,34 @@ func (c *Controller) syncGameServerRequestReadyState(gs *v1alpha1.GameServer) (* c.logger.WithField("gs", gs).Info("Syncing RequestReady State") gsCopy := gs.DeepCopy() + + // if the address hasn't been populated, and the Ready request comes + // before the controller has had a chance to do it, then + // do it here instead + addressPopulated := false + if gs.Status.NodeName == "" { + addressPopulated = true + pod, err := c.gameServerPod(gs) + // errPodNotFound should never happen, and if it does -- something bad happened, + // so go into workerqueue backoff. + if err != nil { + return nil, err + } + gsCopy, err = c.applyGameServerAddressAndPort(gsCopy, pod) + if err != nil { + return gs, err + } + } + gsCopy.Status.State = v1alpha1.Ready 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") + } c.recorder.Event(gs, corev1.EventTypeNormal, string(gs.Status.State), "SDK.Ready() executed") return gs, nil } @@ -635,3 +693,13 @@ func (c *Controller) address(pod *corev1.Pod) (string, error) { return "", errors.Errorf("Could not find an address for Node: %s", node.ObjectMeta.Name) } + +// isGameServerPod returns if this Pod is a Pod that comes from a GameServer +func isGameServerPod(pod *corev1.Pod) bool { + if v1alpha1.GameServerRolePodSelector.Matches(labels.Set(pod.ObjectMeta.Labels)) { + owner := metav1.GetControllerOf(pod) + return owner != nil && owner.Kind == "GameServer" + } + + return false +} diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index f7b2f5cc19..bd0d21e182 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -15,10 +15,10 @@ package gameservers import ( + "context" "encoding/json" "fmt" "testing" - "time" "agones.dev/agones/pkg/apis/stable" "agones.dev/agones/pkg/apis/stable/v1alpha1" @@ -41,7 +41,10 @@ import ( "k8s.io/client-go/tools/cache" ) -const ipFixture = "12.12.12.12" +const ( + ipFixture = "12.12.12.12" + nodeFixtureName = "node1" +) func TestControllerSyncGameServer(t *testing.T) { t.Parallel() @@ -60,11 +63,14 @@ func TestControllerSyncGameServer(t *testing.T) { }, } - node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} fixture.ApplyDefaults() + watchPods := watch.NewFake() + mocks.KubeClient.AddWatchReactor("pods", k8stesting.DefaultWatchReactor(watchPods, nil)) + mocks.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, &corev1.NodeList{Items: []corev1.Node{node}}, nil }) @@ -74,6 +80,9 @@ func TestControllerSyncGameServer(t *testing.T) { pod.Spec.NodeName = node.ObjectMeta.Name podCreated = true assert.Equal(t, fixture.ObjectMeta.Name+"-", pod.ObjectMeta.GenerateName) + watchPods.Add(pod) + // wait for the change to propagate + assert.True(t, cache.WaitForCacheSync(context.Background().Done(), mocks.KubeInformationFactory.Core().V1().Pods().Informer().HasSynced)) return true, pod, nil }) mocks.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { @@ -90,10 +99,12 @@ func TestControllerSyncGameServer(t *testing.T) { expectedState = v1alpha1.Creating case 2: expectedState = v1alpha1.Starting + case 3: + expectedState = v1alpha1.Scheduled } assert.Equal(t, expectedState, gs.Status.State) - if expectedState == v1alpha1.Starting { + if expectedState == v1alpha1.Scheduled { assert.Equal(t, ipFixture, gs.Status.Address) assert.NotEmpty(t, gs.Status.Ports[0].Port) } @@ -109,7 +120,7 @@ func TestControllerSyncGameServer(t *testing.T) { err = c.syncGameServer("default/test") assert.Nil(t, err) - assert.Equal(t, 2, updateCount, "update reactor should twice") + assert.Equal(t, 3, updateCount, "update reactor should fire thrice") assert.True(t, podCreated, "pod should be created") }) @@ -141,7 +152,7 @@ func TestControllerSyncGameServer(t *testing.T) { } func TestControllerWatchGameServers(t *testing.T) { - c, mocks := newFakeController() + c, m := newFakeController() fixture := v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingleContainerSpec()} fixture.ApplyDefaults() pod, err := fixture.Pod() @@ -150,9 +161,9 @@ func TestControllerWatchGameServers(t *testing.T) { gsWatch := watch.NewFake() podWatch := watch.NewFake() - mocks.AgonesClient.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) { + m.AgonesClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(gsWatch, nil)) + m.KubeClient.AddWatchReactor("pods", k8stesting.DefaultWatchReactor(podWatch, nil)) + m.ExtClient.AddReactor("get", "customresourcedefinitions", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, agtesting.NewEstablishedCRD(), nil }) @@ -165,9 +176,21 @@ func TestControllerWatchGameServers(t *testing.T) { return nil } - stop, cancel := agtesting.StartInformers(mocks, c.gameServerSynced) + stop, cancel := agtesting.StartInformers(m, c.gameServerSynced) defer cancel() + noStateChange := func(sync cache.InformerSynced) { + cache.WaitForCacheSync(stop, sync) + select { + case <-received: + assert.Fail(t, "Should not be queued") + default: + } + } + + podSynced := m.KubeInformationFactory.Core().V1().Pods().Informer().HasSynced + gsSynced := m.AgonesInformerFactory.Stable().V1alpha1().GameServers().Informer().HasSynced + go func() { err := c.Run(1, stop) assert.Nil(t, err, "Run should not error") @@ -177,14 +200,25 @@ func TestControllerWatchGameServers(t *testing.T) { gsWatch.Add(&fixture) assert.Equal(t, "default/test", <-received) podWatch.Add(pod) + noStateChange(podSynced) // no state change gsWatch.Modify(&fixture) - select { - case <-received: - assert.Fail(t, "Should not be queued") - case <-time.After(time.Second): - } + noStateChange(gsSynced) + + // add a non game pod + nonGamePod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}} + podWatch.Add(nonGamePod) + noStateChange(podSynced) + + // no state change + gsWatch.Modify(&fixture) + noStateChange(gsSynced) + + // no state change + gsWatch.Modify(&fixture) + noStateChange(gsSynced) + copyFixture := fixture.DeepCopy() copyFixture.Status.State = v1alpha1.Starting logrus.Info("modify copyFixture") @@ -193,6 +227,20 @@ func TestControllerWatchGameServers(t *testing.T) { podWatch.Delete(pod) assert.Equal(t, "default/test", <-received) + + // add an unscheduled game pod + pod, err = fixture.Pod() + assert.Nil(t, err) + pod.ObjectMeta.Name = pod.ObjectMeta.GenerateName + "-pod2" + podWatch.Add(pod) + noStateChange(podSynced) + + // schedule it + podCopy := pod.DeepCopy() + podCopy.Spec.NodeName = nodeFixtureName + + podWatch.Modify(podCopy) + assert.Equal(t, "default/test", <-received) } func TestControllerCreationMutationHandler(t *testing.T) { @@ -392,7 +440,7 @@ func TestControllerSyncGameServerPortAllocationState(t *testing.T) { } fixture.ApplyDefaults() mocks.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { - return true, &corev1.NodeList{Items: []corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node1"}}}}, nil + return true, &corev1.NodeList{Items: []corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}}}}, nil }) updated := false @@ -438,6 +486,8 @@ func TestControllerSyncGameServerPortAllocationState(t *testing.T) { } func TestControllerSyncGameServerCreatingState(t *testing.T) { + t.Parallel() + newFixture := func() *v1alpha1.GameServer { fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.Creating}} @@ -445,23 +495,17 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { return fixture } - node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} - t.Run("Syncing from Created State, with no issues", func(t *testing.T) { c, m := newFakeController() fixture := newFixture() podCreated := false gsUpdated := false - m.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { - return true, &corev1.NodeList{Items: []corev1.Node{node}}, nil - }) var pod *corev1.Pod m.KubeClient.AddReactor("create", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { podCreated = true ca := action.(k8stesting.CreateAction) pod = ca.GetObject().(*corev1.Pod) - pod.Spec.NodeName = node.ObjectMeta.Name assert.True(t, metav1.IsControlledBy(pod, fixture)) return true, pod, nil }) @@ -483,11 +527,8 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { assert.True(t, podCreated, "Pod should have been created") assert.Equal(t, v1alpha1.Starting, gs.Status.State) - assert.Equal(t, ipFixture, gs.Status.Address) - assert.True(t, gsUpdated, "GameServer should have been updated") agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Pod") - agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Address and Port") }) t.Run("Previously started sync, created Pod, but didn't move to Starting", func(t *testing.T) { @@ -496,12 +537,8 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { podCreated := false gsUpdated := false pod, err := fixture.Pod() - pod.Spec.NodeName = node.ObjectMeta.Name assert.Nil(t, err) - m.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { - return true, &corev1.NodeList{Items: []corev1.Node{node}}, nil - }) m.KubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil }) @@ -558,7 +595,7 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { t.Run("GameServer with unknown state", func(t *testing.T) { testNoChange(t, "Unknown", func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { - return c.syncGameServerRequestReadyState(fixture) + return c.syncGameServerCreatingState(fixture) }) }) @@ -569,6 +606,68 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { }) } +func TestControllerSyncGameServerStartingState(t *testing.T) { + t.Parallel() + + newFixture := func() *v1alpha1.GameServer { + fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, + Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.Starting}} + fixture.ApplyDefaults() + return fixture + } + + node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} + + t.Run("sync from Stating state, with no issues", func(t *testing.T) { + c, m := newFakeController() + gsFixture := newFixture() + gsFixture.ApplyDefaults() + pod, err := gsFixture.Pod() + pod.Spec.NodeName = nodeFixtureName + assert.Nil(t, err) + gsUpdated := false + + m.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &corev1.NodeList{Items: []corev1.Node{node}}, nil + }) + m.KubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil + }) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gsUpdated = true + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*v1alpha1.GameServer) + assert.Equal(t, v1alpha1.Scheduled, gs.Status.State) + return true, gs, nil + }) + + _, cancel := agtesting.StartInformers(m, c.gameServerSynced) + defer cancel() + + gs, err := c.syncGameServerStartingState(gsFixture) + assert.Nil(t, err) + + assert.True(t, gsUpdated) + assert.Equal(t, gs.Status.NodeName, node.ObjectMeta.Name) + assert.Equal(t, gs.Status.Address, ipFixture) + + agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Address and port populated") + assert.NotEmpty(t, gs.Status.Ports) + }) + + t.Run("GameServer with unknown state", func(t *testing.T) { + testNoChange(t, "Unknown", func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + return c.syncGameServerStartingState(fixture) + }) + }) + + t.Run("GameServer with non zero deletion datetime", func(t *testing.T) { + testWithNonZeroDeletionTimestamp(t, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + return c.syncGameServerStartingState(fixture) + }) + }) +} + func TestControllerCreateGameServerPod(t *testing.T) { t.Parallel() @@ -614,12 +713,11 @@ func TestControllerCreateGameServerPod(t *testing.T) { return true, pod, nil }) - gs, pod, err := c.createGameServerPod(fixture) + gs, err := c.createGameServerPod(fixture) assert.Nil(t, err) assert.Equal(t, fixture.Status.State, gs.Status.State) assert.True(t, created) - assert.True(t, metav1.IsControlledBy(pod, gs)) agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Pod") }) @@ -641,7 +739,7 @@ func TestControllerCreateGameServerPod(t *testing.T) { return true, gs, nil }) - gs, _, err := c.createGameServerPod(fixture) + gs, err := c.createGameServerPod(fixture) assert.Nil(t, err) assert.True(t, podCreated, "attempt should have been made to create a pod") @@ -657,7 +755,7 @@ func TestControllerApplyGameServerAddressAndPort(t *testing.T) { gsFixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.RequestReady}} gsFixture.ApplyDefaults() - node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} + node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} pod, err := gsFixture.Pod() assert.Nil(t, err) pod.Spec.NodeName = node.ObjectMeta.Name @@ -682,13 +780,53 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) { t.Run("GameServer with ReadyRequest State", func(t *testing.T) { c, m := newFakeController() + gsFixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, + Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.RequestReady}} + gsFixture.ApplyDefaults() + gsFixture.Status.NodeName = "node" + pod, err := gsFixture.Pod() + assert.Nil(t, err) + gsUpdated := false + + m.KubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil + }) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + gsUpdated = true + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*v1alpha1.GameServer) + assert.Equal(t, v1alpha1.Ready, gs.Status.State) + return true, gs, nil + }) + + _, cancel := agtesting.StartInformers(m, c.gameServerSynced) + defer cancel() + + gs, err := c.syncGameServerRequestReadyState(gsFixture) + assert.Nil(t, err, "should not error") + assert.True(t, gsUpdated, "GameServer wasn't updated") + assert.Equal(t, v1alpha1.Ready, gs.Status.State) + agtesting.AssertEventContains(t, m.FakeRecorder.Events, "SDK.Ready() executed") + }) + + t.Run("GameServer without an Address, but RequestReady State", func(t *testing.T) { + c, m := newFakeController() + gsFixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.RequestReady}} gsFixture.ApplyDefaults() pod, err := gsFixture.Pod() + pod.Spec.NodeName = nodeFixtureName assert.Nil(t, err) gsUpdated := false + ipFixture := "12.12.12.12" + nodeFixture := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} + + m.KubeClient.AddReactor("list", "nodes", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, &corev1.NodeList{Items: []corev1.Node{nodeFixture}}, nil + }) + m.KubeClient.AddReactor("list", "pods", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil }) @@ -707,6 +845,11 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) { assert.Nil(t, err, "should not error") assert.True(t, gsUpdated, "GameServer wasn't updated") assert.Equal(t, v1alpha1.Ready, gs.Status.State) + + assert.Equal(t, gs.Status.NodeName, nodeFixture.ObjectMeta.Name) + assert.Equal(t, gs.Status.Address, ipFixture) + + agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Address and port populated") agtesting.AssertEventContains(t, m.FakeRecorder.Events, "SDK.Ready() executed") }) @@ -714,7 +857,7 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) { name := fmt.Sprintf("GameServer with %s state", s) t.Run(name, func(t *testing.T) { testNoChange(t, s, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { - return c.syncGameServerCreatingState(fixture) + return c.syncGameServerRequestReadyState(fixture) }) }) } @@ -775,15 +918,15 @@ func TestControllerAddress(t *testing.T) { expectedAddress string }{ "node with external ip": { - node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: "12.12.12.12", Type: corev1.NodeExternalIP}}}}, + node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: "12.12.12.12", Type: corev1.NodeExternalIP}}}}, expectedAddress: "12.12.12.12", }, "node with an internal ip": { - node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: "11.11.11.11", Type: corev1.NodeInternalIP}}}}, + node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: "11.11.11.11", Type: corev1.NodeInternalIP}}}}, expectedAddress: "11.11.11.11", }, "node with internal and external ip": { - node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, + node: corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeFixtureName}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{ {Address: "9.9.9.8", Type: corev1.NodeExternalIP}, {Address: "12.12.12.12", Type: corev1.NodeInternalIP}, @@ -889,6 +1032,24 @@ func TestControllerAddGameServerHealthCheck(t *testing.T) { assert.Equal(t, fixture.Spec.Health.PeriodSeconds, probe.PeriodSeconds) } +func TestIsGameServerPod(t *testing.T) { + + t.Run("it is a game server pod", func(t *testing.T) { + gs := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gameserver", UID: "1234"}, Spec: newSingleContainerSpec()} + gs.ApplyDefaults() + pod, err := gs.Pod() + assert.Nil(t, err) + + assert.True(t, isGameServerPod(pod)) + }) + + t.Run("it is not a game server pod", func(t *testing.T) { + pod := &corev1.Pod{} + assert.False(t, isGameServerPod(pod)) + }) + +} + // testNoChange runs a test with a state that doesn't exist, to ensure a handler // doesn't do process anything beyond the state it is meant to handle. func testNoChange(t *testing.T, state v1alpha1.State, f func(*Controller, *v1alpha1.GameServer) (*v1alpha1.GameServer, error)) {