Retry expired watches by mitar · Pull Request #133 · kubernetes-client/python-base

mitar · 2019-05-12T19:36:52Z

This is a followup to #102 and fixes#57.

mitar · 2019-05-12T19:38:54Z

codecov-io · 2019-05-12T19:44:54Z

Codecov Report

Merging #133 into master will decrease coverage by 0.23%.
The diff coverage is 83.33%.

@@ Coverage Diff @@## master #133 +/- ##
==========================================
- Coverage 92.86% 92.62% -0.24% 
==========================================
Files 13 13 Lines 1345 1356 +11 ==========================================
+ Hits 1249 1256 +7 - Misses 96 100 +4

Impacted Files	Coverage Δ
watch/watch_test.py	`97.09% <83.33%> (-1.61%)`	⬇️
watch/watch.py	`97.84% <83.33%> (-2.16%)`	⬇️
config/kube_config_test.py	`94.82% <0%> (-0.12%)`	⬇️
config/kube_config.py	`86.37% <0%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 474e9fb...5cc28ea. Read the comment docs.

ccamacho · 2019-06-29T04:57:04Z

It would be nice to have those 3 commits squashed into 1

mitar · 2019-06-29T05:01:42Z

Doesn't GitHub support that when merging the PR?

micw523 · 2019-06-29T11:05:43Z

Manually yes, but we’re using bots to merge the commits so usually the commits need to be squashed.

…

在 2019年6月29日，01:01，Mitar ***@***.***> 写道： Doesn't GitHub support that when merging the PR? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

mitar · 2019-06-29T18:49:11Z

Done.

roycaihw · 2019-07-30T17:56:08Z

/cc

micw523 · 2019-07-31T14:13:45Z

- yield self.unmarshal_event(line, return_type)
+ event = self.unmarshal_event(line, return_type)
+ if isinstance(event, dict) and event['type'] == 'ERROR':
+ obj = event['raw_object']


Is it safe to assume the APIs won't change for accessing event['raw_object']['code'], @roycaihw ?

The contract is that apiserver will always send a watch event of ERROR type with object of metav1.Status type.
We can check if the dict has the key code and handle non-existence like what client-go does

roycaihw

looks good in general. One major question about the retry behavior

roycaihw · 2019-08-01T17:55:46Z

- yield self.unmarshal_event(line, return_type)
+ event = self.unmarshal_event(line, return_type)
+ if isinstance(event, dict) and event['type'] == 'ERROR':
+ obj = event['raw_object']


The contract is that apiserver will always send a watch event of ERROR type with object of metav1.Status type.
We can check if the dict has the key code and handle non-existence like what client-go does

roycaihw · 2019-08-01T18:01:23Z

+ # but only if we have not already retried.
+ if not retry_after_410 and obj['code'] == 410:
+ retry_after_410 = True
+ break


we should not retry the watch request with the same RV when it's already gone in apiserver
we can retry on 500 and 504 according to the client-go implementation

@yliaog pointed out that we need to be careful about the code structure here. We want to be consistent to kubernetes client-go in terms of what a watch is capable of doing
In client-go, a watch function call doesn't retry on err. A client can implement their own handling for watch errors
client-go provides tooling like informer and retrywatcher for clients to leverage, which have retry logics for watch errors
We should be consistent and provide the same choices to our user

But that would be backwards incompatible with current offering by Python package? It already supports retrying. So I think for Python it seems we already made decision that watch is retrying.

roycaihw · 2019-08-01T18:07:15Z

 return data
 js['raw_object'] = js['object']
- if return_type:
+ if return_type and js['type'] != 'ERROR':


this is where we fail to deserialize the response. Good catch

dmateusp · 2019-11-18T15:35:34Z

is this good to go ?

jinchihe · 2019-12-19T09:26:28Z

Can someone review and approve the PR? we meet the problem now... Thanks.

fejta-bot · 2020-03-18T17:37:01Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

mitar · 2020-03-18T18:24:18Z

/remove-lifecycle stale

mel1nn · 2020-05-12T07:54:20Z

Could you fix the merge conflict please ?

codecov-commenter · 2020-05-27T19:27:58Z

Codecov Report

Merging #133 into master will decrease coverage by 0.19%.
The diff coverage is 83.33%.

@@ Coverage Diff @@## master #133 +/- ##
==========================================
- Coverage 92.69% 92.50% -0.20% 
==========================================
Files 13 13 Lines 1519 1547 +28 ==========================================
+ Hits 1408 1431 +23 - Misses 111 116 +5

Impacted Files	Coverage Δ
watch/watch.py	`97.84% <83.33%> (-2.16%)`	⬇️
watch/watch_test.py	`97.09% <83.33%> (-1.61%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ec060...1eca201. Read the comment docs.

mitar · 2020-05-27T19:33:45Z

Done.

mitar · 2020-06-23T17:38:04Z

I have updated this. Please review it before it gets rotten again.

mel1nn · 2020-06-29T14:22:18Z

@mbohlool ?

mel1nn · 2020-07-15T15:15:16Z

@roycaihw ?

yliaog · 2020-07-15T19:29:21Z

thanks for the PR.

/lgtm
/approve

k8s-ci-robot · 2020-07-15T19:29:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mitar, yliaog

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yliaog]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

