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 timeout config in yurthub to handle those watch requests without … #1056

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

AndyEWang
Copy link
Contributor

What type of PR is this?

/kind enhancement

What this PR does / why we need it:

When yurthub takes charge of apiserver and if there is no timeoutSeconds parameter in watch requests, yurthub returns immediately. This causes the requester to retry watch request frantically. Also both yurthub and requester begin rolling logs.
So to add a new startup option for yurthub to define the default watch timeout and also give a default value(1800s like apiserver) to this option.

Does this PR introduce a user-facing change?

A new optional arg "--min-request-timeout" is added in yurthub startup command. If not set, the default value is 1800s.

@openyurt-bot openyurt-bot added the kind/enhancement kind/enhancement label Nov 11, 2022
@openyurt-bot openyurt-bot added the size/L size/L: 100-499 label Nov 11, 2022
@openyurt-bot
Copy link
Collaborator

Welcome @AndyEWang! It looks like this is your first PR to openyurtio/openyurt 🎉

@Congrool
Copy link
Member

I also notice this problem, and my case is calico that results in watch request flood. It's reasonable to add timeout for watch request when offline.

BTW, is there any reason for the value 1800s?

@AndyEWang
Copy link
Contributor Author

is there any reason for the value 1800s?

just refer to https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/ --min-request-timeout int Default: 1800

@codecov
Copy link

codecov bot commented Nov 11, 2022

Codecov Report

Merging #1056 (e5a8cd6) into master (5457b55) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1056      +/-   ##
==========================================
+ Coverage   49.45%   49.47%   +0.02%     
==========================================
  Files          96       96              
  Lines       13054    13056       +2     
==========================================
+ Hits         6456     6460       +4     
+ Misses       6061     6060       -1     
+ Partials      537      536       -1     
Flag Coverage Δ
unittests 49.47% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/yurthub/proxy/local/local.go 70.13% <100.00%> (+0.42%) ⬆️
pkg/util/iptables/iptables.go 87.67% <0.00%> (+0.54%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Congrool
Copy link
Member

Another question here, what's the behaviour of APIServer when handling the watch request with timeoutSeconds parameter?

For example, if we set --min-request-timeout=1800 for APIServer, and the coming watch request is /api/v1/nodes?watch=true&timeoutSeconds=32, how much time before APIServer timing it out?

@AndyEWang
Copy link
Contributor Author

apiserver respects the timeoutSeconds parameter and only use --min-request-timeout when timeoutSeconds is zero or not passed.

@Congrool
Copy link
Member

/lgtm

@rambohe-ch
Copy link
Member

/approve

@openyurt-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndyEWang, rambohe-ch

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

@openyurt-bot openyurt-bot added the approved approved label Nov 15, 2022
@openyurt-bot openyurt-bot merged commit 45266e0 into openyurtio:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved approved kind/enhancement kind/enhancement lgtm lgtm size/L size/L: 100-499
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants