From 9c60fe67df0763c81f2276fd4163ee5354d70b09 Mon Sep 17 00:00:00 2001 From: Guillaume Lours <705411+glours@users.noreply.github.com> Date: Fri, 20 Sep 2024 10:12:09 +0200 Subject: [PATCH] revert commits link to mount API over bind changes Signed-off-by: Guillaume Lours <705411+glours@users.noreply.github.com> --- pkg/compose/compose.go | 17 ------ pkg/compose/convergence_test.go | 31 +++++++--- pkg/compose/create.go | 102 ++++++++++++++------------------ pkg/compose/create_test.go | 16 ++--- 4 files changed, 75 insertions(+), 91 deletions(-) diff --git a/pkg/compose/compose.go b/pkg/compose/compose.go index a93c95bc24..10322fc7d8 100644 --- a/pkg/compose/compose.go +++ b/pkg/compose/compose.go @@ -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 } diff --git a/pkg/compose/convergence_test.go b/pkg/compose/convergence_test.go index 58a9d5fc58..e25ccd9f64 100644 --- a/pkg/compose/convergence_test.go +++ b/pkg/compose/convergence_test.go @@ -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" @@ -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": { @@ -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": { diff --git a/pkg/compose/create.go b/pkg/compose/create.go index 08b9a0aa44..c4d1734fa5 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -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 } @@ -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 { @@ -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 @@ -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 } @@ -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 } @@ -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 @@ -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, @@ -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 @@ -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, @@ -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. @@ -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) @@ -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 { @@ -1092,8 +1089,7 @@ 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") @@ -1101,7 +1097,12 @@ func (s *composeService) buildMountOptions(ctx context.Context, volume types.Ser 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") @@ -1109,30 +1110,19 @@ func (s *composeService) buildMountOptions(ctx context.Context, volume types.Ser 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 { diff --git a/pkg/compose/create_test.go b/pkg/compose/create_test.go index c732ba447d..34d5f59c47 100644 --- a/pkg/compose/create_test.go +++ b/pkg/compose/create_test.go @@ -17,7 +17,6 @@ package compose import ( - "context" "os" "path/filepath" "sort" @@ -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) @@ -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) } @@ -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) @@ -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 }) @@ -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 })