PR kubernetes-client#133 introduces the usage of `http` module for checking the status code for `GONE` HTTP status. However, this doesn't work in Python 2.7. This commit checks if the interpreter is Python 2 and imports the status code from `httplib` module instead and unifies the approach to the checks. Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>

k8s-ci-robot requested review from mbohlool and roycaihw May 12, 2019 19:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2019

k8s-ci-robot assigned mbohlool, micw523 and roycaihw May 12, 2019

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2019

mitar mentioned this pull request Jun 29, 2019
Watch stream should handle HTTP error before unmarshaling event #57
Closed

mitar force-pushed the retry-watch branch from c5e87f7 to 5cc28eaCompare June 29, 2019 18:49

micw523 reviewed Jul 31, 2019
View reviewed changes

roycaihw reviewed Aug 1, 2019
View reviewed changes

micw523 mentioned this pull request Oct 1, 2019
IncompleteRead while watching kubernetes-client/python#972
Closed

cben mentioned this pull request Oct 18, 2019
Using watches and handling timeouts kontena/k8s-client#92
Closed

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 18, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 18, 2020

mitar force-pushed the retry-watch branch from 5cc28ea to 1eca201Compare May 27, 2020 19:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2020

DrAuYueng mentioned this pull request Jun 24, 2020
resource version reset default when event object resource version inv… #199
Closed

yliaog reviewed Jul 15, 2020
View reviewed changes

Comment threadwatch/watch.py Outdated

mitar force-pushed the retry-watch branch from 1eca201 to 7484f4fCompare July 15, 2020 18:06

yliaog reviewed Jul 15, 2020
View reviewed changes

Comment threadwatch/watch.py Outdated

Retry watch if request expires.
06e48c5

mitar force-pushed the retry-watch branch from 7484f4f to 06e48c5Compare July 15, 2020 19:27

k8s-ci-robot assigned yliaog Jul 15, 2020

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020

k8s-ci-robot merged commit ded3d12 into kubernetes-client:masterJul 15, 2020

palnabarun mentioned this pull request Jul 16, 2020
Update OpenAPI generator and the client kubernetes-client/python#1188
Merged

alaiou mentioned this pull request Oct 26, 2020
Support Kuberenetes python live v12 in KubernetesExecutor - ApiException/410 errors apache/airflow#11841
Closed

PoliM mentioned this pull request Oct 27, 2020
Watching stops working after 10 minutes kiwigrid/k8s-sidecar#85
Closed

oliver-goetz mentioned this pull request Jan 28, 2021
cr-syncer does not retry on failed updates googlecloudrobotics/core#64
Closed

This was referenced Feb 13, 2021
Perform automatic retries for 410s during watches tomplus/kubernetes_asyncio#136
Closed
resource_version set incorrectly while watching list_* functions tomplus/kubernetes_asyncio#135
Open

chrisayoub mentioned this pull request Feb 22, 2021
Fix bug with Watch and 410 retries #227
Merged

zzstoatzz mentioned this pull request Dec 7, 2022
Prefect agent raises InvalidChunkLength while streaming Kubernetes logs PrefectHQ/prefect#7653
Closed
4 tasks

thesuperzapper mentioned this pull request Nov 5, 2024
[backend] Metadata writer pod always restarting kubeflow/pipelines#8200
Closed

rijulsharma7 mentioned this pull request Jul 5, 2026
feat: Handle HTTP 429 TooManyRequests (storage is re-initialising) in Watch with exponential backoff tomplus/kubernetes_asyncio#431
Open

Uh oh!

Conversation

mitar commented May 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitar commented May 12, 2019

Uh oh!

codecov-io commented May 12, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ccamacho commented Jun 29, 2019

Uh oh!

mitar commented Jun 29, 2019

Uh oh!

micw523 commented Jun 29, 2019 via email

Uh oh!

mitar commented Jun 29, 2019

Uh oh!

roycaihw commented Jul 30, 2019

Uh oh!

micw523Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

roycaihwAug 1, 2019

Choose a reason for hiding this comment

Uh oh!

roycaihw left a comment

Choose a reason for hiding this comment

Uh oh!

roycaihwAug 1, 2019

Choose a reason for hiding this comment

Uh oh!

roycaihwAug 1, 2019

Choose a reason for hiding this comment

Uh oh!

roycaihwOct 1, 2019

Choose a reason for hiding this comment

Uh oh!

mitarDec 19, 2019

Choose a reason for hiding this comment

Uh oh!

roycaihwAug 1, 2019

Choose a reason for hiding this comment

Uh oh!

dmateusp commented Nov 18, 2019

Uh oh!

jinchihe commented Dec 19, 2019

Uh oh!

fejta-bot commented Mar 18, 2020

Uh oh!

mitar commented Mar 18, 2020

Uh oh!

mel1nn commented May 12, 2020

Uh oh!

codecov-commenter commented May 27, 2020

Codecov Report

Uh oh!

mitar commented May 27, 2020

Uh oh!

mitar commented Jun 23, 2020

Uh oh!

mel1nn commented Jun 29, 2020

Uh oh!

mel1nn commented Jul 15, 2020

Uh oh!

Uh oh!

Uh oh!

yliaog commented Jul 15, 2020

Uh oh!

k8s-ci-robot commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

mitar commented May 12, 2019•
edited
Loading

codecov-io commented May 12, 2019•
edited
Loading