Skip to content

volume/mounts: fix anonymous volume not being labeled#48754

Merged
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_anonymous_volumes_labels
Oct 25, 2024
Merged

volume/mounts: fix anonymous volume not being labeled#48754
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:fix_anonymous_volumes_labels

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented Oct 25, 2024 •
edited
Loading

volume/mounts: fix anonymous volume not being labeled

Parser.ParseMountRaw() labels anonymous volumes with a AnonymousLabel label (com.docker.volume.anonymous) label based on whether a volume has a name (named volume) or no name (anonymous) (see 1).

However both VolumesService.Create() (see 1) and Parser.ParseMountRaw() (see 2, 3) were generating a random name for anonymous volumes. The latter is called before VolumesService.Create() is called, resulting in such volumes not being labeled as anonymous.

Generating the name was originally done in Create (fc7b904), but duplicated in b3b7eb2 with the introduction of the new Mounts field in HostConfig. Duplicating this effort didn't have a real effect until (Create would just skip generating the name), until 618f26c introduced the AnonymousLabel in (v24.0.0, backported to v23.0.0).

Parsing generally should not fill in defaults / generate names, so this patch;

  • Removes generating volume names from Parser.ParseMountRaw()
  • Adds a debug-log entry to VolumesService.Create()
  • Touches up some logs to use structured logs for easier correlating logs

With this patch applied:

docker run --rm --mount=type=volume,target=/toto hello-world

DEBU[2024-10-24T22:50:36.359990376Z] creating anonymous volume                     volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02
DEBU[2024-10-24T22:50:36.360069209Z] probing all drivers for volume                volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02
DEBU[2024-10-24T22:50:36.360341209Z] Registering new volume reference              driver=local volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02

- How to verify it

make TEST_FILTER='TestContainerVolumeAnonymous' TEST_SKIP_INTEGRATION_CLI=1 DOCKER_GRAPHDRIVER=vfs test-integration

=== RUN   TestContainerVolumeAnonymous
--- PASS: TestContainerVolumeAnonymous (0.02s)
PASS

DONE 1 tests in 1.225s

- Description for the changelog

Fix anonymous volumes being created through the `--mount` option not being marked as anonymous.

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah
volume/mounts: fix anonymous volume not being labeled
10d57fd 
`Parser.ParseMountRaw()` labels anonymous volumes with a `AnonymousLabel` label
(`com.docker.volume.anonymous`) label based on whether a volume has a name
(named volume) or no name (anonymous) (see [1]).

However both `VolumesService.Create()` (see [1]) and `Parser.ParseMountRaw()`
(see [2], [3]) were generating a random name for anonymous volumes. The latter
is called before `VolumesService.Create()` is called, resulting in such volumes
not being labeled as anonymous.

Generating the name was originally done in Create (fc7b904),
but duplicated in b3b7eb2 with the introduction
of the new Mounts field in HostConfig. Duplicating this effort didn't have a
real effect until (`Create` would just skip generating the name), until
618f26c introduced the `AnonymousLabel` in
(v24.0.0, backported to v23.0.0).

Parsing generally should not fill in defaults / generate names, so this patch;

- Removes generating volume names from  `Parser.ParseMountRaw()`
- Adds a debug-log entry to `VolumesService.Create()`
- Touches up some logs to use structured logs for easier correlating logs

With this patch applied:

    docker run --rm --mount=type=volume,target=/toto hello-world

    DEBU[2024-10-24T22:50:36.359990376Z] creating anonymous volume                     volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02
    DEBU[2024-10-24T22:50:36.360069209Z] probing all drivers for volume                volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02
    DEBU[2024-10-24T22:50:36.360341209Z] Registering new volume reference              driver=local volume-name=0cfd63d4df363571e7b3e9c04e37c74054cc16ff1d00d9a005232d83e92eda02

