Share disk usage computation results between concurrent invocations by rvolosatovs · Pull Request #42715 · moby/moby

rvolosatovs · 2021-08-06T15:04:00Z

- What I did
Share disk usage computation results between concurrent invocations instead of throwing an error

- How I did it

Use x/sync/singleflight.Group, which ensures computations are simultaneously performed by at most one goroutine and the results are propagated to all goroutines simultaneously calling the method.
Extract the disk usage computation functionality for containers and images for consistency with other object types and better separation of concerns. It also fits nicely with the current design.

- How to verify it
E.g.

docker system df& docker system df& docker system df

Or:

curl -s --unix-socket /var/run/docker.sock 'http://localhost/system/df?types=image'& curl -s --unix-socket /var/run/docker.sock 'http://localhost/system/df?types=container'& curl -s --unix-socket /var/run/docker.sock 'http://localhost/system/df?types=volume'& curl -s --unix-socket /var/run/docker.sock 'http://localhost/system/df?types=build-cache'& curl -s --unix-socket /var/run/docker.sock 'http://localhost/system/df?types=image&type=container'

Such invocations do not error anymore, but just return the result once computed by one of the goroutines

** description for changeling**

The `GET /system/df` endpoint can now be used concurrently. If a request is made
to the endpoint while a calculation is still running, the request will receive
the result of the already running calculation, once completed. Previously, an
error (`a disk usage operation is already running`) would be returned in this
situation.

cpuguy83 · 2021-08-06T18:29:04Z

Seems like we can use the singleflight package in x/sync.
https://pkg.go.dev/golang.org/x/sync@v0.0.0-20210220032951-036812b2e83c/singleflight#Group.DoChan

rvolosatovs · 2021-08-09T10:51:58Z

Thanks, didn't know about existence of this package!
Added a x/sync/singleflight-based implementation in 57b7519.
I do find the previous approach simpler and easier to read, mostly due to the lack of context handling in singleflight. Also, note that the returned channel is never closed and would leak a goroutine if we did not ensure a read on caller side, which means we have to start a goroutine on ctx.Done().
A possible minor advantage, is that we can reuse the "group" for e.g. improving prune UX.
I'm fine with both, with a slight preference to the original due to the above. (note, that for "visibility" of the called function, the function could be passed as argument to Do, just like with singleflight, and, in fact, that was done in the first iteration of this package)

Does it maybe make sense to add a wrapper for singleflight, which would add easy-to-use context support? (basically just wrapping the select block)

What do you think? @cpuguy83 @thaJeztah

thaJeztah

I do find the previous approach simpler and easier to read, mostly due to the lack of context handling in singleflight.

Yeah, the handling is slightly more complex. That said, I do like that we won't have to maintain yet-another-package.

the returned channel is never closed and would leak a goroutine if we did not ensure a read on caller side, which means we have to start a goroutine on ctx.Done().

That's a bit ugly, yes. @cpuguy83 any other suggestions for that?

A possible minor advantage, is that we can reuse the "group" for e.g. improving prune UX.

Ah, just left a comment about that. Was your idea to reuse the same groups, or to use the same approach ?

Does it maybe make sense to add a wrapper for singleflight, which would add easy-to-use context support? (basically just wrapping the select block)

Always a bit cautious with abstracting away things too much. You mean something like;

package singleflight

import (
    "context"

    "golang.org/x/sync/singleflight"
)

func Do(ctx context.Context, sf *singleflight.Group, key string, fn func() (interface{}, error)) (interface{}, error)
	ch := sf.DoChan(key, fn)
	select {
	case <-ctx.Done():
		go func() { <-ch }()
		return nil, ctx.Err()
	case res := <-ch:
		if res.Err != nil {
			return nil, res.Err
		}
		return res.Val, nil
	}
}

daemon/disk_usage.go

thaJeztah · 2021-08-09T12:53:29Z

volume/service/service.go

+	ds                ds
+	pruneRunning      int32
+	eventLogger       VolumeEventLogger
+	singleflightGroup singleflight.Group


perhaps just singleFlight (slightly shorter, and reduces reformatting/reflowing other lines)

Or (if we will be implement the same for pruning) perhaps make the singleFlight more descriptive to what it's for. e.g. usage (which would then be called as usage.DoChan()), then we can use prune for the other one (prune.DoChan())

Same for the other singleFlightGroups

thaJeztah · 2021-08-09T12:55:41Z

daemon/daemon.go

-	startupDone      chan struct{}
+	singleflightGroup singleflight.Group
+
+	pruneRunning int32


Not for this PR, but should we use the same approach for pruning, and use a singleFlightGroup for that as well?

daemon/images/service.go

volume/service/service.go

daemon/images/service.go

rvolosatovs · 2021-08-09T16:17:48Z

Ah, just left a comment about that. Was your idea to reuse the same groups, or to use the same approach ?

Given the library API it seems that the expected usage is to reuse the group. I have hard time understanding why so much functionality would be present for key support otherwise.

