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.

Sign up for GitHub

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

Jump to bottom

KCM: specifies the upper-bound timeout limit for outgoing requests #99358

Merged
merged 1 commit into from Apr 8, 2021
Merged

KCM: specifies the upper-bound timeout limit for outgoing requests #99358

merged 1 commit into from Apr 8, 2021

Conversation

p0lyn0mial
Copy link
Contributor

@p0lyn0mial p0lyn0mial commented Feb 23, 2021 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR sets a timeout for the client-go client used by KCM. Previously no timeout was set. Requests without explicit timeout might potentially hang forever and lead to starvation of the application.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

KCM sets the upper-bound timeout limit for outgoing requests to 70s. Previously no timeout was set. Requests without explicit timeout might potentially hang forever and lead to starvation of the application.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@p0lyn0mial
KCM: specifies the upper-bound timeout limit for outgoing requests
662cc70 
Previously no timeout was set.
Requests without explicit timeout might potentially hang forever and lead to starvation of the application.
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 23, 2021
@p0lyn0mial
Copy link
Contributor Author

/assign @soltysh @sttts

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 23, 2021
tkashem
tkashem reviewed Feb 23, 2021
View reviewed changes
cmd/kube-controller-manager/app/options/options.go
@@ -434,6 +435,7 @@ func (s KubeControllerManagerOptions) Config(allControllers []string, disabledBy
kubeconfig.ContentConfig.ContentType = s.Generic.ClientConnection.ContentType
kubeconfig.QPS = s.Generic.ClientConnection.QPS
kubeconfig.Burst = int(s.Generic.ClientConnection.Burst)
kubeconfig.Timeout = 70 * time.Second // slightly bigger than the default server timeout which is 60 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, this would put timeout=70s to the request URI and the apiserver would truncate it to 60s :)

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/request.go#L494-L497

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, so the requests should be timed out after 60s if not then we know there was something that was holding up requests, for example, a proxy.

@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 23, 2021
@p0lyn0mial p0lyn0mial mentioned this pull request Feb 24, 2021
Closed
@p0lyn0mial
Copy link
Contributor Author

/retest

soltysh
soltysh approved these changes Mar 4, 2021
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
I'd probably provide a bit more details behind the importance of that timeout being set, given you've spend a great deal of time debugging these issues.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2021
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 16, 2021
@p0lyn0mial
Copy link
Contributor Author

/assing @sttts

for approval

@sttts
Copy link
Contributor

sttts commented Mar 23, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, soltysh, sttts

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 Mar 23, 2021
@k8s-ci-robot k8s-ci-robot merged commit c94a2f7 into kubernetes:master Apr 8, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Apr 8, 2021
@p0lyn0mial
Copy link
Contributor Author

@wojtek-t given what we have learned about timeouts and watch requests (#101022 and #100959) have you noticed an increased number of watch requests initiated from KCM on the master branch?

@wojtek-t
Copy link
Member

It doesn't hurt us that much (yet?), but until the timeout is common for watches and regular requests, I think we should revert this.

@p0lyn0mial
Copy link
Contributor Author

It doesn't hurt us that much (yet?), but until the timeout is common for watches and regular requests, I think we should revert this.

Thanks, I'm going to revert the PR.
Also, I am going to work on the "timeout for client-go" issue.

@p0lyn0mial p0lyn0mial mentioned this pull request Apr 14, 2021
Merged
@github-actions github-actions bot mentioned this pull request Apr 21, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkashem tkashem tkashem left review comments

@soltysh soltysh soltysh approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

@resouer resouer Awaiting requested review from resouer

Assignees

@soltysh soltysh

@sttts sttts

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 "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@p0lyn0mial @fedebongio @sttts @k8s-ci-robot @wojtek-t @soltysh @tkashem