Prevent race conditions by syncing cache on new Allocation elements #780
Conversation
markmandel
added
kind/bug
|
/cc @pooneh-m fyi. Apologies for not picking this up in code review 😞 Please let me know if you need to me explain what is going on here 👍 |
|
Build Succeeded 👏 Build Id: a37d64e9-010c-4755-9ef3-80455b67742c The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
| @@ -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) | |||
There was a problem hiding this comment.
if the controller has the WaitForCacheSync, is it really needed to call StartInformers that does the same thing (e.g. calling WaitForCacheSync)
There was a problem hiding this comment.
The WaitforcacheSync is called in the controller .Run() command - so just instantiating the controller doesn't fire the WaitforcacheSync.
And in tests (I've found anyway), you often don't want everything that happens inside the Run() command to fire.
This was following the patterns of the sample controller
|
LGTM |
| @@ -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.secretSynced) | |||
There was a problem hiding this comment.
agtesting.StartInformers(m, c.allocationPolicySynced, c.gameServerSynced, c.secretSynced) here and in other places?
There was a problem hiding this comment.
Nicely spotted! Yep - exactly. Pushing up a change now. 💯
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.
markmandel
force-pushed
the
bug/gameserverallocations-race
branch
from
2e633e4
to
2d5219d
Compare
|
Build Succeeded 👏 Build Id: 077c52d7-a2e3-4057-b168-ff9a326e0666 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
|
Build Succeeded 👏 Build Id: eeeaef75-d1d9-46e5-a56f-048718a230b2 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
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.