Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAISTRA-2262: Make sure IOR respond to all events #312

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

jwendell
Copy link
Member

@jwendell jwendell commented Apr 9, 2021

Even those that arrive while IOR is performing its initial
sync. Previously those events were ignored.

Manual backport of #311

Copy link

@knrc knrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you have addressed the race between the incoming events and the initial sync it looks as if you have introduced another by switching to namespaces instead of r.namespaces. I think your goroutine needs to grab the namespace lock around initialSync and use the current r.namespaces otherwise you could miss SMMR events

@knrc
Copy link

knrc commented Apr 10, 2021

While you have addressed the race between the incoming events and the initial sync it looks as if you have introduced another by switching to namespaces instead of r.namespaces. I think your goroutine needs to grab the namespace lock around initialSync and use the current r.namespaces otherwise you could miss SMMR events

Looking further into the code it doesn't look as if you take any notice of the additional updates after the initial sync, other than to note the namespaces, which makes this change okay although I think you are going to face other issues by doing this. The code is still querying pods and services for every namespace and it relies on the UpdateNamespace call being invoked in time for it to do that which may not always happen, if so you will possibly miss a namespace update when you process the config updates for the gateways.

@jwendell
Copy link
Member Author

I think your concern is covered in the unit tests (see the 2.1 PR).
When a new namespace is added to SMMR, and it contains a gateway, two things happen:
UpdateNamespaces() is called, amd it just updates r.namespaces.
handleEvent() is called, eventually calls findService() which reads r.namespaces

The reason I switched from r.namespaces to namespaces in initialSync() (within the first UpdateNamespaces() call) was to deal with this:

  • first UpdateNamespaces() is called, say, with ns1, ns2.
  • initialSync() is invoked in a goroutine
  • UpdateNamespaces() is called again with ns1, ns2, ns3
  • when initialSync() actually runs, it would read the new set of namespaces, instead of the initial set.

@knrc
Copy link

knrc commented Apr 11, 2021

I think your concern is covered in the unit tests (see the 2.1 PR).
When a new namespace is added to SMMR, and it contains a gateway, two things happen:
UpdateNamespaces() is called, amd it just updates r.namespaces.
handleEvent() is called, eventually calls findService() which reads r.namespaces

The issue I think you will face is that these could come in the other way around, i.e. you could get a call to handleEvent before the related UpdateNamespaces is called. The risk of the race is small but I think it does still exist.

Even those that arrive while IOR is performing its initial
sync. Previously those events were ignored.

Manual backport of maistra#311
- Handle ADD event only for namespaces that belong to SMMR
- Check the result of cache.WaitForNamedCacheSync()
@jwendell
Copy link
Member Author

@knrc I just backported the 2.1 commits here.

@knrc
Copy link

knrc commented Apr 12, 2021

/test unit

@maistra-bot maistra-bot merged commit bcafb04 into maistra:maistra-2.0 Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants