Skip to content

Consistent timeout parameter #3159

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

Closed
CamJN wants to merge 9 commits into from
Closed

Consistent timeout parameter #3159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b1c7535
add timeout parameter to cli and driver
avoidik Jul 10, 2024
8a5814e
add initial timeout to driver
avoidik Sep 21, 2024
7424d39
clarify cli timeout definition
avoidik Sep 21, 2024
8397072
renew docs
avoidik Oct 3, 2024
f56bc0d
Make timeout work more like before change
CamJN Apr 30, 2025
8e99912
missing config parsing in docker-container driver factory
CamJN Apr 30, 2025
bf77130
check for bad timeout
CamJN Apr 30, 2025
0abd361
remove use of microsecond constant
CamJN May 1, 2025
a972eb1
fix BuildxController
CamJN May 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion builder/builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ type CreateOpts struct {
Use bool
Endpoint string
Append bool
Timeout time.Duration
}

func Create(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts CreateOpts) (*Builder, error) {
Expand Down Expand Up @@ -524,7 +525,7 @@ func Create(ctx context.Context, txn *store.Txn, dockerCli command.Cli, opts Cre
}

cancelCtx, cancel := context.WithCancelCause(ctx)
timeoutCtx, _ := context.WithTimeoutCause(cancelCtx, 20*time.Second, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
timeoutCtx, _ := context.WithTimeoutCause(cancelCtx, opts.Timeout, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
defer func() { cancel(errors.WithStack(context.Canceled)) }()

nodes, err := b.LoadNodes(timeoutCtx, WithData())
Expand Down
4 changes: 4 additions & 0 deletions builder/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"sort"
"strings"
"time"

"github.com/containerd/platforms"
"github.com/docker/buildx/driver"
Expand Down Expand Up @@ -39,6 +40,8 @@ type Node struct {
CDIDevices []client.CDIDevice
}

const defaultDriverTimeout = 120 * time.Second

// Nodes returns nodes for this builder.
func (b *Builder) Nodes() []Node {
return b.nodes
Expand Down Expand Up @@ -130,6 +133,7 @@ func (b *Builder) LoadNodes(ctx context.Context, opts ...LoadNodesOption) (_ []N
Platforms: n.Platforms,
ContextPathHash: b.opts.contextPathHash,
DialMeta: lno.dialMeta,
Timeout: defaultDriverTimeout,
Copy link
Member

@tonistiigi tonistiigi Apr 29, 2025

Choose a reason for hiding this comment

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

If the driver timeout is still constant 120s and can't be changed, wouldn't it be safer if this would be added in the driver pkg. Atm I think you could still theoretically call driver pkg forgetting to pass in InitConfig.Timeout and then possibly bad things can happen as it doesn't seem to be validated.

Copy link
Author

@CamJN CamJN Apr 30, 2025

Choose a reason for hiding this comment

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

I added a check in GetDriver that the Timeout is set to something > 0.

})
if err != nil {
node.Err = err
Expand Down
6 changes: 5 additions & 1 deletion commands/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"fmt"
"time"

"github.com/docker/buildx/builder"
"github.com/docker/buildx/driver"
Expand All @@ -27,6 +28,7 @@ type createOptions struct {
buildkitdFlags string
buildkitdConfigFile string
bootstrap bool
timeout time.Duration
// upgrade bool // perform upgrade of the driver
}

Expand Down Expand Up @@ -61,6 +63,7 @@ func runCreate(ctx context.Context, dockerCli command.Cli, in createOptions, arg
Use: in.use,
Endpoint: ep,
Append: in.actionAppend,
Timeout: in.timeout,
Copy link
Member

@tonistiigi tonistiigi Apr 29, 2025

Choose a reason for hiding this comment

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

Is it intentional that this is passed as a value rather than added to ctx? So this Create() call can still take more time than timeout.

Copy link
Author

@CamJN CamJN Apr 30, 2025

Choose a reason for hiding this comment

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

Addressing how the timeout is used:
In builder.Create that this builder.CreateOpts struct is passed to, the context.WithTimeoutCause is created using the opts.Timeout and the opts there is this builder.CreateOpts we are passing to builder.Create in the last argument here. So this Timeout is the one that is used.

Addressing where the Timeout comes from:
The in on this line comes from the createOptions struct passed to the runCreate function we are in, and that is populated in the createCmd function that calls us here. The options variable passed into runCreate has its timeout field populated from the rootOpts struct's timeout field before being passed into the function. The rootOpts var seems to be populated in the rootFlags function, including the timeout.

As for passing the timeout as a field in CreateOpts vs adding it to ctx, I don't know for sure why it was done that way, but I'd guess because nothing else in runCreate modifies the ctx. Not being much of a go user, I will take your word that the Create call can take more time than timeout but the slow part of runCreate seems bounded by the timeout just like before?

Copy link
Member

@tonistiigi tonistiigi May 1, 2025

Choose a reason for hiding this comment

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

I'd just add it to the context as with the other commands if we don't know the reason why this one should be different.

Copy link
Author

@CamJN CamJN May 1, 2025

Choose a reason for hiding this comment

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

Actually, I'm a bit confused what you mean. Upon closer inspection the rm, ls, inspect, and create commands all do it this way, vs the build command that's different. The build command gets the timeout from the control.ControlOptions inclusion in the buildOptions struct, but doesn't seem to use it anywhere, possibly because builds tend to take absolute ages?

As a non go user what does, "add it to the context" mean, and in what way is that different between the create command and the rm, ls, or inspect commands?

})
if err != nil {
return err
Expand All @@ -80,7 +83,7 @@ func runCreate(ctx context.Context, dockerCli command.Cli, in createOptions, arg
return nil
}

func createCmd(dockerCli command.Cli) *cobra.Command {
func createCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
var options createOptions

var drivers bytes.Buffer
Expand All @@ -96,6 +99,7 @@ func createCmd(dockerCli command.Cli) *cobra.Command {
Short: "Create a new builder instance",
Args: cli.RequiresMaxArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
options.timeout = rootOpts.timeout
return runCreate(cmd.Context(), dockerCli, options, args)
},
ValidArgsFunction: completion.Disable,
Expand Down
4 changes: 3 additions & 1 deletion commands/inspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
type inspectOptions struct {
bootstrap bool
builder string
timeout time.Duration
}

func runInspect(ctx context.Context, dockerCli command.Cli, in inspectOptions) error {
Expand All @@ -36,7 +37,7 @@ func runInspect(ctx context.Context, dockerCli command.Cli, in inspectOptions) e
}

timeoutCtx, cancel := context.WithCancelCause(ctx)
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, 20*time.Second, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, in.timeout, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
defer func() { cancel(errors.WithStack(context.Canceled)) }()

nodes, err := b.LoadNodes(timeoutCtx, builder.WithData())
Expand Down Expand Up @@ -180,6 +181,7 @@ func inspectCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
if len(args) > 0 {
options.builder = args[0]
}
options.timeout = rootOpts.timeout
return runInspect(cmd.Context(), dockerCli, options)
},
ValidArgsFunction: completion.BuilderNames(dockerCli),
Expand Down
6 changes: 4 additions & 2 deletions commands/ls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const (
type lsOptions struct {
format string
noTrunc bool
timeout time.Duration
}

func runLs(ctx context.Context, dockerCli command.Cli, in lsOptions) error {
Expand All @@ -60,7 +61,7 @@ func runLs(ctx context.Context, dockerCli command.Cli, in lsOptions) error {
}

timeoutCtx, cancel := context.WithCancelCause(ctx)
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, 20*time.Second, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, in.timeout, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
defer func() { cancel(errors.WithStack(context.Canceled)) }()

eg, _ := errgroup.WithContext(timeoutCtx)
Expand Down Expand Up @@ -97,14 +98,15 @@ func runLs(ctx context.Context, dockerCli command.Cli, in lsOptions) error {
return nil
}

func lsCmd(dockerCli command.Cli) *cobra.Command {
func lsCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
var options lsOptions

cmd := &cobra.Command{
Use: "ls",
Short: "List builder instances",
Args: cli.ExactArgs(0),
RunE: func(cmd *cobra.Command, args []string) error {
options.timeout = rootOpts.timeout
return runLs(cmd.Context(), dockerCli, options)
},
ValidArgsFunction: completion.Disable,
Expand Down
4 changes: 3 additions & 1 deletion commands/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ type rmOptions struct {
keepDaemon bool
allInactive bool
force bool
timeout time.Duration
}

const (
Expand Down Expand Up @@ -109,6 +110,7 @@ func rmCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command {
}
options.builders = args
}
options.timeout = rootOpts.timeout
return runRm(cmd.Context(), dockerCli, options)
},
ValidArgsFunction: completion.BuilderNames(dockerCli),
Expand Down Expand Up @@ -151,7 +153,7 @@ func rmAllInactive(ctx context.Context, txn *store.Txn, dockerCli command.Cli, i
}

timeoutCtx, cancel := context.WithCancelCause(ctx)
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, 20*time.Second, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
timeoutCtx, _ = context.WithTimeoutCause(timeoutCtx, in.timeout, errors.WithStack(context.DeadlineExceeded)) //nolint:govet,lostcancel // no need to manually cancel this context as we already rely on parent
defer func() { cancel(errors.WithStack(context.Canceled)) }()

eg, _ := errgroup.WithContext(timeoutCtx)
Expand Down
18 changes: 16 additions & 2 deletions commands/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"fmt"
"os"
"time"

debugcmd "github.com/docker/buildx/commands/debug"
historycmd "github.com/docker/buildx/commands/history"
Expand All @@ -22,6 +23,8 @@ import (
"github.com/spf13/pflag"
)

const defaultTimeoutCli = 20 * time.Second

func NewRootCmd(name string, isPlugin bool, dockerCli *command.DockerCli) *cobra.Command {
var opt rootOptions
cmd := &cobra.Command{
Expand Down Expand Up @@ -96,6 +99,7 @@ func NewRootCmd(name string, isPlugin bool, dockerCli *command.DockerCli) *cobra
type rootOptions struct {
builder string
debug bool
timeout time.Duration
}

func addCommands(cmd *cobra.Command, opts *rootOptions, dockerCli command.Cli) {
Expand All @@ -104,10 +108,10 @@ func addCommands(cmd *cobra.Command, opts *rootOptions, dockerCli command.Cli) {
cmd.AddCommand(
buildCmd(dockerCli, opts, nil),
bakeCmd(dockerCli, opts),
createCmd(dockerCli),
createCmd(dockerCli, opts),
dialStdioCmd(dockerCli, opts),
rmCmd(dockerCli, opts),
lsCmd(dockerCli),
lsCmd(dockerCli, opts),
useCmd(dockerCli, opts),
inspectCmd(dockerCli, opts),
stopCmd(dockerCli, opts),
Expand All @@ -134,4 +138,14 @@ func addCommands(cmd *cobra.Command, opts *rootOptions, dockerCli command.Cli) {
func rootFlags(options *rootOptions, flags *pflag.FlagSet) {
flags.StringVar(&options.builder, "builder", os.Getenv("BUILDX_BUILDER"), "Override the configured builder instance")
flags.BoolVarP(&options.debug, "debug", "D", debug.IsEnabled(), "Enable debug logging")

var timeoutDuration = defaultTimeoutCli
if value, ok := os.LookupEnv("BUILDX_TIMEOUT"); ok {
var err error
timeoutDuration, err = time.ParseDuration(value)
if err != nil {
timeoutDuration = defaultTimeoutCli
}
}
flags.DurationVar(&options.timeout, "timeout", timeoutDuration, "Override the default global timeout (as duration, for example 1m20s)")
}
10 changes: 8 additions & 2 deletions controller/control/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package control
import (
"context"
"io"
"time"

"github.com/docker/buildx/build"
cbuild "github.com/docker/buildx/controller/build"
Expand All @@ -13,7 +14,7 @@ import (
)

type BuildxController interface {
Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, inputs *build.Inputs, err error)
Build(ctx context.Context, options *ControlOptions, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, inputs *build.Inputs, err error)
// Invoke starts an IO session into the specified process.
// If pid doesn't match to any running processes, it starts a new process with the specified config.
// If there is no container running or InvokeConfig.Rollback is specified, the process will start in a newly created container.
Expand All @@ -22,5 +23,10 @@ type BuildxController interface {
Close() error
ListProcesses(ctx context.Context) (infos []*processes.ProcessInfo, retErr error)
DisconnectProcess(ctx context.Context, pid string) error
Inspect(ctx context.Context) *cbuild.Options
Inspect(ctx context.Context) *ControlOptions
}

type ControlOptions struct {
cbuild.Options
Timeout time.Duration
}
8 changes: 4 additions & 4 deletions controller/local/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type buildConfig struct {
// TODO: these two structs should be merged
// Discussion: https://github.com/docker/buildx/pull/1640#discussion_r1113279719
resultCtx *build.ResultHandle
buildOptions *cbuild.Options
buildOptions *control.ControlOptions
}

type localController struct {
Expand All @@ -41,13 +41,13 @@ type localController struct {
buildOnGoing atomic.Bool
}

func (b *localController) Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (*client.SolveResponse, *build.Inputs, error) {
func (b *localController) Build(ctx context.Context, options *control.ControlOptions, in io.ReadCloser, progress progress.Writer) (*client.SolveResponse, *build.Inputs, error) {
if !b.buildOnGoing.CompareAndSwap(false, true) {
return nil, nil, errors.New("build ongoing")
}
defer b.buildOnGoing.Store(false)

resp, res, dockerfileMappings, buildErr := cbuild.RunBuild(ctx, b.dockerCli, options, in, progress, true)
resp, res, dockerfileMappings, buildErr := cbuild.RunBuild(ctx, b.dockerCli, &options.Options, in, progress, true)
// NOTE: RunBuild can return *build.ResultHandle even on error.
if res != nil {
b.buildConfig = buildConfig{
Expand Down Expand Up @@ -118,6 +118,6 @@ func (b *localController) Close() error {
return nil
}

func (b *localController) Inspect(ctx context.Context) *cbuild.Options {
func (b *localController) Inspect(ctx context.Context) *control.ControlOptions {
return b.buildConfig.buildOptions
}
9 changes: 5 additions & 4 deletions docs/reference/buildx.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,11 @@ Extended build capabilities with BuildKit

### Options

| Name | Type | Default | Description |
|:------------------------|:---------|:--------|:-----------------------------------------|
| [`--builder`](#builder) | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| Name | Type | Default | Description |
|:------------------------|:-----------|:--------|:---------------------------------------------------------------------|
| [`--builder`](#builder) | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |


<!---MARKER_GEN_END-->
Expand Down
1 change: 1 addition & 0 deletions docs/reference/buildx_bake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ Build from a file
| `--push` | `bool` | | Shorthand for `--set=*.output=type=registry` |
| [`--sbom`](#sbom) | `string` | | Shorthand for `--set=*.attest=type=sbom` |
| [`--set`](#set) | `stringArray` | | Override target value (e.g., `targetpattern.key=value`) |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |


<!---MARKER_GEN_END-->
Expand Down
1 change: 1 addition & 0 deletions docs/reference/buildx_build.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Start a build
| [`--ssh`](#ssh) | `stringArray` | | SSH agent socket or keys to expose to the build (format: `default\|<id>[=<socket>\|<key>[,<key>]]`) |
| [`-t`](#tag), [`--tag`](#tag) | `stringArray` | | Name and optionally a tag (format: `name:tag`) |
| [`--target`](#target) | `string` | | Set the target build stage to build |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |
| [`--ulimit`](#ulimit) | `ulimit` | | Ulimit options |


Expand Down
1 change: 1 addition & 0 deletions docs/reference/buildx_create.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Create a new builder instance
| [`--name`](#name) | `string` | | Builder instance name |
| [`--node`](#node) | `string` | | Create/modify node with given name |
| [`--platform`](#platform) | `stringArray` | | Fixed platforms for current node |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |
| [`--use`](#use) | `bool` | | Set the current builder instance |


Expand Down
15 changes: 8 additions & 7 deletions docs/reference/buildx_debug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ Start debugger (EXPERIMENTAL)

### Options

| Name | Type | Default | Description |
|:----------------|:---------|:--------|:--------------------------------------------------------------------------------------------------------------------|
| `--builder` | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| `--invoke` | `string` | | Launch a monitor with executing specified command (EXPERIMENTAL) |
| `--on` | `string` | `error` | When to launch the monitor ([always, error]) (EXPERIMENTAL) |
| `--progress` | `string` | `auto` | Set type of progress output (`auto`, `plain`, `tty`, `rawjson`) for the monitor. Use plain to show container output |
| Name | Type | Default | Description |
|:----------------|:-----------|:--------|:--------------------------------------------------------------------------------------------------------------------|
| `--builder` | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| `--invoke` | `string` | | Launch a monitor with executing specified command (EXPERIMENTAL) |
| `--on` | `string` | `error` | When to launch the monitor ([always, error]) (EXPERIMENTAL) |
| `--progress` | `string` | `auto` | Set type of progress output (`auto`, `plain`, `tty`, `rawjson`) for the monitor. Use plain to show container output |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |


<!---MARKER_GEN_END-->
Expand Down
1 change: 1 addition & 0 deletions docs/reference/buildx_debug_build.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ Start a build
| `--ssh` | `stringArray` | | SSH agent socket or keys to expose to the build (format: `default\|<id>[=<socket>\|<key>[,<key>]]`) |
| `-t`, `--tag` | `stringArray` | | Name and optionally a tag (format: `name:tag`) |
| `--target` | `string` | | Set the target build stage to build |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |
| `--ulimit` | `ulimit` | | Ulimit options |


Expand Down
13 changes: 7 additions & 6 deletions docs/reference/buildx_dial-stdio.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,13 @@ Proxy current stdio streams to builder instance

### Options

| Name | Type | Default | Description |
|:----------------|:---------|:--------|:----------------------------------------------------------------------------------------------------|
| `--builder` | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| `--platform` | `string` | | Target platform: this is used for node selection |
| `--progress` | `string` | `quiet` | Set type of progress output (`auto`, `plain`, `tty`, `rawjson`). Use plain to show container output |
| Name | Type | Default | Description |
|:----------------|:-----------|:--------|:----------------------------------------------------------------------------------------------------|
| `--builder` | `string` | | Override the configured builder instance |
| `-D`, `--debug` | `bool` | | Enable debug logging |
| `--platform` | `string` | | Target platform: this is used for node selection |
| `--progress` | `string` | `quiet` | Set type of progress output (`auto`, `plain`, `tty`, `rawjson`). Use plain to show container output |
| `--timeout` | `duration` | `20s` | Override the default global timeout (as duration, for example 1m20s) |


<!---MARKER_GEN_END-->
Expand Down
Loading