From c0b8d34b26db53c99af1ac1c9d2a247622dd72e6 Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 14 Sep 2023 09:11:27 +0200 Subject: [PATCH] don't rely on depends_on to resolve volume_from, better use observed state Signed-off-by: Nicolas De Loof --- pkg/compose/convergence.go | 25 ++++++++ pkg/compose/create.go | 73 +---------------------- pkg/compose/create_test.go | 40 ------------- pkg/compose/run.go | 3 - pkg/e2e/fixtures/no-deps/volume-from.yaml | 10 ++++ pkg/e2e/noDeps_test.go | 44 ++++++++++++++ 6 files changed, 80 insertions(+), 115 deletions(-) create mode 100644 pkg/e2e/fixtures/no-deps/volume-from.yaml create mode 100644 pkg/e2e/noDeps_test.go diff --git a/pkg/compose/convergence.go b/pkg/compose/convergence.go index 33b97344b19..adaad8476ce 100644 --- a/pkg/compose/convergence.go +++ b/pkg/compose/convergence.go @@ -178,6 +178,11 @@ func (c *convergence) ensureService(ctx context.Context, project *types.Project, eg, _ := errgroup.WithContext(ctx) + err = c.resolveVolumeFrom(&service) + if err != nil { + return err + } + sort.Slice(containers, func(i, j int) bool { return containers[i].Created < containers[j].Created }) @@ -258,6 +263,26 @@ func (c *convergence) ensureService(ctx context.Context, project *types.Project, return err } +func (c *convergence) resolveVolumeFrom(service *types.ServiceConfig) error { + for i, vol := range service.VolumesFrom { + spec := strings.Split(vol, ":") + if len(spec) == 0 { + continue + } + if spec[0] == "container" { + service.VolumesFrom[i] = spec[1] + continue + } + name := spec[0] + dependencies := c.getObservedState(name) + if len(dependencies) == 0 { + return fmt.Errorf("cannot share volume with service %s: container missing", name) + } + service.VolumesFrom[i] = dependencies.sorted()[0].ID + } + return nil +} + func mustRecreate(expected types.ServiceConfig, actual moby.Container, policy string) (bool, error) { if policy == api.RecreateNever { return false, nil diff --git a/pkg/compose/create.go b/pkg/compose/create.go index d905c14b3b4..0b0d81ca25d 100644 --- a/pkg/compose/create.go +++ b/pkg/compose/create.go @@ -86,11 +86,6 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt prepareNetworks(project) - err = prepareVolumes(project) - if err != nil { - return err - } - if err := s.ensureNetworks(ctx, project.Networks); err != nil { return err } @@ -123,31 +118,6 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt return newConvergence(options.Services, observedState, s).apply(ctx, project, options) } -func prepareVolumes(p *types.Project) error { - for i := range p.Services { - volumesFrom, dependServices, err := getVolumesFrom(p, p.Services[i].VolumesFrom) - if err != nil { - return err - } - p.Services[i].VolumesFrom = volumesFrom - if len(dependServices) > 0 { - if p.Services[i].DependsOn == nil { - p.Services[i].DependsOn = make(types.DependsOnConfig, len(dependServices)) - } - for _, service := range p.Services { - if utils.StringContains(dependServices, service.Name) && - p.Services[i].DependsOn[service.Name].Condition == "" { - p.Services[i].DependsOn[service.Name] = types.ServiceDependency{ - Condition: types.ServiceConditionStarted, - Required: true, - } - } - } - } - } - return nil -} - func prepareNetworks(project *types.Project) { for k, network := range project.Networks { network.Labels = network.Labels.Add(api.NetworkLabel, k) @@ -249,13 +219,6 @@ func (s *composeService) getCreateConfigs(ctx context.Context, if err != nil { return createConfigs{}, err } - var volumesFrom []string - for _, v := range service.VolumesFrom { - if !strings.HasPrefix(v, "container:") { - return createConfigs{}, fmt.Errorf("invalid volume_from: %s", v) - } - volumesFrom = append(volumesFrom, v[len("container:"):]) - } // NETWORKING links, err := s.getLinks(ctx, p.Name, service, number) @@ -296,7 +259,7 @@ func (s *composeService) getCreateConfigs(ctx context.Context, PortBindings: portBindings, Resources: resources, VolumeDriver: service.VolumeDriver, - VolumesFrom: volumesFrom, + VolumesFrom: service.VolumesFrom, DNS: service.DNS, DNSSearch: service.DNSSearch, DNSOptions: service.DNSOpts, @@ -676,40 +639,6 @@ func buildContainerPortBindingOptions(s types.ServiceConfig) nat.PortMap { return bindings } -func getVolumesFrom(project *types.Project, volumesFrom []string) ([]string, []string, error) { - var volumes = []string{} - var services = []string{} - // parse volumes_from - if len(volumesFrom) == 0 { - return volumes, services, nil - } - for _, vol := range volumesFrom { - spec := strings.Split(vol, ":") - if len(spec) == 0 { - continue - } - if spec[0] == "container" { - volumes = append(volumes, vol) - continue - } - serviceName := spec[0] - services = append(services, serviceName) - service, err := project.GetService(serviceName) - if err != nil { - return nil, nil, err - } - - firstContainer := getContainerName(project.Name, service, 1) - v := fmt.Sprintf("container:%s", firstContainer) - if len(spec) > 2 { - v = fmt.Sprintf("container:%s:%s", firstContainer, strings.Join(spec[1:], ":")) - } - volumes = append(volumes, v) - } - return volumes, services, nil - -} - func getDependentServiceFromMode(mode string) string { if strings.HasPrefix( mode, diff --git a/pkg/compose/create_test.go b/pkg/compose/create_test.go index b3a6b4fee56..26f7f4d1587 100644 --- a/pkg/compose/create_test.go +++ b/pkg/compose/create_test.go @@ -98,46 +98,6 @@ func TestPrepareNetworkLabels(t *testing.T) { })) } -func TestPrepareVolumes(t *testing.T) { - t.Run("adds dependency condition if service depends on volume from another service", func(t *testing.T) { - project := composetypes.Project{ - Name: "myProject", - Services: []composetypes.ServiceConfig{ - { - Name: "aService", - VolumesFrom: []string{"anotherService"}, - }, - { - Name: "anotherService", - }, - }, - } - err := prepareVolumes(&project) - assert.NilError(t, err) - assert.Equal(t, project.Services[0].DependsOn["anotherService"].Condition, composetypes.ServiceConditionStarted) - }) - t.Run("doesn't overwrite existing dependency condition", func(t *testing.T) { - project := composetypes.Project{ - Name: "myProject", - Services: []composetypes.ServiceConfig{ - { - Name: "aService", - VolumesFrom: []string{"anotherService"}, - DependsOn: map[string]composetypes.ServiceDependency{ - "anotherService": {Condition: composetypes.ServiceConditionHealthy, Required: true}, - }, - }, - { - Name: "anotherService", - }, - }, - } - err := prepareVolumes(&project) - assert.NilError(t, err) - assert.Equal(t, project.Services[0].DependsOn["anotherService"].Condition, composetypes.ServiceConditionHealthy) - }) -} - func TestBuildContainerMountOptions(t *testing.T) { project := composetypes.Project{ Name: "myProject", diff --git a/pkg/compose/run.go b/pkg/compose/run.go index 471af3b0146..e48f56a904c 100644 --- a/pkg/compose/run.go +++ b/pkg/compose/run.go @@ -58,9 +58,6 @@ func (s *composeService) RunOneOffContainer(ctx context.Context, project *types. } func (s *composeService) prepareRun(ctx context.Context, project *types.Project, opts api.RunOptions) (string, error) { - if err := prepareVolumes(project); err != nil { // all dependencies already checked, but might miss service img - return "", err - } service, err := project.GetService(opts.Service) if err != nil { return "", err diff --git a/pkg/e2e/fixtures/no-deps/volume-from.yaml b/pkg/e2e/fixtures/no-deps/volume-from.yaml new file mode 100644 index 00000000000..96b35761e0f --- /dev/null +++ b/pkg/e2e/fixtures/no-deps/volume-from.yaml @@ -0,0 +1,10 @@ +services: + app: + image: nginx:alpine + volumes_from: + - db + + db: + image: nginx:alpine + volumes: + - /var/data diff --git a/pkg/e2e/noDeps_test.go b/pkg/e2e/noDeps_test.go new file mode 100644 index 00000000000..8f87331d272 --- /dev/null +++ b/pkg/e2e/noDeps_test.go @@ -0,0 +1,44 @@ +//go:build !windows +// +build !windows + +/* + Copyright 2022 Docker Compose CLI authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package e2e + +import ( + "fmt" + "testing" + + "gotest.tools/v3/icmd" +) + +func TestNoDepsVolumeFrom(t *testing.T) { + c := NewParallelCLI(t) + const projectName = "e2e-no-deps-volume-from" + t.Cleanup(func() { + c.RunDockerComposeCmd(t, "--project-name", projectName, "down") + }) + + c.RunDockerComposeCmd(t, "-f", "fixtures/no-deps/volume-from.yaml", "--project-name", projectName, "up", "-d") + + c.RunDockerComposeCmd(t, "-f", "fixtures/no-deps/volume-from.yaml", "--project-name", projectName, "up", "--no-deps", "-d", "app") + + c.RunDockerCmd(t, "rm", "-f", fmt.Sprintf("%s-db-1", projectName)) + + res := c.RunDockerComposeCmdNoCheck(t, "-f", "fixtures/no-deps/volume-from.yaml", "--project-name", projectName, "up", "--no-deps", "-d", "app") + res.Assert(t, icmd.Expected{ExitCode: 1, Err: "cannot share volume with service db: container missing"}) +}