From 2d5219de96941718daff1425ee92885d7493f2fc Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Fri, 17 May 2019 20:21:37 +0900 Subject: [PATCH] Prevent race conditions by syncing cache on new Allocation elements Added cache.InformerSynced members for the new resources that are worked on within this code for the multicluster allocations, to ensure they aren't empty values on controller startup. Also applying these at the test level, so we don't get random test flakiness due to cache's not being populated in time for the test to run. --- pkg/gameserverallocations/controller.go | 6 +++++- pkg/gameserverallocations/controller_test.go | 20 ++++++++++---------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/gameserverallocations/controller.go b/pkg/gameserverallocations/controller.go index e4a547b30f..d7eda8cb2b 100644 --- a/pkg/gameserverallocations/controller.go +++ b/pkg/gameserverallocations/controller.go @@ -87,7 +87,9 @@ type Controller struct { gameServerGetter getterv1alpha1.GameServersGetter gameServerLister listerv1alpha1.GameServerLister allocationPolicyLister multiclusterlisterv1alpha1.GameServerAllocationPolicyLister + allocationPolicySynced cache.InformerSynced secretLister corev1lister.SecretLister + secretSynced cache.InformerSynced stop <-chan struct{} workerqueue *workerqueue.WorkerQueue recorder record.EventRecorder @@ -124,7 +126,9 @@ func NewController(apiServer *apiserver.APIServer, gameServerGetter: agonesClient.StableV1alpha1(), gameServerLister: agonesInformer.GameServers().Lister(), allocationPolicyLister: agonesInformerFactory.Multicluster().V1alpha1().GameServerAllocationPolicies().Lister(), + allocationPolicySynced: agonesInformerFactory.Multicluster().V1alpha1().GameServerAllocationPolicies().Informer().HasSynced, secretLister: kubeInformerFactory.Core().V1().Secrets().Lister(), + secretSynced: kubeInformerFactory.Core().V1().Secrets().Informer().HasSynced, } c.baseLogger = runtime.NewLoggerWithType(c) c.workerqueue = workerqueue.NewWorkerQueue(c.syncGameServers, c.baseLogger, logfields.GameServerKey, stable.GroupName+".GameServerUpdateController") @@ -177,7 +181,7 @@ func (c *Controller) registerAPIResource(api *apiserver.APIServer) { func (c *Controller) Run(_ int, stop <-chan struct{}) error { c.stop = stop c.baseLogger.Info("Wait for cache sync") - if !cache.WaitForCacheSync(stop, c.gameServerSynced) { + if !cache.WaitForCacheSync(stop, c.gameServerSynced, c.secretSynced, c.allocationPolicySynced) { return errors.New("failed to wait for caches to sync") } diff --git a/pkg/gameserverallocations/controller_test.go b/pkg/gameserverallocations/controller_test.go index 5773a9b926..99e4a5ab4e 100644 --- a/pkg/gameserverallocations/controller_test.go +++ b/pkg/gameserverallocations/controller_test.go @@ -75,7 +75,7 @@ func TestControllerAllocationHandler(t *testing.T) { return true, gs, nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -254,7 +254,7 @@ func TestControllerAllocatePriority(t *testing.T) { return true, gs, nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -646,7 +646,7 @@ func TestControllerRunCacheSync(t *testing.T) { m.AgonesClient.AddWatchReactor("gameservers", k8stesting.DefaultWatchReactor(watch, nil)) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.gameServerSynced) defer cancel() assertCacheEntries := func(expected int) { @@ -759,7 +759,7 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) { }, nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.allocationPolicySynced, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -805,7 +805,7 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) { }, nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.allocationPolicySynced, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -893,7 +893,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { return true, getTestSecret(secretName, server.TLS.Certificates[0].Certificate[0]), nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.allocationPolicySynced, c.secretSynced, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -968,7 +968,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { return true, getTestSecret(secretName, server.TLS.Certificates[0].Certificate[0]), nil }) - stop, cancel := agtesting.StartInformers(m) + stop, cancel := agtesting.StartInformers(m, c.allocationPolicySynced, c.secretSynced, c.gameServerSynced) defer cancel() // This call initializes the cache @@ -1023,7 +1023,7 @@ func TestCreateRestClientError(t *testing.T) { }}}, nil }) - _, cancel := agtesting.StartInformers(m) + _, cancel := agtesting.StartInformers(m, c.secretSynced) defer cancel() _, err := c.createRemoteClusterRestClient(defaultNs, "secret-name") @@ -1048,7 +1048,7 @@ func TestCreateRestClientError(t *testing.T) { }}}, nil }) - _, cancel := agtesting.StartInformers(m) + _, cancel := agtesting.StartInformers(m, c.secretSynced) defer cancel() _, err := c.createRemoteClusterRestClient(defaultNs, "secret-name") @@ -1063,7 +1063,7 @@ func TestCreateRestClientError(t *testing.T) { return true, getTestSecret("secret-name", []byte("XXX")), nil }) - _, cancel := agtesting.StartInformers(m) + _, cancel := agtesting.StartInformers(m, c.secretSynced) defer cancel() _, err := c.createRemoteClusterRestClient(defaultNs, "secret-name")