daemon/disk_usage: Use context aware singleflight by vvoland · Pull Request #44520 · moby/moby

vvoland · 2022-11-24T12:59:50Z

Follow up to:

imageservice: Add context to various methods #44365 (comment)

Relates to:

Share disk usage computation results between concurrent invocations #42715 (it replaces the implementation introduced in that PR)

The singleflight function was capturing the context.Context of the first caller that invoked the singleflight.Do. So context cancellation of the first caller could cancel the execution of other concurrent invocations, even if their context wasn't cancelled.

singleflight calls were also moved up from the ImageService to Daemon. This allows to avoid duplicating this behaviour in every ImageService implementation (with containerd integration, we have two now).

vvoland · 2022-11-24T13:00:05Z

vendor.mod

 	google.golang.org/genproto v0.0.0-20220706185917-7780775163c4
 	google.golang.org/grpc v1.48.0
 	gotest.tools/v3 v3.4.0
+	resenje.org/singleflight v0.3.0


Are we fine with vendoring this dependency? It's not popular but it's small and I don't see anything widely used that could be used instead.

^^ for posterity; we briefly discussed this, and I think it's ok to vendor it for now. Looks like the module was recently updated (so actively maintained). We can revisit that in future if there's concerns or if we decide to implement it ourselves.

corhere

singleflight calls were also moved up from the ImageService to Daemon. This allows to avoid duplicating this behaviour in every ImageService implementation (with containerd integration, we have two now).

💯 Great call! In addition to DRYing the implementations, this change is also architecturally sound IMO.

corhere · 2022-11-28T17:30:45Z

daemon/daemon.go

 	"google.golang.org/grpc"
 	"google.golang.org/grpc/backoff"
 	"google.golang.org/grpc/credentials/insecure"
+	ctxsingleflight "resenje.org/singleflight"


Avoid renaming imports except to avoid a name collision

Yeah I wasn't sure about it. My intention here was to make it explicitly visible that this isn't the singleflight from go/x/sync.
On the other hand, it still does the same thing but with context handling, so it's probably fine not to disambiguate them.

daemon/daemon.go

daemon/disk_usage.go

The singleflight function was capturing the context.Context of the first caller that invoked the `singleflight.Do`. This could cause all concurrent calls to be cancelled when the first request is cancelled. singleflight calls were also moved from the ImageService to Daemon, to avoid having to implement this logic in both graphdriver and containerd based image services. Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

corhere

LGTM!

thaJeztah · 2022-11-30T11:11:07Z

Failure looks unrelated, but I don't recall seeing it before, so let me put it in a comment for posterity;

=== FAIL: github.com/docker/docker/integration-cli TestDockerAPISuite/TestContainersAPICreateMountsCreate/0_config:_{volume__c:\foo_false__<nil>_<nil>_<nil>_<nil>} (24.07s)
    docker_api_containers_test.go:2137: timeout hit after 10s: first check never completed
        --- FAIL: TestDockerAPISuite/TestContainersAPICreateMountsCreate/0_config:_{volume__c:\foo_false__<nil>_<nil>_<nil>_<nil>} (24.07s)
723

thaJeztah

LGTM! Thanks

thaJeztah · 2022-11-30T11:23:54Z

daemon/disk_usage.go

+		if err != nil {
+			return nil, errors.Wrap(err, "failed to retrieve image list")
 		}


Something for a follow up;

we seem to be inconsistent in wrapping these errors (and not); e.g. localVolumesSize below doesn't wrap the error

perhaps the wrapping should be handled in daemon.imageService.Images itself? (it looks like some specific errors are returned, and not all of them currently have a proper errdefs type)

Wondering if the wrapping should be done inside the daemon.usageImages.Do or outside (at the exit-point of imageDiskUsage - effectively won't make a difference in this case (so mostly a nit! :joy')

thaJeztah · 2022-11-30T11:26:07Z

vendor.mod

 	google.golang.org/genproto v0.0.0-20220706185917-7780775163c4
 	google.golang.org/grpc v1.48.0
 	gotest.tools/v3 v3.4.0
+	resenje.org/singleflight v0.3.0


