Skip to content

Allow timeouts to be configured in config#3459

Merged
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:timeout-config
Aug 19, 2019
Merged

Allow timeouts to be configured in config#3459
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:timeout-config

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Jul 26, 2019

This adds a singleton timeout package that will allow services and user
to configure timeouts in the daemon. When a service wants to use a
timeout, it should declare a const and register it's default value
inside an init() function for that package. When the default config
is generated, we can use the timeout package to provide the available
timeout keys so that a user knows that they can configure.

These show up in the config as follows:

[timeouts]
  "io.containerd.timeout.shim.cleanup" = "5s"
  "io.containerd.timeout.shim.load" = "5s"
  "io.containerd.timeout.shim.shutdown" = "300ms"
  "io.containerd.timeout.task.state" = "100ms"

Timeouts in the config are specified as a string and parsed as a duration.

Timeouts are very hard to get right and giving this power to the user to
configure things is a huge improvement. Machines can be faster and
slower and depending on the CPU or load of the machine, a timeout may
need to be adjusted.

Signed-off-by: Michael Crosby [email protected]

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 26, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 26, 2019

Codecov Report

Merging #3459 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3459      +/-   ##
==========================================
- Coverage   44.24%   44.23%   -0.01%     
==========================================
  Files         124      124              
  Lines       13732    13735       +3     
==========================================
  Hits         6076     6076              
- Misses       6725     6728       +3     
  Partials      931      931
Flag Coverage Δ
#linux 48.03% <0%> (-0.01%) ⬇️
#windows 39.85% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
services/server/server.go 1.22% <0%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a49df98...0d0dcb8. Read the comment docs.

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@jterry75
Copy link
Copy Markdown
Contributor

@kevpar - FYI this is similar to what we have been seeing.

Comment thread pkg/timeout/timeout.go Outdated
@ehazlett
Copy link
Copy Markdown
Member

I agree with @dmcgowan with naming GetContext -> WithContext -- otherwise LGTM

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jul 31, 2019

Build succeeded.

@crosbymichael
Copy link
Copy Markdown
Member Author

Updated

Comment thread services/server/config/config.go Outdated
This adds a singleton `timeout` package that will allow services and user
to configure timeouts in the daemon.  When a service wants to use a
timeout, it should declare a const and register it's default value
inside an `init()` function for that package.  When the default config
is generated, we can use the `timeout` package to provide the available
timeout keys so that a user knows that they can configure.

These show up in the config as follows:

```toml
[timeouts]
  "io.containerd.timeout.shim.cleanup" = 5
  "io.containerd.timeout.shim.load" = 5
  "io.containerd.timeout.shim.shutdown" = 3
  "io.containerd.timeout.task.state" = 2

```

Timeouts in the config are specified in seconds.

Timeouts are very hard to get right and giving this power to the user to
configure things is a huge improvement.  Machines can be faster and
slower and depending on the CPU or load of the machine, a timeout may
need to be adjusted.

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Copy Markdown
Member Author

Updated to specify timeouts as a time.duration ( string in the config)

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Aug 13, 2019

Build succeeded.

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

still LGTM

@estesp estesp merged commit fc9335d into containerd:master Aug 19, 2019
@thaJeztah
Copy link
Copy Markdown
Member

@jterry75 see above; those commits are from 2019 😅 (not sure why they showed up in timelines / notifications)

@jterry75
Copy link
Copy Markdown
Contributor

jterry75 commented Apr 7, 2022

LOL!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants