don't rely on depends_on to resolve volume_from, better use observed state

Signed-off-by: Nicolas De Loof <[email protected]>

Author: ndeloof

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

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,

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",

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

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

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"})
+}