^^ for posterity; we briefly discussed this, and I think it's ok to vendor it for now. Looks like the module was recently updated (so actively maintained). We can revisit that in future if there's concerns or if we decide to implement it ourselves.

thaJeztah · 2022-11-30T11:33:04Z

daemon/daemon.go

+	usageContainers singleflight.Group[struct{}, []*types.Container]
+	usageImages     singleflight.Group[struct{}, []*types.ImageSummary]
+	usageVolumes    singleflight.Group[struct{}, []*volume.Volume]
+	usageLayer      singleflight.Group[struct{}, int64]


Not a blocker, and I see @corhere suggested using struct{} for this (#44520 (comment)).

When reading this code, I had to look up the implementation to understand what the first argument was for, so for readability, perhaps (in a follow-up) we should consider defining an ad-hoc type to make it slightly more descriptive / self-documenting. (or add a comment).

thaJeztah · 2022-11-30T11:34:57Z

vendor/resenje.org/singleflight/singleflight.go

+
+// Group represents a class of work and forms a namespace in
+// which units of work can be executed with duplicate suppression.
+type Group[K comparable, V any] struct {


I had to double-check which version of Go added generics, but it's go1.18 (https://go.dev/blog/intro-generics), which matches the Go version we define as minimum in our vendor.mod, so we should be fine on that front 👍

thaJeztah · 2022-11-30T11:41:06Z

daemon/disk_usage.go

+	volumes, _, err := daemon.usageVolumes.Do(ctx, struct{}{}, func(ctx context.Context) ([]*volume.Volume, error) {
+		volumes, err := daemon.volumes.LocalVolumesSize(ctx)


singleflight calls were also moved up from the ImageService to Daemon. This allows to avoid duplicating this behaviour in every ImageService implementation (with containerd integration, we have two now).

LOL, even though I read that comment; I started reading the code and my first thought was: why not keep this part of daemon.volumes.LocalVolumesSize so that any consumer of it would automatically get this feature? Then recalled "ooooooh" right 🤦 🙈 🙈 🙈

Makes a lot of sense from that perspective 👍

vvoland commented Nov 24, 2022

View reviewed changes

vvoland requested review from corhere and thaJeztah and removed request for corhere November 24, 2022 13:00

corhere reviewed Nov 28, 2022

View reviewed changes

vvoland force-pushed the disk-usage-singleflight branch from acb32da to dec81e4 Compare November 29, 2022 15:47

corhere approved these changes Nov 29, 2022

View reviewed changes

thaJeztah added status/2-code-review containerd-integration Issues and PRs related to containerd integration labels Nov 30, 2022

thaJeztah added this to the v-next milestone Nov 30, 2022

thaJeztah approved these changes Nov 30, 2022

View reviewed changes

thaJeztah added status/4-merge and removed status/2-code-review labels Nov 30, 2022

thaJeztah merged commit 1907027 into moby:master Nov 30, 2022

thaJeztah mentioned this pull request Nov 30, 2022

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

Closed

tianon mentioned this pull request Nov 30, 2022

Update docker-master, especially with explicit "go X.YY" directive in our fake go.mod files tianon/dockerfiles#321

Merged

thaJeztah mentioned this pull request Dec 5, 2022

Fix ssh process killed when context is done docker/cli#3900

Merged

thaJeztah mentioned this pull request Sep 11, 2023

Preserve context values janos/singleflight#6

Merged

thaJeztah mentioned this pull request Feb 3, 2025

Flaky test (on Windows): TestContainersAPICreateMountsCreate #49382

Open

		volumes, _, err := daemon.usageVolumes.Do(ctx, struct{}{}, func(ctx context.Context) ([]*volume.Volume, error) {
		volumes, err := daemon.volumes.LocalVolumesSize(ctx)

Conversation

vvoland commented Nov 24, 2022 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland Nov 29, 2022 • 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!

Uh oh!

Uh oh!

corhere left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 30, 2022

Uh oh!

thaJeztah left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vvoland commented Nov 24, 2022 •

edited by thaJeztah

Loading

vvoland Nov 29, 2022 •

edited

Loading