-
Notifications
You must be signed in to change notification settings - Fork 71
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
Timeout related fixes #320
Conversation
Thanks for the PR. Can I ask what issue you want to resolve? |
Hi @tomplus , Thank you for your feedback. Sorry, you are right, I should have started with the problem. Here is the definition of a dummy custom resource (define-crd.yaml):
Here is a simple yaml file to add a custom resource called test1 (create-crd.yaml):
And here is a simple test application to watch these CRDs (watch_crd.py):
Here is the result of the test:
The backtrace appears 5mn after the program start, and is the same than the one observed in #259. After applying the first patch, there is no TimeoutError exception anymore, but the behavior is not what expected. Every 5mn, a new "add" log is displayed, however the object is added only once:
The next 2 patches aim to solve this new issue, but indeed I confused Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, your example helped me to understand the root cause.
On this page you can find differences between 2 types of timeout: https://github.com/kubernetes-client/python/blob/master/examples/watch/timeout-settings.md
DynamicClient shouldn't set timeout_second if it's not provided. Let's leave _request_timout unchanged (rest.py, client.py).
If timeout_second is not set Watch() should wait forever and reconnect if necessary. You get duplicated events because there is a bug and resource_version is not handled properly. I'll commit a fix for it in another PR.
Currently, when a dynamic client watcher is used, timeout_seconds is always passed to watcher.stream() from DynamicClient.watch(). Its value is None when the timeout argument was not passed by the user. The current check done on TimeoutError in the Watch.watch() method when awaiting from self.resp.content.readline() prevents to reopen the request in this case because it behaves differently if timeout_seconds was set to None or if timeout_seconds was not passed to the function. Do not pass the timeout_seconds argument in DynamicClient.watch() if timeout was not specified by the user. Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
This has to be merged with your changes too: #321 |
Hi, Thank you for the explanations and the reconnect fix. |
0820017
to
5c5e6a3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #320 +/- ##
==========================================
- Coverage 28.11% 28.11% -0.01%
==========================================
Files 779 779
Lines 92119 92122 +3
==========================================
Hits 25899 25899
- Misses 66220 66223 +3 ☔ View full report in Codecov by Sentry. |
I've merged my PR so you can test it from master branch. I'll release new version soon. |
Hi @tomplus, I confirm my problem is solved with the master branch. Many thanks! |
I'm happy to hear that. Thank you for your contribution too! |
Currently, when a dynamic client watcher is used, timeout_seconds is always passed to watcher.stream() from DynamicClient.watch(). Its value is None when the timeout argument was not passed by the user. The current check done on TimeoutError in the Watch.watch() method when awaiting from self.resp.content.readline() prevents to reopen the request in this case because it behaves differently if timeout_seconds was set to None or if timeout_seconds was not passed to the function. Do not pass the timeout_seconds argument in DynamicClient.watch() if timeout was not specified by the user. Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
This pull request contains 3 small fixes related to timeouts, seen during a watch operation.
I don't know if they can all be applied, especially the one in rest.py since this file is generated.