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

add documentation for the server & client side timeout #1467

Conversation

Priyankasaggu11929
Copy link
Contributor

What type of PR is this?

/kind documentation

What this PR does / why we need it:

This PR documents information about server-side & client-side timeout in the examples/pod_namespace_watch.py example script.

Which issue(s) this PR fixes:

Fixes #1402

Does this PR introduce a user-facing change?

NONE

Signed-off-by: Priyanka Saggu priyankasaggu11929@gmail.com

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 14, 2021
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 2021
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from 1d1fcfa to 8bdb484 Compare May 14, 2021 13:45
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
examples/pod_namespace_watch.py Outdated Show resolved Hide resolved
@Priyankasaggu11929
Copy link
Contributor Author

Thank you @yliaog for all the pointers.

I'll read through the links to understand the concepts better, & will update the PR.

@Priyankasaggu11929 Priyankasaggu11929 changed the title add documentation for the server & client side timeout [WIP] add documentation for the server & client side timeout May 15, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2021
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from 8bdb484 to 8fa80dc Compare May 31, 2021 06:43
@k8s-ci-robot 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 31, 2021
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from 8fa80dc to 2b40cdf Compare May 31, 2021 06:44
@Priyankasaggu11929 Priyankasaggu11929 changed the title [WIP] add documentation for the server & client side timeout add documentation for the server & client side timeout May 31, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2021
@Priyankasaggu11929
Copy link
Contributor Author

@yliaog, I've tried to redo the previous attempt of documentation, adding more information after reading & understanding through the links/pointers you provided above.

Please review. Thank you!

@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from 2b40cdf to 9bb536c Compare May 31, 2021 11:56
examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
- *[kubernetes/apiserver/pkg/endpoints/handlers/get.go#L258](https://github.com/kubernetes/apiserver/blob/92392ef22153d75b3645b0ae339f89c12767fb52/pkg/endpoints/handlers/get.go#L258)*
- *[kubernetes/release-1.1/docs/admin/kube-apiserver.md](https://github.com/kubernetes/kubernetes/blob/release-1.1/docs/admin/kube-apiserver.md)*

- In case of a network outage, this timeout value will have no effect & the client will hang indefinitely without raising any exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of network outage would cause this? it hangs when not accounting for the client side timeout described below, right?

Copy link
Contributor Author

@Priyankasaggu11929 Priyankasaggu11929 Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re:

what kind of network outage would cause this?

I'm not sure how to define or specify the what kind of network outage part. Could you please phrase it or point me somewhere that talks more about the kind of network outage, I'll read more & try to add a proper definition here.

Thank you!

Re:

It hangs when not accounting for the client side timeout described below, right?

I might be wrong with my understanding, but so far, what I understood is ~

that it is the server side timeout (i.e. value of argument timeout_seconds, that would have no effect in case of the said network outage),

but in the same network outage situation, if there is a client-side timeout value set (i.e. a value for argument _requests_timeout) then the client would honor that timeout duration value & would wait for the same before dropping the connection.

Just a pointer from this comment here ~ #1148 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yliaog, no hurries at all. Just pointing in case, it was missed!

could you please take a look at this review comment too, as what more could be done to improve this section here?

Thank you so much!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think probably add the condition that client side timeout is not specified, something like below:

this timeout value will have no effect & the client will hang indefinitely without raising any exception if client side timeout (see below) is not specified

Copy link
Contributor Author

@Priyankasaggu11929 Priyankasaggu11929 Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yliaog, Thank you for the pointers.

I'm rephrasing it as following: ~

In case of a network outage, the server side timeout value will have no effect & the client will hang indefinitely without raising any exception. Note, that this is the case provided when there is no other client-side timeout (i.e., _request_timeout) value specified.

(See the section below for information on client side timeout)

examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
examples/watch/timeout-settings.md Show resolved Hide resolved
examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from 9bb536c to fc95388 Compare June 2, 2021 03:45
examples/watch/timeout-settings.md Show resolved Hide resolved
examples/watch/timeout-settings.md Outdated Show resolved Hide resolved
Signed-off-by: Priyanka Saggu <priyankasaggu11929@gmail.com>
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the documet-server-and-client-side-timeout branch from fc95388 to bfb3222 Compare June 2, 2021 18:03
@Priyankasaggu11929
Copy link
Contributor Author

@yliaog, I've updated the PR with the suggested review changes.

@yliaog
Copy link
Contributor

yliaog commented Jun 2, 2021

thanks for the pr
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Priyankasaggu11929, 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2021
@Priyankasaggu11929
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@Priyankasaggu11929: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

@roycaihw
Copy link
Member

travis-ci.org is in read-only mode

@roycaihw roycaihw merged commit 0543412 into kubernetes-client:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. 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.

Document client-side timeouts in watch
4 participants