Skip to content

Commit

Permalink
revert commits link to mount API over bind changes
Browse files Browse the repository at this point in the history 
Signed-off-by: Guillaume Lours <[email protected]>
  • Loading branch information
@glours @ndeloof
glours authored and ndeloof committed Sep 20, 2024
1 parent c16df17 commit 9c60fe6
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 91 deletions.
17 changes: 0 additions & 17 deletions pkg/compose/compose.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -321,23 +321,6 @@ func (s *composeService) RuntimeVersion(ctx context.Context) (string, error) {

}

var windowsContainer = struct {
once sync.Once
val bool
err error
}{}

func (s *composeService) isWindowsContainer(ctx context.Context) (bool, error) {
windowsContainer.once.Do(func() {
info, err := s.apiClient().Info(ctx)
if err != nil {
windowsContainer.err = err
}
windowsContainer.val = info.OSType == "windows"
})
return windowsContainer.val, windowsContainer.err
}

func (s *composeService) isDesktopIntegrationActive() bool {
return s.desktopCli != nil
}
Expand Down
31 changes: 23 additions & 8 deletions pkg/compose/convergence_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
containerType "github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/network"
"github.com/docker/go-connections/nat"
"go.uber.org/mock/gomock"
"gotest.tools/v3/assert"

Expand Down Expand Up @@ -300,10 +301,17 @@ func TestCreateMobyContainer(t *testing.T) {
},
}

apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Cond(func(x any) bool {
v := x.(*containerType.HostConfig)
return v.NetworkMode == "b-moby-name"
}), gomock.Eq(
var falseBool bool
apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Eq(
&containerType.HostConfig{
PortBindings: nat.PortMap{},
ExtraHosts: []string{},
Tmpfs: map[string]string{},
Resources: containerType.Resources{
OomKillDisable: &falseBool,
},
NetworkMode: "b-moby-name",
}), gomock.Eq(
&network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
"b-moby-name": {
Expand Down Expand Up @@ -382,10 +390,17 @@ func TestCreateMobyContainer(t *testing.T) {
},
}

apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Cond(func(x any) bool {
v := x.(*containerType.HostConfig)
return v.NetworkMode == "b-moby-name"
}), gomock.Eq(
var falseBool bool
apiClient.EXPECT().ContainerCreate(gomock.Any(), gomock.Any(), gomock.Eq(
&containerType.HostConfig{
PortBindings: nat.PortMap{},
ExtraHosts: []string{},
Tmpfs: map[string]string{},
Resources: containerType.Resources{
OomKillDisable: &falseBool,
},
NetworkMode: "b-moby-name",
}), gomock.Eq(
&network.NetworkingConfig{
EndpointsConfig: map[string]*network.EndpointSettings{
"a-moby-name": {
Expand Down
102 changes: 46 additions & 56 deletions pkg/compose/create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -801,28 +801,28 @@ func (s *composeService) buildContainerVolumes(
return nil, nil, err
}

mountOptions, err := s.buildContainerMountOptions(ctx, p, service, imgInspect, inherit)
mountOptions, err := buildContainerMountOptions(p, service, imgInspect, inherit)
if err != nil {
return nil, nil, err
}

version, err := s.RuntimeVersion(ctx)
if err != nil {
return nil, nil, err
}
if versions.GreaterThan(version, "1.42") {
// We can fully leverage `Mount` API as a replacement for legacy `Bind`
return nil, mountOptions, nil
}

MOUNTS:
for _, m := range mountOptions {
if m.Type == mount.TypeNamedPipe {
mounts = append(mounts, m)
continue
}
if m.Type == mount.TypeBind {
// `Mount` does not offer option to created host path if missing
// `Mount` is preferred but does not offer option to created host path if missing
// so `Bind` API is used here with raw volume string
// see https://github.com/moby/moby/issues/43483
for _, v := range service.Volumes {
if v.Target == m.Target {
if v.Bind != nil && v.Bind.CreateHostPath {
switch {
case string(m.Type) != v.Type:
v.Source = m.Source
fallthrough
case v.Bind != nil && v.Bind.CreateHostPath:
binds = append(binds, v.String())
continue MOUNTS
}
Expand All @@ -834,7 +834,7 @@ MOUNTS:
return binds, mounts, nil
}

func (s *composeService) buildContainerMountOptions(ctx context.Context, p types.Project, service types.ServiceConfig, img moby.ImageInspect, inherit *moby.Container) ([]mount.Mount, error) {
func buildContainerMountOptions(p types.Project, s types.ServiceConfig, img moby.ImageInspect, inherit *moby.Container) ([]mount.Mount, error) {
var mounts = map[string]mount.Mount{}
if inherit != nil {
for _, m := range inherit.Mounts {
Expand All @@ -859,7 +859,7 @@ func (s *composeService) buildContainerMountOptions(ctx context.Context, p types
}
}
volumes := []types.ServiceVolumeConfig{}
for _, v := range service.Volumes {
for _, v := range s.Volumes {
if v.Target != m.Destination || v.Source != "" {
volumes = append(volumes, v)
continue
Expand All @@ -872,11 +872,11 @@ func (s *composeService) buildContainerMountOptions(ctx context.Context, p types
ReadOnly: !m.RW,
}
}
service.Volumes = volumes
s.Volumes = volumes
}
}

mounts, err := s.fillBindMounts(ctx, p, service, mounts)
mounts, err := fillBindMounts(p, s, mounts)
if err != nil {
return nil, err
}
Expand All @@ -888,27 +888,27 @@ func (s *composeService) buildContainerMountOptions(ctx context.Context, p types
return values, nil
}

func (s *composeService) fillBindMounts(ctx context.Context, p types.Project, service types.ServiceConfig, m map[string]mount.Mount) (map[string]mount.Mount, error) {
for _, v := range service.Volumes {
bindMount, err := s.buildMount(ctx, p, v)
func fillBindMounts(p types.Project, s types.ServiceConfig, m map[string]mount.Mount) (map[string]mount.Mount, error) {
for _, v := range s.Volumes {
bindMount, err := buildMount(p, v)
if err != nil {
return nil, err
}
m[bindMount.Target] = bindMount
}

secrets, err := s.buildContainerSecretMounts(ctx, p, service)
secrets, err := buildContainerSecretMounts(p, s)
if err != nil {
return nil, err
}
for _, secret := range secrets {
if _, found := m[secret.Target]; found {
for _, s := range secrets {
if _, found := m[s.Target]; found {
continue
}
m[secret.Target] = secret
m[s.Target] = s
}

configs, err := s.buildContainerConfigMounts(ctx, p, service)
configs, err := buildContainerConfigMounts(p, s)
if err != nil {
return nil, err
}
Expand All @@ -921,11 +921,11 @@ func (s *composeService) fillBindMounts(ctx context.Context, p types.Project, se
return m, nil
}

func (s *composeService) buildContainerConfigMounts(ctx context.Context, p types.Project, service types.ServiceConfig) ([]mount.Mount, error) {
func buildContainerConfigMounts(p types.Project, s types.ServiceConfig) ([]mount.Mount, error) {
var mounts = map[string]mount.Mount{}

configsBaseDir := "/"
for _, config := range service.Configs {
for _, config := range s.Configs {
target := config.Target
if config.Target == "" {
target = configsBaseDir + config.Source
Expand Down Expand Up @@ -953,7 +953,7 @@ func (s *composeService) buildContainerConfigMounts(ctx context.Context, p types
continue
}

bindMount, err := s.buildMount(ctx, p, types.ServiceVolumeConfig{
bindMount, err := buildMount(p, types.ServiceVolumeConfig{
Type: types.VolumeTypeBind,
Source: definedConfig.File,
Target: target,
Expand All @@ -971,11 +971,11 @@ func (s *composeService) buildContainerConfigMounts(ctx context.Context, p types
return values, nil
}

func (s *composeService) buildContainerSecretMounts(ctx context.Context, p types.Project, service types.ServiceConfig) ([]mount.Mount, error) {
func buildContainerSecretMounts(p types.Project, s types.ServiceConfig) ([]mount.Mount, error) {
var mounts = map[string]mount.Mount{}

secretsDir := "/run/secrets/"
for _, secret := range service.Secrets {
for _, secret := range s.Secrets {
target := secret.Target
if secret.Target == "" {
target = secretsDir + secret.Source
Expand Down Expand Up @@ -1003,7 +1003,7 @@ func (s *composeService) buildContainerSecretMounts(ctx context.Context, p types
continue
}

mnt, err := s.buildMount(ctx, p, types.ServiceVolumeConfig{
mnt, err := buildMount(p, types.ServiceVolumeConfig{
Type: types.VolumeTypeBind,
Source: definedSecret.File,
Target: target,
Expand Down Expand Up @@ -1039,7 +1039,7 @@ func isWindowsAbs(p string) bool {
return false
}

func (s *composeService) buildMount(ctx context.Context, project types.Project, volume types.ServiceVolumeConfig) (mount.Mount, error) {
func buildMount(project types.Project, volume types.ServiceVolumeConfig) (mount.Mount, error) {
source := volume.Source
// on windows, filepath.IsAbs(source) is false for unix style abs path like /var/run/docker.sock.
// do not replace these with filepath.Abs(source) that will include a default drive.
Expand All @@ -1060,10 +1060,7 @@ func (s *composeService) buildMount(ctx context.Context, project types.Project,
}
}

bind, vol, tmpfs, err := s.buildMountOptions(ctx, volume)
if err != nil {
return mount.Mount{}, err
}
bind, vol, tmpfs := buildMountOptions(project, volume)

volume.Target = path.Clean(volume.Target)

Expand All @@ -1083,7 +1080,7 @@ func (s *composeService) buildMount(ctx context.Context, project types.Project,
}, nil
}

func (s *composeService) buildMountOptions(ctx context.Context, volume types.ServiceVolumeConfig) (*mount.BindOptions, *mount.VolumeOptions, *mount.TmpfsOptions, error) {
func buildMountOptions(project types.Project, volume types.ServiceVolumeConfig) (*mount.BindOptions, *mount.VolumeOptions, *mount.TmpfsOptions) {
switch volume.Type {
case "bind":
if volume.Volume != nil {
Expand All @@ -1092,47 +1089,40 @@ func (s *composeService) buildMountOptions(ctx context.Context, volume types.Ser
if volume.Tmpfs != nil {
logrus.Warnf("mount of type `bind` should not define `tmpfs` option")
}
option, err := s.buildBindOption(ctx, volume.Bind)
return option, nil, nil, err
return buildBindOption(volume.Bind), nil, nil
case "volume":
if volume.Bind != nil {
logrus.Warnf("mount of type `volume` should not define `bind` option")
}
if volume.Tmpfs != nil {
logrus.Warnf("mount of type `volume` should not define `tmpfs` option")
}
return nil, buildVolumeOptions(volume.Volume), nil, nil
if v, ok := project.Volumes[volume.Source]; ok && v.DriverOpts["o"] == types.VolumeTypeBind {
return buildBindOption(&types.ServiceVolumeBind{
CreateHostPath: true,
}), nil, nil
}
return nil, buildVolumeOptions(volume.Volume), nil
case "tmpfs":
if volume.Bind != nil {
logrus.Warnf("mount of type `tmpfs` should not define `bind` option")
}
if volume.Volume != nil {
logrus.Warnf("mount of type `tmpfs` should not define `volume` option")
}
return nil, nil, buildTmpfsOptions(volume.Tmpfs), nil
return nil, nil, buildTmpfsOptions(volume.Tmpfs)
}
return nil, nil, nil, nil
return nil, nil, nil
}

func (s *composeService) buildBindOption(ctx context.Context, bind *types.ServiceVolumeBind) (*mount.BindOptions, error) {
func buildBindOption(bind *types.ServiceVolumeBind) *mount.BindOptions {
if bind == nil {
return nil, nil
}

propagation := bind.Propagation
isWindowsContainer, err := s.isWindowsContainer(ctx)
if err != nil {
return nil, err
}
if propagation == "" && !isWindowsContainer {
propagation = types.PropagationRPrivate
return nil
}

return &mount.BindOptions{
Propagation: mount.Propagation(propagation),
CreateMountpoint: bind.CreateHostPath,
Propagation: mount.Propagation(bind.Propagation),
// NonRecursive: false, FIXME missing from model ?
}, nil
}
}

func buildVolumeOptions(vol *types.ServiceVolumeVolume) *mount.VolumeOptions {
Expand Down
16 changes: 6 additions & 10 deletions pkg/compose/create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
package compose

import (
"context"
"os"
"path/filepath"
"sort"
Expand All @@ -42,8 +41,7 @@ func TestBuildBindMount(t *testing.T) {
Source: "",
Target: "/data",
}
s := composeService{}
mount, err := s.buildMount(context.TODO(), project, volume)
mount, err := buildMount(project, volume)
assert.NilError(t, err)
assert.Assert(t, filepath.IsAbs(mount.Source))
_, err = os.Stat(mount.Source)
Expand All @@ -58,8 +56,7 @@ func TestBuildNamedPipeMount(t *testing.T) {
Source: "\\\\.\\pipe\\docker_engine_windows",
Target: "\\\\.\\pipe\\docker_engine",
}
s := composeService{}
mount, err := s.buildMount(context.TODO(), project, volume)
mount, err := buildMount(project, volume)
assert.NilError(t, err)
assert.Equal(t, mount.Type, mountTypes.TypeNamedPipe)
}
Expand All @@ -78,8 +75,7 @@ func TestBuildVolumeMount(t *testing.T) {
Source: "myVolume",
Target: "/data",
}
s := composeService{}
mount, err := s.buildMount(context.TODO(), project, volume)
mount, err := buildMount(project, volume)
assert.NilError(t, err)
assert.Equal(t, mount.Source, "myProject_myVolume")
assert.Equal(t, mount.Type, mountTypes.TypeVolume)
Expand Down Expand Up @@ -156,8 +152,8 @@ func TestBuildContainerMountOptions(t *testing.T) {
},
},
}
s := composeService{}
mounts, err := s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], moby.ImageInspect{}, inherit)

mounts, err := buildContainerMountOptions(project, project.Services["myService"], moby.ImageInspect{}, inherit)
sort.Slice(mounts, func(i, j int) bool {
return mounts[i].Target < mounts[j].Target
})
Expand All @@ -169,7 +165,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 = s.buildContainerMountOptions(context.TODO(), project, project.Services["myService"], moby.ImageInspect{}, inherit)
mounts, err = buildContainerMountOptions(project, project.Services["myService"], moby.ImageInspect{}, inherit)
sort.Slice(mounts, func(i, j int) bool {
return mounts[i].Target < mounts[j].Target
})
Expand Down

0 comments on commit 9c60fe6

Please sign in to comment.