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

Set a request timeout on kubectl commands #360

Merged
merged 3 commits into from
Jul 26, 2022
Merged

Set a request timeout on kubectl commands #360

merged 3 commits into from
Jul 26, 2022

Conversation

smlx
Copy link
Contributor

@smlx smlx commented Oct 12, 2021

What this PR does / why we need it:

Simple kubectl commands such as get, describe, and logs should return
quickly, but kubectl does not set a timeout on requests by default.

This change sets a 30s timeout by default on such kubectl commands to
ensure that these don't hang forever if the kubernetes API stops
responding, which is quite possible in CI environments where jobs may
cancelled and kubernetes clusters torn down while ct is running.

This timeout value is configurable via the new kubectl-timeout option.

Which issue this PR fixes

Hopefully closes: #332

Special notes for your reviewer:

n/a

@davidkarlsen
Copy link
Member

Would it make sense to have this configurable, falling back to a default if not defined?

@smlx
Copy link
Contributor Author

smlx commented Oct 13, 2021

I don't really think it is worth the extra complexity. The only reason to set this at all is to avoid an infinite timeout. If your API is taking 90s to return you probably have other problems. I can't imagine a scenario where you would want to configure the timeout on the kubectl get commands used by ct.

However of course if you disagree and think this needs to be configurable let me know and I'll take a look at updating the PR 🙂

90s is an arbitrary value, so if you think it should be higher or lower let me know.

@cpanato
Copy link
Member

cpanato commented Mar 14, 2022

+1 to have that configurable

smlx added 2 commits April 8, 2022 22:03
Simple kubectl commands such as get, describe, and logs should return
quickly, but kubectl does not set a timeout on requests by default.

This can mean kubectl hangs forever if the kubernetes API stops
responding during a request, which is quite possible in CI environments
where jobs may cancelled and kubernetes clusters torn down while ct is
running.

This change sets a configurable timeout on such kubectl commands, with a
default value of 30s.

Signed-off-by: Scott Leggett <[email protected]>
@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 8, 2022
@smlx
Copy link
Contributor Author

smlx commented Apr 8, 2022

Okay I added a configuration option for the kubectl timeout value.

Copy link
Member

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

lgtm

@davidkarlsen davidkarlsen requested a review from cpanato July 22, 2022 11:11
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

thanks!

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

Successfully merging this pull request may close these issues.

Log output for container in 'ct install' does not terminate
4 participants