This change is 

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@azweb76 please use quay.io/aledbf/nginx-ingress-controller:0.326 (new value is 1024)

Correct me if Im wrong. On startup all events are written to the ring. Then later popped off the ring.
https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/nginx.go#L246-L273

Given this, only a maximum of 1024 events will be tracked in sync. If I understand, the ring is intialized as 1024/4096/etc and if the cluster exceeds this, the first N items are dropped before they are processed.

Seems like we need to start processing the channel ring before we append to it so we can treat it like a buffer on initial startup.

Given this, only a maximum of 1024 events will be tracked in sync. If I understand, the ring is intialized as 1024/4096/etc and if the cluster exceeds this, the first N items are dropped before they are processed.

Yes. That does not mean we are not handling the events. Keep in mind we use a work queue for the update of the configuration and we discard all the event in a time window (all the events in the time lapse we start and finish the update).

Seems like we need to start processing the channel ring before we append to it so we can treat it like a buffer on initial startup.

This is one example of the chicken and the egg problem, without the store we don't have events and without the syncQueue we cannot start to process updates but syncQueue depends on the store.

Using the ring channel is the right solution because it allows us to discard the some of the initial events

Sure. After it has started. But this loop is what processes those items and since the loop doesnt happen until all initial events are loaded, only up to the updateCh limit is processed.

https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/controller/nginx.go#L302
https://github.com/eapache/channels/blob/master/ring_channel.go#L92

@azweb76 what are you proposing exactly?

I greatly appreciate the effort you're putting into this fix. I just want to make sure we're not introducing another bug. You certainly know more about this than I do, so I will be quiet now. Thanks again!

I greatly appreciate the effort you're putting into this fix

The update channel was introduced because of the store refactor. This is something we needed because it was almost impossible to have tests for the informers and all the logic behind the sync process.
So I have to fix this, this is not optional :)

The issue in 2022 is related to the number of events we receive at startup that causes a write contention in the channel (we exceed the buffer size). The change introduced here allows us to discard events when we exceed the defined size. As I mentioned previously we don't need all the events (from the start) but at the same time, we cannot run any filter at this point because of the lack of context.

If you run the test image and increase the log level to 3 using the flag --v=3 you should see lots of "skipping %v sync (%v > %v)". That means those events in the work queue where received in the middle of an update and are not really required to process (this avoids unnecessary reloads)

awesome. Looking forward to the release. 😄