Personally, reusing anything here does not seem like the right approach, but if that's the "idiomatic" usage, then I suppose we should follow.

Always a bit cautious with abstracting away things too much. You mean something like;
(example)

Yes, exactly that, but it doesn't seem like it should deserve it's own package. It's something I would like to put in a internal/syncutil, if there was such a package.

cpuguy83 · 2021-08-09T16:17:52Z

I don't see how we'd leak anything since the channel is buffered.

rvolosatovs · 2021-08-09T16:19:06Z

I don't see how we'd leak anything since the channel is buffered.

Yes, you're right, we don't actually need the goroutine

thaJeztah · 2021-08-09T16:31:47Z

For reviewers; use the ?w=1 option to view the diff; it makes the diff a lot easier to review (due to whitespace changes); https://github.com/moby/moby/pull/42715/files?w=1

cpuguy83

LGTM

thaJeztah

changes look good, but can you squash/combine the first and last commit?

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>

thaJeztah

LGTM, thanks!

thaJeztah · 2021-08-09T19:42:52Z

Hmm


[2021-08-09T18:06:13.240Z] 29d5b6a226d2: Download complete
[2021-08-09T18:07:54.461Z] powershell.exe : failed to register layer: re-exec error: exit status 1: output: hcsshim::ProcessUtilityVMImage 
[2021-08-09T18:07:54.461Z] \\?\D:\docker\windowsfilter\1258f08accef9b7f046ebfe52bddc3814446163e842dca420fe8242533da0e06\UtilityVM: To use this container image, you must join the Windows Insider Program. Please see 
[2021-08-09T18:07:54.461Z] https://go.microsoft.com/fwlink/?linkid=850659 for more information.

@StefanScherer could this be because the machines were updated? (if so, do we need #42527 to be merged?)

thaJeztah · 2021-08-10T08:49:47Z

merged #42527. Let me trigger CI again to see if it picks up that change

thaJeztah · 2021-08-10T11:50:59Z

Yup! All green again now

rvolosatovs force-pushed the shared_disk_usage branch 2 times, most recently from 82c1da4 to 9b26c2b Compare August 6, 2021 15:12

rvolosatovs marked this pull request as ready for review August 6, 2021 15:36

This comment has been minimized.

Sign in to view

rvolosatovs force-pushed the shared_disk_usage branch 4 times, most recently from 03a6cb9 to 57b7519 Compare August 9, 2021 10:38

thaJeztah reviewed Aug 9, 2021

View reviewed changes

rvolosatovs force-pushed the shared_disk_usage branch 2 times, most recently from 9b31dc1 to 700a0c4 Compare August 9, 2021 16:12

rvolosatovs force-pushed the shared_disk_usage branch from 700a0c4 to 3ba9168 Compare August 9, 2021 16:20

rvolosatovs requested a review from thaJeztah August 9, 2021 16:22

cpuguy83 approved these changes Aug 9, 2021

View reviewed changes

thaJeztah requested changes Aug 9, 2021

View reviewed changes

rvolosatovs force-pushed the shared_disk_usage branch from 3ba9168 to 8e01003 Compare August 9, 2021 17:58

Roman Volosatovs added 2 commits August 9, 2021 19:59

daemon: sort imports according to gofmt

5adc29f

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>

daemon,volume: share disk usage computations

135cec5

Signed-off-by: Roman Volosatovs <roman.volosatovs@docker.com>

rvolosatovs force-pushed the shared_disk_usage branch from 8e01003 to 135cec5 Compare August 9, 2021 18:00

rvolosatovs requested a review from thaJeztah August 9, 2021 18:00

thaJeztah approved these changes Aug 9, 2021

View reviewed changes

thaJeztah added area/api API impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. labels Aug 9, 2021

thaJeztah added the status/4-merge label Aug 9, 2021

thaJeztah added this to the 21.xx milestone Aug 9, 2021

thaJeztah merged commit b88acf7 into moby:master Aug 10, 2021

thaJeztah mentioned this pull request Aug 10, 2021

docs/api: add node about concurrency on /system/df endpoint #42730

Merged

This was referenced Nov 30, 2022

[23.0] daemon/disk_usage: Use context aware singleflight #44558

Closed

daemon/disk_usage: Use context aware singleflight #44520

Merged

Conversation

rvolosatovs commented Aug 6, 2021 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

cpuguy83 commented Aug 6, 2021

Uh oh!

rvolosatovs commented Aug 9, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

rvolosatovs Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rvolosatovs commented Aug 9, 2021

Uh oh!

cpuguy83 commented Aug 9, 2021

Uh oh!

rvolosatovs commented Aug 9, 2021

Uh oh!

thaJeztah commented Aug 9, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 10, 2021

Uh oh!

thaJeztah commented Aug 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rvolosatovs commented Aug 6, 2021 •

edited by thaJeztah

Loading

thaJeztah commented Aug 9, 2021 •

edited

Loading