Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Fixes kubernetes-client/python issue 1047 "ResponseNotChunked from watch" #231

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

dhague
Copy link
Contributor

@dhague dhague commented Apr 8, 2021

In recent versions of K8S (>1.16?), when a Watch.stream() call uses a
resource_version which is too old the resulting 410 error is wrapped in JSON
and returned in a non-chunked 200 response. Using resp.stream() instead of
resp.read_chunked() automatically handles the response being either chunked or
non-chunked.

Fix watch stream non-chunked response handling 

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Apr 8, 2021
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 8, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @dhague!

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 k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 8, 2021
@dhague dhague changed the title Fixes #1047 "ResponseNotChunked from watching namespaced configmap or secret" Fixes #1047 "ResponseNotChunked from watch" Apr 8, 2021
@dhague dhague changed the title Fixes #1047 "ResponseNotChunked from watch" Fixes kubernetes-client/python issue 1047 "ResponseNotChunked from watch" Apr 8, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 8, 2021
@dhague
Copy link
Contributor Author

dhague commented Apr 8, 2021

The build failure seems to be a flaky test unrelated to this PR, and has occurred in other commits on master

watch/watch.py Outdated Show resolved Hide resolved
@yliaog
Copy link
Contributor

yliaog commented Apr 8, 2021

please squash the commits

have you manually tested it in your env to verify the fix?

The documentation below says stream is a wrapper for read(), but does not mention read_chunked().

stream(amt=65536, decode_content=None)
A generator wrapper for the read() method.

…tch"

In recent versions of K8S (>1.16?), when a `Watch.stream()` call uses a
resource_version which is too old the resulting 410 error is wrapped in JSON
and returned in a non-chunked 200 response. Using `resp.stream()` instead of
`resp.read_chunked()` automatically handles the response being either chunked or
non-chunked.
@dhague
Copy link
Contributor Author

dhague commented Apr 8, 2021

  1. Commits are now squashed

  2. I have been testing this in my dev environment, and also in the CI pipeline for our app that uses the Python Kubernetes client. Some flaky tests in the CI pipeline are what led me to fixing this in the first place :-)

  3. As to the stream() method itself, you can see from the implementation below that it deals explicitly with both read() and read_chunked() cases - it does exactly what we want:

    def stream(self, amt=2 ** 16, decode_content=None):
        """
        A generator wrapper for the read() method. A call will block until
        ``amt`` bytes have been read from the connection or until the
        connection is closed.

        :param amt:
            How much of the content to read. The generator will return up to
            much data per iteration, but may return less. This is particularly
            likely when using compressed data. However, the empty string will
            never be returned.

        :param decode_content:
            If True, will attempt to decode the body based on the
            'content-encoding' header.
        """
        if self.chunked and self.supports_chunked_reads():
            for line in self.read_chunked(amt, decode_content=decode_content):
                yield line
        else:
            while not is_fp_closed(self._fp):
                data = self.read(amt=amt, decode_content=decode_content)

                if data:
                    yield data

@yliaog
Copy link
Contributor

yliaog commented Apr 8, 2021

thanks for pr

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit db50d02 into kubernetes-client:master Apr 8, 2021
@roycaihw roycaihw added the kind/bug Categorizes issue or PR as related to a bug. label Apr 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants