fiaas / k8s

Python client library for the Kubernetes API
https://fiaas.github.io/
Apache License 2.0
39 stars 24 forks source link

Watcher: list, then watch to try to avoid yielding events for deleted resources #132

Closed oyvindio closed 5 months ago

oyvindio commented 6 months ago

It looks like bookmark events aren't being sent on the watch connection on the first connect, when the connection is started with resourceVersion=None. When the watcher reconnects and sends a value for resourceVersion, bookmark events are sent by the apiserver.

When bookmark events aren't received, the watcher will use the resourceVersion of the last event it saw when restarting the watch connection. There is an edge case where this can cause the watcher to yield stale events after reconnecting for the first time. For example if we assume a empty namespace where the following changes happen (in this order):

After that, the watch connection starts, and watcher would yield one event for the only existing resource;

when the watch connection eventually times out and restarts it would use resourceVersion=1, and watcher would yield;

since these events happened after resourceVersion 1.

Some options to avoid this may be;

1) Enable the streaming lists1 feature, where a bookmark event should be sent after the list of ADDED events for the existing resources at the time the watch connection starts. The watcher should probably not set last_seen_resource_version until receiving the first bookmark event in that case. However, this feature is only in alpha status as of Kubernetes 1.27.

2) Instead of relying on the initial list of events from the watch_list call, do a list call before starting watch connection, and use resourceVersion from the ListMeta returned from list call when starting watch connection, as suggested in the documentation2. This is the solution in this PR.

To be able to do this, a list_with_meta method is added on the ApiMixIn to support getting the ListMeta resourceVersion3

oyvindio commented 6 months ago

cba48a9 effectively includes a minor API change as it changes the signature of WatchBaseEvent.__init__() and WatchEvent.__init() to make it easier to extend the classes. In theory this could mean callers of the API may need to update their code, which isn't ideal. At the same time, WatchBaseEvent shouldn't need to be used by clients of this library, and WatchEvent is emitted by Model.watch_list and Watcher.watch, and clients should not usually need to create instances of it, except for maybe in tests. I'm not sure if that is good enough reason to include the change or not. There are some more details in the commit message.

perj commented 5 months ago

Copilot suggested using named arguments for the new version of WatchBaseEvent and WatchEvent and keeping the old parameters for backwards compatibility, setting a default of None to all arguments. But I'm not sure it's worth the effort, what do you think?

oyvindio commented 5 months ago

Copilot suggested using named arguments for the new version of WatchBaseEvent and WatchEvent and keeping the old parameters for backwards compatibility, setting a default of None to all arguments. But I'm not sure it's worth the effort, what do you think?

Yeah maybe that is a better approach than adding the from_dict factory function, since it preserves the init signature

oyvindio commented 5 months ago

Copilot suggested using named arguments for the new version of WatchBaseEvent and WatchEvent and keeping the old parameters for backwards compatibility, setting a default of None to all arguments. But I'm not sure it's worth the effort, what do you think?

Yeah maybe that is a better approach than adding the from_dict factory function, since it preserves the init signature

I tried to do this in 35fd364