Skip to content
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

Check if its possible to parse a chunked response #140

Closed
wants to merge 1 commit into from

Conversation

@ccamacho
Copy link

@ccamacho ccamacho commented Jun 26, 2019

This patch is a simple check for either read the chunks
of the response of use the standard read method.
Currently if it is not possible to use the 'read_chunked'
method the client fails.
This commit fixes #139

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jun 26, 2019

Welcome @ccamacho!

It looks like this is your first PR to kubernetes-client/python-base 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/python-base has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jun 26, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ccamacho
To complete the pull request process, please assign yliaog
You can assign the PR to them by writing /assign @yliaog in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@codecov-io
Copy link

@codecov-io codecov-io commented Jun 26, 2019

Codecov Report

Merging #140 into master will decrease coverage by 0.05%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #140      +/-   ##
=========================================
- Coverage   92.86%   92.8%   -0.06%     
=========================================
  Files          13      13              
  Lines        1345    1349       +4     
=========================================
+ Hits         1249    1252       +3     
- Misses         96      97       +1
Impacted Files Coverage Δ
watch/watch.py 98.85% <80%> (-1.15%) ⬇️

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...d20f2b5. Read the comment docs.

@ccamacho
Copy link
Author

@ccamacho ccamacho commented Jun 27, 2019

Hey folks @mbohlool @roycaihw, I'm hitting all over #139 and I wanted to know if this can be done. I already have patched my clusters and seemed to work fine.

Thanks!!

This patch is a simple check for either read the chunks
of the response of use the standard read method.

Currently if it is not possible to use the 'read_chunked'
method the client fails.
@ccamacho
Copy link
Author

@ccamacho ccamacho commented Jun 29, 2019

@mmazur @tripledes reviews are welcomed :)

@mmazur
Copy link

@mmazur mmazur commented Jul 1, 2019

Right, so let me start with the fact that I'm not familiar with any of this code and I'm not in a position to merge anything.

However, from doing some digging around it looks to me that the proper way to handle chunky responses is not with exceptions, but by emulating what the authoritative source (urllib3 itself) does. Which is this. Now, I'm not sure what the is_fp_closed() thing does and whether it's applicable in our case, but it looks to me like if self.chunked and self.supports_chunked_reads() is what we want to be doing here in lieu of catching exceptions.

@ccamacho
Copy link
Author

@ccamacho ccamacho commented Jul 1, 2019

@mmazur yeahp, you are actually right as we can handle the chunked case in that way. The main problem is that the urllib3 version we deliver does not have those methods.

rpm -qf /usr/lib/python2.7/site-packages/urllib3/response.py
python-urllib3-1.10.2-5.el7.noarch

Presently from 1.25.3

That's the main reason I decided to use an exception instead, just to avoid the dependency.

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Sep 29, 2019

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

@roycaihw
Copy link
Member

@roycaihw roycaihw commented Oct 1, 2019

may I ask what responses from Kubernetes APIs do not support read_chunked?

@ccamacho
Copy link
Author

@ccamacho ccamacho commented Oct 9, 2019

/remove-lifecycle stale

@ccamacho
Copy link
Author

@ccamacho ccamacho commented Oct 9, 2019

Hi @roycaihw thanks a lot for your reply! And sorry for not replying faster.
The issue in question is described in #139, basically when calling the watch() method the client breaks.

rpm -qf /usr/lib/python2.7/site-packages/urllib3/response.py
python-urllib3-1.10.2-5.el7.noarch

For example:
stream = watch.Watch().stream(crds.list_cluster_custom_object, DOMAIN, "v1", "guitars", resource_version=resource_version)

The problem is that the urllib version we ship does not have this method read_chunked, and handling it with an exception allows having backwards compatibility with older versions of urllib.

The fix is actually really simple and we are already using this fix but it would be awesome if we can include it by default as it affects any OpenShift and OKD distribution based in the 4.0 version.

Thanks!

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Jan 7, 2020

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

@ccamacho
Copy link
Author

@ccamacho ccamacho commented Jan 7, 2020

/remove-lifecycle stale

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Apr 6, 2020

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

@fejta-bot
Copy link

@fejta-bot fejta-bot commented May 6, 2020

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

@fejta-bot
Copy link

@fejta-bot fejta-bot commented Jun 5, 2020

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Jun 5, 2020

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants