event-watcher: don't parse resource version #12
Conversation
Parsing the resourceVersion as an integer is strictly prohibited by the Kubernetes API [1]. During a review of code that would be affected by non-numerical resourceVersions, I came upon this code. It looks like this code was using resource version comparison to only handle "new" objects. The Kubernetes watch API ensures that events on a watch stream are delivered once and only once, and always in order. Therefore, no check of this sort is necessary as each new event will always be unique and newer than any previous event. [1] https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions Signed-off-by: Steve Kuznetsov <[email protected]>
|
What happens if the controller is restarted? The reason for adding those checks was to avoid re-checking the same events every time the controller is restarted. Only the first time, it should go through all of them. There is room for many improvements as the core logic was never updated since it was written over three years ago. This repository has not been in active development for years, but I appreciate the effort to fix or improve anything related to it. |
|
@agalue good question about restarts! On process exit, you will lose your in-memory record of the previous version you saw anyway, so that won't be necessary. When the underlying informer gets a transient watch error and re-starts, it will start watching from that |
|
Apologize for the delay. The idea is to ensure that the service will only process the events generated after it started and ignore whatever events are already on the system to avoid processing duplicates. That's the reason for the logic. Without it, it will always process all the current events, meaning if you restart the service, you'd get duplicates. Makes sense? |
@agalue that is not how the informer library works, as explained in my previous comment. Consider as evidence that none of the core k8s controllers have implemented or ever will implement such filtering logic :) |
Parsing the resourceVersion as an integer is strictly prohibited by the Kubernetes API [1]. During a review of code that would be affected by non-numerical resourceVersions, I came upon this code.
It looks like this code was using resource version comparison to only handle "new" objects. The Kubernetes watch API ensures that events on a watch stream are delivered once and only once, and always in order. Therefore, no check of this sort is necessary as each new event will always be unique and newer than any previous event.
[1] https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions
Signed-off-by: Steve Kuznetsov [email protected]