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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 34 additions & 21 deletions pkg/compose/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"github.com/docker/docker/api/types/blkiodev"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/image"
"github.com/docker/docker/api/types/mount"
"github.com/docker/docker/api/types/network"
"github.com/docker/docker/api/types/strslice"
Expand Down Expand Up @@ -828,7 +827,6 @@ func getDependentServiceFromMode(mode string) string {
return ""
}

//nolint:gocyclo
func (s *composeService) buildContainerVolumes(
ctx context.Context,
p types.Project,
Expand All @@ -838,13 +836,7 @@ func (s *composeService) buildContainerVolumes(
var mounts []mount.Mount
var binds []string

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

@ndeloof ndeloof Apr 11, 2025

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)

imgInspect, err := s.apiClient().ImageInspect(ctx, img)
if err != nil {
return nil, nil, err
}

mountOptions, err := buildContainerMountOptions(p, service, imgInspect, inherit)
mountOptions, err := s.buildContainerMountOptions(ctx, p, service, inherit)
if err != nil {
return nil, nil, err
}
Expand All @@ -857,11 +849,10 @@ func (s *composeService) buildContainerVolumes(
// see https://github.com/moby/moby/issues/43483
v := findVolumeByTarget(service.Volumes, m.Target)
if v != nil {
switch {
case v.Type != types.VolumeTypeBind:
if v.Type != types.VolumeTypeBind {
v.Source = m.Source
fallthrough
case !requireMountAPI(v.Bind):
}
if !bindRequiresMountAPI(v.Bind) {
source := m.Source
if vol := findVolumeByName(p.Volumes, m.Source); vol != nil {
source = m.Source
Expand All @@ -874,8 +865,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

@ndeloof ndeloof Apr 11, 2025

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

// Looks like a volume, but actually a bind mount which requires the bind API
// Prefer the bind API if no advanced option is used, to preserve backward compatibility
if !volumeRequiresMountAPI(v.Volume) {
binds = append(binds, toBindString(vol.Name, v))
continue
}
Expand Down Expand Up @@ -930,9 +921,9 @@ func findVolumeByTarget(volumes []types.ServiceVolumeConfig, target string) *typ
return nil
}

// requireMountAPI check if Bind declaration can be implemented by the plain old Bind API or uses any of the advanced
// bindRequiresMountAPI check if Bind declaration can be implemented by the plain old Bind API or uses any of the advanced
// options which require use of Mount API
func requireMountAPI(bind *types.ServiceVolumeBind) bool {
func bindRequiresMountAPI(bind *types.ServiceVolumeBind) bool {
switch {
case bind == nil:
return false
Expand All @@ -947,7 +938,24 @@ func requireMountAPI(bind *types.ServiceVolumeBind) bool {
}
}

func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img image.InspectResponse, inherit *container.Summary) ([]mount.Mount, error) {
// volumeRequiresMountAPI check if Volume declaration can be implemented by the plain old Bind API or uses any of the advanced
// options which require use of Mount API
func volumeRequiresMountAPI(vol *types.ServiceVolumeVolume) bool {
switch {
case vol == nil:
return false
case len(vol.Labels) > 0:
return true
case vol.Subpath != "":
return true
case vol.NoCopy:
return true
default:
return false
}
}

func (s *composeService) buildContainerMountOptions(ctx context.Context, p types.Project, service types.ServiceConfig, inherit *container.Summary) ([]mount.Mount, error) {
mounts := map[string]mount.Mount{}
if inherit != nil {
for _, m := range inherit.Mounts {
Expand All @@ -959,6 +967,11 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
src = m.Name
}

img, err := s.apiClient().ImageInspect(ctx, api.GetImageNameOrDefault(service, p.Name))
if err != nil {
return nil, err
}

if img.Config != nil {
if _, ok := img.Config.Volumes[m.Destination]; ok {
// inherit previous container's anonymous volume
Expand All @@ -971,7 +984,7 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
}
}
volumes := []types.ServiceVolumeConfig{}
for _, v := range s.Volumes {
for _, v := range service.Volumes {
if v.Target != m.Destination || v.Source != "" {
volumes = append(volumes, v)
continue
Expand All @@ -984,11 +997,11 @@ func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img imag
ReadOnly: !m.RW,
}
}
s.Volumes = volumes
service.Volumes = volumes
}
}

mounts, err := fillBindMounts(p, s, mounts)
mounts, err := fillBindMounts(p, service, mounts)
if err != nil {
return nil, err
}
Expand Down
136 changes: 134 additions & 2 deletions pkg/compose/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,16 @@
package compose

import (
"context"
"os"
"path/filepath"
"sort"
"testing"

composeloader "github.com/compose-spec/compose-go/v2/loader"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/image"
"go.uber.org/mock/gomock"
"gotest.tools/v3/assert/cmp"

"github.com/docker/compose/v2/pkg/api"
Expand Down Expand Up @@ -154,7 +157,16 @@ func TestBuildContainerMountOptions(t *testing.T) {
},
}

mounts, err := buildContainerMountOptions(project, project.Services["myService"], image.InspectResponse{}, inherit)
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

mock, cli := prepareMocks(mockCtrl)
s := composeService{
dockerCli: cli,
}
mock.EXPECT().ImageInspect(gomock.Any(), "myProject-myService").AnyTimes().Return(image.InspectResponse{}, nil)

mounts, err := s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], inherit)
sort.Slice(mounts, func(i, j int) bool {
return mounts[i].Target < mounts[j].Target
})
Expand All @@ -166,7 +178,7 @@ func TestBuildContainerMountOptions(t *testing.T) {
assert.Equal(t, mounts[2].VolumeOptions.Subpath, "etc")
assert.Equal(t, mounts[3].Target, "\\\\.\\pipe\\docker_engine")

mounts, err = buildContainerMountOptions(project, project.Services["myService"], image.InspectResponse{}, inherit)
mounts, err = s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], inherit)
sort.Slice(mounts, func(i, j int) bool {
return mounts[i].Target < mounts[j].Target
})
Expand Down Expand Up @@ -321,3 +333,123 @@ func TestCreateEndpointSettings(t *testing.T) {
IPv6Gateway: "fdb4:7a7f:373a:3f0c::42",
}))
}

func Test_buildContainerVolumes(t *testing.T) {
pwd, err := os.Getwd()
assert.NilError(t, err)

tests := []struct {
name string
yaml string
binds []string
mounts []mountTypes.Mount
}{
{
name: "bind mount local path",
yaml: `
services:
test:
volumes:
- ./data:/data
`,
binds: []string{filepath.Join(pwd, "data") + ":/data:rw"},
mounts: nil,
},
{
name: "bind mount, not create host path",
yaml: `
services:
test:
volumes:
- type: bind
source: ./data
target: /data
bind:
create_host_path: false
`,
binds: nil,
mounts: []mountTypes.Mount{
{
Type: "bind",
Source: filepath.Join(pwd, "data"),
Target: "/data",
BindOptions: &mountTypes.BindOptions{CreateMountpoint: false},
},
},
},
{
name: "mount volume",
yaml: `
services:
test:
volumes:
- data:/data
volumes:
data:
name: my_volume
`,
binds: []string{"my_volume:/data:rw"},
mounts: nil,
},
{
name: "mount volume, readonly",
yaml: `
services:
test:
volumes:
- data:/data:ro
volumes:
data:
name: my_volume
`,
binds: []string{"my_volume:/data:ro"},
mounts: nil,
},
{
name: "mount volume subpath",
yaml: `
services:
test:
volumes:
- type: volume
source: data
target: /data
volume:
subpath: test/
volumes:
data:
name: my_volume
`,
binds: nil,
mounts: []mountTypes.Mount{
{
Type: "volume",
Source: "my_volume",
Target: "/data",
VolumeOptions: &mountTypes.VolumeOptions{Subpath: "test/"},
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
p, err := composeloader.LoadWithContext(context.TODO(), composetypes.ConfigDetails{
ConfigFiles: []composetypes.ConfigFile{
{
Filename: "test",
Content: []byte(tt.yaml),
},
},
}, func(options *composeloader.Options) {
options.SkipValidation = true
options.SkipConsistencyCheck = true
})
assert.NilError(t, err)
s := &composeService{}
binds, mounts, err := s.buildContainerVolumes(context.TODO(), *p, p.Services["test"], nil)
assert.NilError(t, err)
assert.DeepEqual(t, tt.binds, binds)
assert.DeepEqual(t, tt.mounts, mounts)
})
}
}