Skip to content

prefer bind API #12734

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.

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

Merged
merged 1 commit into from
Apr 11, 2025
Merged

prefer bind API #12734

merged 1 commit into from
Apr 11, 2025

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Apr 11, 2025

What I did
Always prefer bind API for volumes over mount when not required.
added a test case to check various combinations

Related issue
test to prevent regression reported on #12121

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@ndeloof ndeloof requested a review from a team as a code owner April 11, 2025 06:39
@ndeloof ndeloof requested a review from glours April 11, 2025 06:39
@@ -874,8 +866,8 @@ func (s *composeService) buildContainerVolumes(
v := findVolumeByTarget(service.Volumes, m.Target)
vol := findVolumeByName(p.Volumes, m.Source)
if v != nil && vol != nil {
if _, ok := vol.DriverOpts["device"]; ok && vol.Driver == "local" && vol.DriverOpts["o"] == "bind" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed this earlier fix, as with volumeRequiresMountAPI we will prefer bind for all volumes without advanced mount options, not just those created with o=bind

@@ -838,13 +837,7 @@ func (s *composeService) buildContainerVolumes(
var mounts []mount.Mount
var binds []string

img := api.GetImageNameOrDefault(service, p.Name)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to buildContainerMountOptions as this is only used when we manage anonymous volumes inheritance (allows to write test without a useless API call/mock)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours merged commit a3f88a0 into docker:main Apr 11, 2025
26 checks passed
@vg22
Copy link

vg22 commented Apr 11, 2025

@ndeloof Will this fix be available in 2.36.0 release?

@glours
Copy link
Contributor

glours commented Apr 19, 2025

@vg22 the issue should be fixed in the latest v2.35.1 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants