MAISTRA-2197: Push first update when cache is warm

[jwendell]

Change SMMR Controller behavior to only send the first update to
listeners when it has a list of members ready. In other words, when
cache is synced.

Before this, the first update was sent with only one hardcoded member:
The control plane namespace. Now it is sent with the full list of SMMR
members.

While on that, switch to our own log scope, to facilitate debugging.

[dgn]

This is kind of doing the wrong thing now. There are two cases we have to cover:

1. handler register before the cache is warm. In this case, send nothing because we don't have real data. As soon as data comes in, we'll send it anyway (because handler will be registered by that time)
2. handler register after cache has warmed up. Send current list of namespaces

What this implementation does is in case 1. it will send updates twice - once in the goroutine that waits for the cache, and once in the actual event coming in (as part of cache sync). In case 2 it will not send anything at all but leak a goroutine that waits on an already-closed channel.

I think it would be simpler to do the following:
at handler registration, check whether cache is warm (can be boolean protected by mutex). if cache is warm, send list of namespaces. if not, do nothing and just return. as soon as the cache has warmed up, the events will come in anyway and we'll send an update automatically.

[dgn]

isn't this reading from a channel that might be long closed?

[jwendell]

yes, that's the idea (reading from a closed channel is a nop). The channel remains open until the cache is warmed, hence it blocks. Once channel is closed, it doesn't block anymore and the behavior is the same we have currently.

[dgn]

oh yeah, you're right. so we're not leaking a goroutine, that's good. but aren't we still sending the same event twice?

[jwendell]

This is kind of doing the wrong thing now. There are two cases we have to cover:

1. handler register before the cache is warm. In this case, send nothing because we don't have real data. As soon as data comes in, we'll send it anyway (because handler will be registered by that time)

2. handler register after cache has warmed up. Send current list of namespaces

What this implementation does is in case 1. it will send updates twice - once in the goroutine that waits for the cache, and once in the actual event coming in (as part of cache sync). In case 2 it will not send anything at all but leak a goroutine that waits on an already-closed channel.

As explained above, the goroutine does not block.

I think it would be simpler to do the following:
at handler registration, check whether cache is warm (can be boolean protected by mutex). if cache is warm, send list of namespaces. if not, do nothing and just return. as soon as the cache has warmed up, the events will come in anyway and we'll send an update automatically.

"as soon as the cache has warmed up, the events will come in anyway" => Not quite true. This is exactly the problem we have right now. Events only come when something happen, that's why Rob added that initial push with the control plane.

I've tested this approach and it worked fine. A basic workflow is:

• SMMR CR already exists with 10 members
• Controller starts
• Listener A registers
• Listener B registers
• Cache is synced
• Send initial push to Listeners A and B
• Listener C registers
• Send initial push to listener C
• SMMR CR is updated
• Send 'updated' push normally to all listeners

[dgn]

"as soon as the cache has warmed up, the events will come in anyway" => Not quite true. This is exactly the problem we have right now. Events only come when something happen, that's why Rob added that initial push with the control plane.

that doesn't match my observations. when watching a resource, you'll get "add" events as part of the first sync for all resources that existed before starting to watch. the reason this was added was for another use case, which is registering a handler after the cache has warmed up, because then, as you said, you only get notified when things change - so the handler is never called if the SMMR is not updated

[dgn]

reducing my review to 'comment' to unblock this, as my assumption of the goroutine leaking was wrong. I still think we shouldn't send the event twice, and by not doing that we can make this simpler, too

[jwendell]

In my tests it did not send events twice. I'll test again.

[jwendell]

I was not getting updates twice because we only send an update if the members are different. Since the initial update and the normal add update are essentially the same, only one update was being sent to listeners.

I've changed the logic accordingly. Thanks for catching this!

[jwendell]

/retest

[maistra-bot]

In response to a cherrypick label: #291 failed to apply on top of branch "maistra-2.1":

Applying: MAISTRA-2197: Push first update when cache is warm
Using index info to reconstruct a base tree...
A	pkg/servicemesh/controller/memberroll/controller.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/servicemesh/controller/controller.go
CONFLICT (content): Merge conflict in pkg/servicemesh/controller/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 MAISTRA-2197: Push first update when cache is warm
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

[bison]

I think this is breaking things in 2.1 -- What if the SMMR doesn't exist? All controllers registered before the cache is synced will never get an update until one is created. That's a problem though, because they may need to know about the system namespace regardless.

Concrete example: we use the member-roll controller in the namespace controller. One thing the namespace controller is responsible for is creating the ConfigMap in each namespace that contains the Istio CA cert. The gateways need this to start.

So, if I deploy a fresh istiod, and I haven't created an SMMR, the namespace controller is never informed of the system namespace and doesn't create the ConfigMap. Now my gateway pods won't start. This also means the operator won't consider the installation finished and start reconciling SMMR updates.

I'm not sure I understand the original problem. Why do we care if we get an update with the system namespace before the others?

[jwendell]

@bison pasted from the jira issue:

"In my scenario, the SMMR is already "ready" with some configuredMembers already in it. I'm not touching it in any way. I'm just registering for updates and getting two pushes as a result."

Having two push updates when we only need one was the original issue. This is not optimal, for example, for IOR when there are lots of namespaces (200+). IOR relies on SMMR controller to update its internal state. IOR does an initial synchronization of Gateways and Routes on initialization. The problem was:

• There are 200 namespaces
• There are 200 Istio Gateways, one in each namespace
• There are 200 Openshift Routes, one for each Gateway
• When IOR starts, it gets only 1 namespace (control plane) as the first push
• It now removes all Routes because it thinks all 200 namespaces were removed from SMMR
• It gets a second push, with all 200 namespaces in it
• It recreates all 200 Routes.

We discussed this in a call and the consensus was that the fix would be to only send the first update when cache is synced. The scenario you described above was not take into consideration. We need to address it in a follow up then.

[bison]

Ah, okay. That makes sense. I guess it does kind of break the API to potentially only send the system namespace when the contract with listeners is SetNamespaces(), which you would assume would always be the full set.

I think as a quick fix we can just trigger an update for all listeners once caches are synced. Ultimately, it may be better to allow listeners to register add and remove handlers -- basically what the xns-informers NamespaceSet does.