Consistent timeout parameter #3159

CamJN · 2025-04-26T17:53:28Z

I rebased #2586 off master, and am willing to try to fix any issues that remain if they are mentioned here.

tonistiigi · 2025-04-29T04:31:30Z

driver/docker-container/driver.go

 			case <-ctx.Done():
 				return context.Cause(ctx)
-			case <-time.After(time.Duration(try*120) * time.Millisecond):
+			case <-time.After(d.Timeout / time.Microsecond):


see https://github.com/docker/buildx/pull/2586/files#diff-403916c02204229a19313f586f2fffa2536dde693a6e625bc4525f25be068ec9

So it looks like before it did a linear backoff with step=120ms:

try 1: After 120ms
try2: After 240ms
…
try 15: After 1,800ms

total: 14,400ms or 14.4s

if the new Timeout is the total time and we want to keep the linear backoff then I think the math should be:

total = 15*(1*step+15*step)/2 (total*2)/(15*16) = step

I implemented somethng like that.

tonistiigi · 2025-04-29T04:39:29Z

commands/create.go

 		Use:                 in.use,
 		Endpoint:            ep,
 		Append:              in.actionAppend,
+		Timeout:             in.timeout,


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.

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?

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.

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?

tonistiigi · 2025-04-29T04:43:11Z

builder/node.go

 					Platforms:       n.Platforms,
 					ContextPathHash: b.opts.contextPathHash,
 					DialMeta:        lno.dialMeta,
+					Timeout:         defaultDriverTimeout,


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.

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

tonistiigi · 2025-04-29T04:44:15Z

driver/remote/factory.go

 				return nil, err
 			}
 			d.defaultLoad = parsed
+		case "timeout":


Why is this specific for remote driver?

In the driver/kubernetes/factory.go New function the option parsing is done in processDriverOpts, the driver/docker-container/factory.go New function missing this option parsing seems like an oversight possibly, I added the timeout option parsing there too, the driver/docker/factory.go New function doesn't seem to do any option parsing at all so not doing any timeout parsing there is consistent with the existing code but I don't know if it is okay.

tonistiigi · 2025-05-01T00:43:15Z

driver/docker-container/driver.go

-	try := 1
+	var try uint64 = 1
+	var maxTries uint64 = 15
+	var step uint64 = max((uint64(d.Timeout.Abs().Milliseconds()) * 2) / (maxTries * (1 + maxTries)), 1)


I don't understand this construction. Why Abs(), why Milliseconds() , why maxTries ** 2?

I don't think this really makes much difference what the step is. If you set the timeout too short it will error anyway and checking more frequently than 120ms doesn't make it faster but might just increase overhead. Maybe better is to just remember how much time we have spent waiting on each iteration and if the last time.After(time.Duration(try * step) goes over Timeout then just clip it to smth like Timeout - 1 second so that we give the waiter the best chance to succeed before the timeout is reached.

My original comment was for division by time.Microsecond that didn't make any logical sense to me. I still see that in k8s driver.

Why Abs(), why Milliseconds() , why maxTries ** 2?

Abs() because duration is signed and the math doesn't work if the duration is negative, it's just a defensive coding thing.

Milliseconds() because I am working in milliseconds, like the original code before the PR did anything.

maxtries*(maxtries+1) is just part of the math for getting the sum of a number of elements in an arithmetic sequence, like the old timeouts (120ms, 240ms, 360ms, etc).

I just solved for the step size given a total duration, where before the step size was fixed and the total duration was implied (14,400ms). I could keep the step size the same and rather cap the total duration and not control for the number of tries, that's definitely an option.

I replaced the use of time.Microsecond in the k8s driver with /1000.

I switched to altering the number of tries instead of the step size

tonistiigi · 2025-05-01T00:45:58Z

commands/create.go

 		Use:                 in.use,
 		Endpoint:            ep,
 		Append:              in.actionAppend,
+		Timeout:             in.timeout,


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.

tonistiigi

After you are done and CI is green, you can also squash the commits.

Signed-off-by: avoidik <[email protected]> Signed-off-by: Viacheslav Vasilyev <[email protected]>

Signed-off-by: Viacheslav Vasilyev <[email protected]>

change try count not step size sill control for total time

avoidik · 2026-01-10T13:38:08Z

if you don't mind, I will try again ;)

CamJN changed the title ~~Pullrequests/consistent timeout parameter~~ Consistent timeout parameter Apr 26, 2025

github-actions bot assigned CamJN Apr 26, 2025

github-actions bot added area/driver/kubernetes area/driver/remote area/driver/docker-container area/driver area/cli area/controller area/builder area/docs labels Apr 26, 2025

This was referenced Apr 26, 2025

add timeout parameter to cli and driver #2586

Closed

Allow setting timeout when running docker buildx inspect moby/buildkit#5946

Open

tonistiigi reviewed Apr 29, 2025

View reviewed changes

tonistiigi reviewed May 1, 2025

View reviewed changes

avoidik and others added 8 commits May 1, 2025 00:23

add timeout parameter to cli and driver

b1c7535

Signed-off-by: avoidik <[email protected]> Signed-off-by: Viacheslav Vasilyev <[email protected]>

add initial timeout to driver

8a5814e

Signed-off-by: Viacheslav Vasilyev <[email protected]>

clarify cli timeout definition

7424d39

Signed-off-by: Viacheslav Vasilyev <[email protected]>

renew docs

8397072

Signed-off-by: Viacheslav Vasilyev <[email protected]>

Make timeout work more like before change

f56bc0d

change try count not step size sill control for total time

missing config parsing in docker-container driver factory

8e99912

check for bad timeout

bf77130

remove use of microsecond constant

0abd361

CamJN force-pushed the pullrequests/consistent-timeout-parameter branch from b6d2f1c to 4409317 Compare May 1, 2025 06:25

fix BuildxController

a972eb1

CamJN force-pushed the pullrequests/consistent-timeout-parameter branch from 4409317 to a972eb1 Compare May 1, 2025 06:46

CamJN closed this May 1, 2025

avoidik mentioned this pull request Jan 10, 2026

revisit: add timeout parameter to cli and driver #3585

Open

Consistent timeout parameter #3159

Consistent timeout parameter #3159

Uh oh!

Conversation

CamJN commented Apr 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CamJN Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

avoidik commented Jan 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CamJN Apr 30, 2025 •

edited

Loading

tonistiigi May 1, 2025 •

edited

Loading