[1]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/service/service.go#L72-L83
[2]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/mounts/linux_parser.go#L330-L336
[3]: https://github.com/moby/moby/blob/032721ff75fe4a71a45b4a7a9b7e7c4c80c6431b/volume/mounts/windows_parser.go#L394-L400

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added this to the 28.0.0 milestone Oct 25, 2024
@thaJeztah thaJeztah self-assigned this Oct 25, 2024
This was referenced Oct 25, 2024
Closed
Merged
@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 25, 2024

@neersighted this one's for you (docker/compose#10833) 😄

vvoland
vvoland approved these changes Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM; good catch!

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 25, 2024

good catch!

Thanks for @ndeloof for finding the discrepancy.

I initially thought the problem was here; with Daemon.registerMountPoints() adding volumeopts.WithCreateLabels();

moby/daemon/volumes.go

Lines 210 to 216 in d3c3a74

v, err = daemon.volumes.Create(ctx,
mp.Name,
mp.Driver,
volumeopts.WithCreateReference(container.ID),
volumeopts.WithCreateOptions(driverOpts),
volumeopts.WithCreateLabels(cfg.VolumeOptions.Labels),
)

And VolumeService.Create using opts.WithCreateLabel()

moby/volume/service/service.go

Lines 72 to 76 in 032721f

func (s *VolumesService) Create(ctx context.Context, name, driverName string, options ...opts.CreateOption) (*volumetypes.Volume, error) {
if name == "" {
name = stringid.GenerateRandomID()
options = append(options, opts.WithCreateLabel(AnonymousLabel, ""))
}

Because if they are applied in the reverse order, then volumeopts.WithCreateLabels() will overwrite the other label that's set; WithCreateLabel() merges, but WithCreateLabels() doesn't;

moby/volume/service/opts/opts.go

Lines 15 to 22 in d3c3a74

func WithCreateLabel(key, value string) CreateOption {
return func(cfg *CreateConfig) {
if cfg.Labels == nil {
cfg.Labels = map[string]string{}
}
cfg.Labels[key] = value
}
}

moby/volume/service/opts/opts.go

Lines 26 to 30 in d3c3a74

func WithCreateLabels(labels map[string]string) CreateOption {
return func(cfg *CreateConfig) {
cfg.Labels = labels
}
}

@thaJeztah
Copy link
Copy Markdown
Member Author

thaJeztah commented Oct 25, 2024

three LGTM's - works for me; thanks!

@thaJeztah thaJeztah merged commit b50f8af into moby:master Oct 25, 2024
@thaJeztah thaJeztah deleted the fix_anonymous_volumes_labels branch October 25, 2024 13:16
@thaJeztah thaJeztah mentioned this pull request Oct 25, 2024
Closed
@thaJeztah thaJeztah mentioned this pull request Oct 26, 2024
Merged
thaJeztah
thaJeztah commented Oct 26, 2024
View reviewed changes
Comment thread volume/service/service.go
Comment on lines +76 to +78
log.G(ctx).WithField("volume-name", name).Error("Creating anonymous volume")
} else {
log.G(ctx).WithField("volume-name", name).Error("Creating named volume")
Copy link
Copy Markdown
Member Author

@thaJeztah thaJeztah Oct 26, 2024

Choose a reason for hiding this comment

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

Whoops; I left these as Error() log instead of Debug() 🙈

@thaJeztah thaJeztah mentioned this pull request Oct 26, 2024
Merged
@austinvazquez austinvazquez mentioned this pull request Oct 28, 2024
Merged
@KevinMind KevinMind mentioned this pull request Oct 30, 2024
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vvoland vvoland vvoland approved these changes

@austinvazquez austinvazquez austinvazquez approved these changes

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83

@neersighted neersighted Awaiting requested review from neersighted

+1 more reviewer

@ndeloof ndeloof ndeloof approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@thaJeztah thaJeztah

Labels

area/volumes Volumes kind/bugfix PR's that fix bugs process/cherry-pick/23.0 process/cherry-picked status/2-code-review

Projects

None yet

Milestone

28.0.0

Development

Successfully merging this pull request may close these issues.

anonymous volumes created using --mount don't get AnonymousLabel set

4 participants

@thaJeztah @ndeloof @vvoland @austinvazquez