From fd0b5bf587c00f510aef71f48b88e6e830a367f2 Mon Sep 17 00:00:00 2001 From: Ilker Celikyilmaz Date: Thu, 7 Feb 2019 10:48:04 -0800 Subject: [PATCH] Remove the mutex usage Delete GS in both GS and GSS --- pkg/gameservers/controller.go | 2 -- pkg/gameserversets/controller.go | 9 +++++---- pkg/gameserversets/controller_test.go | 29 +++++++++++++++++---------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index 7f161d3a63..57f9151c7f 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -677,9 +677,7 @@ func (c *Controller) syncGameServerShutdownState(gs *v1alpha1.GameServer) error // be explicit about where to delete. We only need to wait for the Pod to be removed, which we handle with our // own finalizer. p := metav1.DeletePropagationBackground - c.allocationMutex.Lock() err := c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Delete(gs.ObjectMeta.Name, &metav1.DeleteOptions{PropagationPolicy: &p}) - c.allocationMutex.Unlock() if err != nil { return errors.Wrapf(err, "error deleting Game Server %s", gs.ObjectMeta.Name) } diff --git a/pkg/gameserversets/controller.go b/pkg/gameserversets/controller.go index cb1b3f0a96..8ca1b2d818 100644 --- a/pkg/gameserversets/controller.go +++ b/pkg/gameserversets/controller.go @@ -440,11 +440,12 @@ func (c *Controller) deleteGameServers(gsSet *v1alpha1.GameServerSet, toDelete [ c.logger.WithField("diff", len(toDelete)).WithField("gameserverset", gsSet.ObjectMeta.Name).Info("Deleting gameservers") return parallelize(gameServerListToChannel(toDelete), maxDeletionParallelism, func(gs *v1alpha1.GameServer) error { - c.allocationMutex.Lock() - err := c.gameServerGetter.GameServers(gs.Namespace).Delete(gs.ObjectMeta.Name, nil) - c.allocationMutex.Unlock() + // We should not delete the gameservers directly buy set their state to shutdown and let the gameserver controller to delete + gsCopy := gs.DeepCopy() + gsCopy.Status.State = v1alpha1.GameServerStateShutdown + _, err := c.gameServerGetter.GameServers(gs.Namespace).Update(gsCopy) if err != nil { - return errors.Wrapf(err, "error deleting gameserver %s", gsSet.ObjectMeta.Name) + return errors.Wrapf(err, "error updating gameserver %s from status %s to Shutdown status.", gs.ObjectMeta.Name, gs.Status.State) } c.stateCache.forGameServerSet(gsSet).deleted(gs) diff --git a/pkg/gameserversets/controller_test.go b/pkg/gameserversets/controller_test.go index 3089c5e497..c3aa0e29ff 100644 --- a/pkg/gameserversets/controller_test.go +++ b/pkg/gameserversets/controller_test.go @@ -285,7 +285,7 @@ func TestSyncGameServerSet(t *testing.T) { // make some as unhealthy list[0].Status.State = v1alpha1.GameServerStateUnhealthy - deleted := false + updated := false count := 0 c, m := newFakeController() @@ -296,10 +296,13 @@ func TestSyncGameServerSet(t *testing.T) { return true, &v1alpha1.GameServerList{Items: list}, nil }) - m.AgonesClient.AddReactor("delete", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { - da := action.(k8stesting.DeleteAction) - deleted = true - assert.Equal(t, "test-0", da.GetName()) + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*v1alpha1.GameServer) + assert.Equal(t, gs.Status.State, v1alpha1.GameServerStateShutdown) + + updated = true + assert.Equal(t, "test-0", gs.GetName()) return true, nil, nil }) m.AgonesClient.AddReactor("create", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { @@ -317,7 +320,7 @@ func TestSyncGameServerSet(t *testing.T) { c.syncGameServerSet(gsSet.ObjectMeta.Namespace + "/" + gsSet.ObjectMeta.Name) // nolint: errcheck assert.Equal(t, 6, count) - assert.True(t, deleted, "A game servers should have been deleted") + assert.True(t, updated, "A game servers should have been updated") }) t.Run("removing gamservers", func(t *testing.T) { @@ -332,7 +335,7 @@ func TestSyncGameServerSet(t *testing.T) { m.AgonesClient.AddReactor("list", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { return true, &v1alpha1.GameServerList{Items: list}, nil }) - m.AgonesClient.AddReactor("delete", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { count++ return true, nil, nil }) @@ -363,12 +366,16 @@ func TestControllerSyncUnhealthyGameServers(t *testing.T) { gs3.ObjectMeta.DeletionTimestamp = &now gs3.Status = v1alpha1.GameServerStatus{State: v1alpha1.GameServerStateReady} - var deletedCount int + var updatedCount int c, m := newFakeController() - m.AgonesClient.AddReactor("delete", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { - deletedCount++ + m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { + ua := action.(k8stesting.UpdateAction) + gs := ua.GetObject().(*v1alpha1.GameServer) + + assert.Equal(t, gs.Status.State, v1alpha1.GameServerStateShutdown) + updatedCount++ return true, nil, nil }) @@ -378,7 +385,7 @@ func TestControllerSyncUnhealthyGameServers(t *testing.T) { err := c.deleteGameServers(gsSet, []*v1alpha1.GameServer{gs1, gs2, gs3}) assert.Nil(t, err) - assert.Equal(t, 3, deletedCount, "Deletion should have occured") + assert.Equal(t, 3, updatedCount, "Updates should have occured") } func TestSyncMoreGameServers(t *testing.T) {