up: handle various attach use cases better

By default, `compose up` attaches to all services (i.e.
shows log output from every associated container). If
a service is specified, e.g. `compose up foo`, then
only `foo`'s logs are tailed. The `--attach-dependencies`
flag can also be used, so that if `foo` depended upon
`bar`, then `bar`'s logs would also be followed. It's
also possible to use `--no-attach` to filter out one
or more services explicitly, e.g. `compose up --no-attach=noisy`
would launch all services, including `noisy`, and would
show log output from every service _except_ `noisy`.
Lastly, it's possible to use `up --attach` to explicitly
restrict to a subset of services (or their dependencies).

How these flags interact with each other is also worth
thinking through.

There were a few different connected issues here, but
the primary issue was that running `compose up foo` was
always attaching dependencies regardless of `--attach-dependencies`.

The filtering logic here has been updated so that it
behaves predictably both when launching all services
(`compose up`) or a subset (`compose up foo`) as well
as various flag combinations on top of those.

Notably, this required making some changes to how it
watches containers. The logic here between attaching
for logs and monitoring for lifecycle changes is
tightly coupled, so some changes were needed to ensure
that the full set of services being `up`'d are _watched_
and the subset that should have logs shown are _attached_.
(This does mean faking the attach with an event but not
actually doing it.)

While handling that, I adjusted the context lifetimes
here, which improves error handling that gets shown to
the user and should help avoid potential leaks by getting
rid of a `context.Background()`.

Signed-off-by: Milas Bowman <[email protected]>

Author: milas

cmd/compose/up.go

@@ -18,7 +18,9 @@ package compose
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/compose-spec/compose-go/types"
@@ -83,7 +85,10 @@ func upCommand(p *ProjectOptions, streams api.Streams, backend api.Service) *cob
 		RunE: p.WithServices(func(ctx context.Context, project *types.Project, services []string) error {
 			create.ignoreOrphans = utils.StringToBool(project.Environment[ComposeIgnoreOrphans])
 			if create.ignoreOrphans && create.removeOrphans {
-				return fmt.Errorf("%s and --remove-orphans cannot be combined", ComposeIgnoreOrphans)
+				return fmt.Errorf("cannot combine %s and --remove-orphans", ComposeIgnoreOrphans)
+			}
+			if len(up.attach) != 0 && up.attachDependencies {
+				return errors.New("cannot combine --attach and --attach-dependencies")
 			}
 			return runUp(ctx, streams, backend, create, up, project, services)
 		}),
@@ -108,12 +113,12 @@ func upCommand(p *ProjectOptions, streams api.Streams, backend api.Service) *cob
 	flags.BoolVar(&up.noDeps, "no-deps", false, "Don't start linked services.")
 	flags.BoolVar(&create.recreateDeps, "always-recreate-deps", false, "Recreate dependent containers. Incompatible with --no-recreate.")
 	flags.BoolVarP(&create.noInherit, "renew-anon-volumes", "V", false, "Recreate anonymous volumes instead of retrieving data from the previous containers.")
-	flags.BoolVar(&up.attachDependencies, "attach-dependencies", false, "Attach to dependent containers.")
 	flags.BoolVar(&create.quietPull, "quiet-pull", false, "Pull without printing progress information.")
-	flags.StringArrayVar(&up.attach, "attach", []string{}, "Attach to service output.")
-	flags.StringArrayVar(&up.noAttach, "no-attach", []string{}, "Don't attach to specified service.")
+	flags.StringArrayVar(&up.attach, "attach", []string{}, "Restrict attaching to the specified services. Incompatible with --attach-dependencies.")
+	flags.StringArrayVar(&up.noAttach, "no-attach", []string{}, "Do not attach (stream logs) to the specified services.")
+	flags.BoolVar(&up.attachDependencies, "attach-dependencies", false, "Automatically attach to log output of dependent services.")
 	flags.BoolVar(&up.wait, "wait", false, "Wait for services to be running|healthy. Implies detached mode.")
-	flags.IntVar(&up.waitTimeout, "wait-timeout", 0, "timeout waiting for application to be running|healthy.")
+	flags.IntVar(&up.waitTimeout, "wait-timeout", 0, "Maximum duration to wait for the project to be running|healthy.")
 
 	return upCmd
 }
@@ -158,37 +163,6 @@ func runUp(ctx context.Context, streams api.Streams, backend api.Service, create
 		return err
 	}
 
-	var consumer api.LogConsumer
-	if !upOptions.Detach {
-		consumer = formatter.NewLogConsumer(ctx, streams.Out(), streams.Err(), !upOptions.noColor, !upOptions.noPrefix, upOptions.timestamp)
-	}
-
-	attachTo := utils.Set[string]{}
-	if len(upOptions.attach) > 0 {
-		attachTo.AddAll(upOptions.attach...)
-	}
-	if upOptions.attachDependencies {
-		if err := project.WithServices(attachTo.Elements(), func(s types.ServiceConfig) error {
-			if s.Attach == nil || *s.Attach {
-				attachTo.Add(s.Name)
-			}
-			return nil
-		}); err != nil {
-			return err
-		}
-	}
-	if len(attachTo) == 0 {
-		if err := project.WithServices(services, func(s types.ServiceConfig) error {
-			if s.Attach == nil || *s.Attach {
-				attachTo.Add(s.Name)
-			}
-			return nil
-		}); err != nil {
-			return err
-		}
-	}
-	attachTo.RemoveAll(upOptions.noAttach...)
-
 	create := api.CreateOptions{
 		Services:             services,
 		RemoveOrphans:        createOptions.removeOrphans,
@@ -204,14 +178,48 @@ func runUp(ctx context.Context, streams api.Streams, backend api.Service, create
 		return backend.Create(ctx, project, create)
 	}
 
-	timeout := time.Duration(upOptions.waitTimeout) * time.Second
+	var consumer api.LogConsumer
+	var attach []string
+	if !upOptions.Detach {
+		consumer = formatter.NewLogConsumer(ctx, streams.Out(), streams.Err(), !upOptions.noColor, !upOptions.noPrefix, upOptions.timestamp)
 
+		var attachSet utils.Set[string]
+		if len(upOptions.attach) != 0 {
+			// services are passed explicitly with --attach, verify they're valid and then use them as-is
+			attachSet = utils.NewSet(upOptions.attach...)
+			unexpectedSvcs := attachSet.Diff(utils.NewSet(project.ServiceNames()...))
+			if len(unexpectedSvcs) != 0 {
+				return fmt.Errorf("cannot attach to services not included in up: %s", strings.Join(unexpectedSvcs.Elements(), ", "))
+			}
+		} else {
+			// mark services being launched (and potentially their deps) for attach
+			// if they didn't opt-out via Compose YAML
+			attachSet = utils.NewSet[string]()
+			var dependencyOpt types.DependencyOption = types.IgnoreDependencies
+			if upOptions.attachDependencies {
+				dependencyOpt = types.IncludeDependencies
+			}
+			if err := project.WithServices(services, func(s types.ServiceConfig) error {
+				if s.Attach == nil || *s.Attach {
+					attachSet.Add(s.Name)
+				}
+				return nil
+			}, dependencyOpt); err != nil {
+				return err
+			}
+		}
+		// filter out any services that have been explicitly marked for ignore with `--no-attach`
+		attachSet.RemoveAll(upOptions.noAttach...)
+		attach = attachSet.Elements()
+	}
+
+	timeout := time.Duration(upOptions.waitTimeout) * time.Second
 	return backend.Up(ctx, project, api.UpOptions{
 		Create: create,
 		Start: api.StartOptions{
 			Project:      project,
 			Attach:       consumer,
-			AttachTo:     attachTo.Elements(),
+			AttachTo:     attach,
 			ExitCodeFrom: upOptions.exitCodeFrom,
 			CascadeStop:  upOptions.cascadeStop,
 			Wait:         upOptions.wait,

docs/reference/compose_up.md

@@ -9,14 +9,14 @@ Create and start containers
 |:-----------------------------|:--------------|:----------|:---------------------------------------------------------------------------------------------------------|
 | `--abort-on-container-exit`  |               |           | Stops all containers if any container was stopped. Incompatible with -d                                  |
 | `--always-recreate-deps`     |               |           | Recreate dependent containers. Incompatible with --no-recreate.                                          |
-| `--attach`                   | `stringArray` |           | Attach to service output.                                                                                |
-| `--attach-dependencies`      |               |           | Attach to dependent containers.                                                                          |
+| `--attach`                   | `stringArray` |           | Restrict attaching to the specified services. Incompatible with --attach-dependencies.                   |
+| `--attach-dependencies`      |               |           | Automatically attach to log output of dependent services.                                                |
 | `--build`                    |               |           | Build images before starting containers.                                                                 |
 | `-d`, `--detach`             |               |           | Detached mode: Run containers in the background                                                          |
 | `--dry-run`                  |               |           | Execute command in dry run mode                                                                          |
 | `--exit-code-from`           | `string`      |           | Return the exit code of the selected service container. Implies --abort-on-container-exit                |
 | `--force-recreate`           |               |           | Recreate containers even if their configuration and image haven't changed.                               |
-| `--no-attach`                | `stringArray` |           | Don't attach to specified service.                                                                       |
+| `--no-attach`                | `stringArray` |           | Do not attach (stream logs) to the specified services.                                                   |
 | `--no-build`                 |               |           | Don't build an image, even if it's missing.                                                              |
 | `--no-color`                 |               |           | Produce monochrome output.                                                                               |
 | `--no-deps`                  |               |           | Don't start linked services.                                                                             |
@@ -31,7 +31,7 @@ Create and start containers
 | `-t`, `--timeout`            | `int`         | `0`       | Use this timeout in seconds for container shutdown when attached or when containers are already running. |
 | `--timestamps`               |               |           | Show timestamps.                                                                                         |
 | `--wait`                     |               |           | Wait for services to be running\|healthy. Implies detached mode.                                         |
-| `--wait-timeout`             | `int`         | `0`       | timeout waiting for application to be running\|healthy.                                                  |
+| `--wait-timeout`             | `int`         | `0`       | Maximum duration to wait for the project to be running\|healthy.                                         |
 
 
 <!---MARKER_GEN_END-->

docs/reference/docker_compose_up.yaml

@@ -48,7 +48,8 @@ options:
     - option: attach
       value_type: stringArray
       default_value: '[]'
-      description: Attach to service output.
+      description: |
+        Restrict attaching to the specified services. Incompatible with --attach-dependencies.
       deprecated: false
       hidden: false
       experimental: false
@@ -58,7 +59,7 @@ options:
     - option: attach-dependencies
       value_type: bool
       default_value: "false"
-      description: Attach to dependent containers.
+      description: Automatically attach to log output of dependent services.
       deprecated: false
       hidden: false
       experimental: false
@@ -110,7 +111,7 @@ options:
     - option: no-attach
       value_type: stringArray
       default_value: '[]'
-      description: Don't attach to specified service.
+      description: Do not attach (stream logs) to the specified services.
       deprecated: false
       hidden: false
       experimental: false
@@ -266,7 +267,7 @@ options:
     - option: wait-timeout
       value_type: int
       default_value: "0"
-      description: timeout waiting for application to be running|healthy.
+      description: Maximum duration to wait for the project to be running|healthy.
       deprecated: false
       hidden: false
       experimental: false

pkg/compose/start.go

@@ -56,17 +56,48 @@ func (s *composeService) start(ctx context.Context, projectName string, options
 		}
 	}
 
-	eg, ctx := errgroup.WithContext(ctx)
+	// use an independent context tied to the errgroup for background attach operations
+	// the primary context is still used for other operations
+	// this means that once any attach operation fails, all other attaches are cancelled,
+	// but an attach failing won't interfere with the rest of the start
+	eg, attachCtx := errgroup.WithContext(ctx)
 	if listener != nil {
-		attached, err := s.attach(ctx, project, listener, options.AttachTo)
+		_, err := s.attach(attachCtx, project, listener, options.AttachTo)
 		if err != nil {
 			return err
 		}
 
 		eg.Go(func() error {
-			return s.watchContainers(context.Background(), project.Name, options.AttachTo, options.Services, listener, attached,
+			// it's possible to have a required service whose log output is not desired
+			// (i.e. it's not in the attach set), so watch everything and then filter
+			// calls to attach; this ensures that `watchContainers` blocks until all
+			// required containers have exited, even if their output is not being shown
+			attachTo := utils.NewSet[string](options.AttachTo...)
+			required := utils.NewSet[string](options.Services...)
+			toWatch := attachTo.Union(required).Elements()
+
+			containers, err := s.getContainers(ctx, projectName, oneOffExclude, true, toWatch...)
+			if err != nil {
+				return err
+			}
+
+			// N.B. this uses the parent context (instead of attachCtx) so that the watch itself can
+			// continue even if one of the log streams fails
+			return s.watchContainers(ctx, project.Name, toWatch, required.Elements(), listener, containers,
 				func(container moby.Container, _ time.Time) error {
-					return s.attachContainer(ctx, container, listener)
+					svc := container.Labels[api.ServiceLabel]
+					if attachTo.Has(svc) {
+						return s.attachContainer(attachCtx, container, listener)
+					}
+
+					// HACK: simulate an "attach" event
+					listener(api.ContainerEvent{
+						Type:      api.ContainerEventAttach,
+						Container: getContainerNameWithoutProject(container),
+						ID:        container.ID,
+						Service:   svc,
+					})
+					return nil
 				}, func(container moby.Container, _ time.Time) error {
 					listener(api.ContainerEvent{
 						Type:      api.ContainerEventAttach,
@@ -156,6 +187,13 @@ func (s *composeService) watchContainers(ctx context.Context, //nolint:gocyclo
 		required = services
 	}
 
+	unexpected := utils.NewSet[string](required...).Diff(utils.NewSet[string](services...))
+	if len(unexpected) != 0 {
+		return fmt.Errorf(`required service(s) "%s" not present in watched service(s) "%s"`,
+			strings.Join(unexpected.Elements(), ", "),
+			strings.Join(services, ", "))
+	}
+
 	// predicate to tell if a container we receive event for should be considered or ignored
 	ofInterest := func(c moby.Container) bool {
 		if len(services) > 0 {
@@ -190,6 +228,12 @@ func (s *composeService) watchContainers(ctx context.Context, //nolint:gocyclo
 	err := s.Events(ctx, projectName, api.EventsOptions{
 		Services: services,
 		Consumer: func(event api.Event) error {
+			defer func() {
+				// after consuming each event, check to see if we're done
+				if len(expected) == 0 {
+					stop()
+				}
+			}()
 			inspected, err := s.apiClient().ContainerInspect(ctx, event.Container)
 			if err != nil {
 				if errdefs.IsNotFound(err) {
@@ -291,9 +335,6 @@ func (s *composeService) watchContainers(ctx context.Context, //nolint:gocyclo
 					}
 				}
 			}
-			if len(expected) == 0 {
-				stop()
-			}
 			return nil
 		},
 	})

pkg/utils/set.go

@@ -16,6 +16,23 @@ package utils
 
 type Set[T comparable] map[T]struct{}
 
+func NewSet[T comparable](v ...T) Set[T] {
+	if len(v) == 0 {
+		return make(Set[T])
+	}
+
+	out := make(Set[T], len(v))
+	for i := range v {
+		out.Add(v[i])
+	}
+	return out
+}
+
+func (s Set[T]) Has(v T) bool {
+	_, ok := s[v]
+	return ok
+}
+
 func (s Set[T]) Add(v T) {
 	s[v] = struct{}{}
 }
@@ -53,3 +70,24 @@ func (s Set[T]) RemoveAll(elements ...T) {
 		s.Remove(e)
 	}
 }
+
+func (s Set[T]) Diff(other Set[T]) Set[T] {
+	out := make(Set[T])
+	for k := range s {
+		if _, ok := other[k]; !ok {
+			out[k] = struct{}{}
+		}
+	}
+	return out
+}
+
+func (s Set[T]) Union(other Set[T]) Set[T] {
+	out := make(Set[T])
+	for k := range s {
+		out[k] = struct{}{}
+	}
+	for k := range other {
+		out[k] = struct{}{}
+	}
+	return out
+}

pkg/utils/set_test.go

@@ -0,0 +1,41 @@
+/*
+   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 utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestSet_Has(t *testing.T) {
+	x := NewSet[string]("value")
+	require.True(t, x.Has("value"))
+	require.False(t, x.Has("VALUE"))
+}
+
+func TestSet_Diff(t *testing.T) {
+	a := NewSet[int](1, 2)
+	b := NewSet[int](2, 3)
+	require.ElementsMatch(t, []int{1}, a.Diff(b).Elements())
+	require.ElementsMatch(t, []int{3}, b.Diff(a).Elements())
+}
+
+func TestSet_Union(t *testing.T) {
+	a := NewSet[int](1, 2)
+	b := NewSet[int](2, 3)
+	require.ElementsMatch(t, []int{1, 2, 3}, a.Union(b).Elements())
+	require.ElementsMatch(t, []int{1, 2, 3}, b.Union(a).Elements())
+}