introduce ability to select service to be stopped by `compose down` #10552

ndeloof · 2023-05-10T15:26:18Z

What I did
introduce ability to select service to be stopped by compose down

Related issue
closes #10230

require #10555

(not mandatory) A picture of a cute animal, if possible in relation to what you did

glours

LGTM, I think you need to update the doc also

codecov · 2023-05-10T17:37:03Z

Codecov Report

Patch coverage: 60.97% and project coverage change: +0.12 🎉

Comparison is base (68c462e) 59.45% compared to head (abc9081) 59.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##               v2   #10552      +/-   ##
==========================================
+ Coverage   59.45%   59.57%   +0.12%     
==========================================
  Files         107      107              
  Lines        9364     9437      +73     
==========================================
+ Hits         5567     5622      +55     
- Misses       3220     3239      +19     
+ Partials      577      576       -1

Impacted Files	Coverage Δ
pkg/api/api.go	`43.39% <ø> (ø)`
pkg/progress/event.go	`80.28% <0.00%> (-7.42%)`	⬇️
pkg/utils/set.go	`0.00% <0.00%> (ø)`
pkg/compose/down.go	`64.70% <40.74%> (-1.39%)`	⬇️
cmd/compose/down.go	`79.16% <100.00%> (+0.44%)`	⬆️
pkg/compose/dependencies.go	`90.03% <100.00%> (+3.74%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

milas

Current code looks like a good starting point, but I think we'll have some special cases to look at.

For example:

services:
  a:
   image: nginx
  b:
   image: nginx
   depends_on: [a]

First, docker compose down b will fail because it'll try to remove the network:

❯ ~/dev/compose/bin/build/docker-compose up -d && ~/dev/compose/bin/build/docker-compose down b
[+] Building 0.0s (0/0)
[+] Running 2/2
 ✔ Container network-a-1  Running                                                                                                                     0.0s
 ✔ Container network-b-1  Started                                                                                                                     0.4s
[+] Running 2/1
 ✔ Container network-b-1    Removed                                                                                                                   0.1s
 ✘ Network network_default  Error                                                                                                                     0.0s
failed to remove network network_default: Error response from daemon: error while removing network: network network_default id 0a59b1a2a52def0193fd82617668d734ebfb33fc433cf80862e1db012c44ee1f has active endpoints

But also, what should happen if you docker compose down a? Currently, it'll just stop a even though b depends upon it 🤔

ndeloof · 2023-05-11T20:21:50Z

docker compose down b will fail because it'll try to remove the network:

yes, this is why this PR depends on #10555

what should happen if you docker compose down a?

good point. For symmetry with compose up we could also stop dependent service and include support for --no-deps flag

ndeloof · 2023-05-12T08:48:11Z

#10555 has been merged 🎉

implemented complimentary support for dependent services, walking the dependency graph from specified service nodes (actually walking the whole graph, but skipping nodes which aren't relevant for requested operation)

I haven't introduced --no-deps in this PR, this is something we could add if requested

glours · 2023-05-15T08:38:20Z

pkg/compose/down.go

+	// Check requested services exists in model
+	var services []string
+	for _, service := range options.Services {
+		_, err := project.GetService(service)
+		if err != nil {
+			if options.Project != nil {
+				// ran with an explicit compose.yaml file, so we should not ignore
+				return err
+			}
+		} else {
+			services = append(services, service)
+		}
+	}
+	options.Services = services


Can we extract this as a function, this way we won't need the //nolint:gocyclo and will make this code reusable elsewhere? WDYT?

unfortunately, even with this extracted into a func, cyclomatic complexity still is 17

moved to checkSelectedServices

glours

LGTM

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

… and down Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

milas

No issues with the code as-is, and stopping a service is depended upon by other services properly brings those down as well.

Volumes and --rmi flag are problematic, however:

name: pr10552

services:
  parent:
    image: nginx
    volumes:
      - aa:/a
  child:
    depends_on: [parent]
    image: nginx
    volumes:
      - aa:/a
      - bb:/b
  other:
    image: nginx
    volumes:
      - bb:/b

volumes:
 aa:
 bb:

Assume you've run compose up --wait first:

Images

$ compose down other --rmi=all
[+] Running 2/1
 ✔ Container pr10552-other-1  Removed                                                                                                                                          0.1s
 ⠋ Image nginx:latest         Removing                                                                                                                                         0.0s
 ! Network pr10552_default    Resource is still in use                                                                                                                         0.0s
Error response from daemon: conflict: unable to remove repository reference "nginx:latest" (must force) - container 9e9979562577 is using its referenced image b005e88565d7

Volumes

$ compose down child --volumes
[+] Running 3/1
 ✔ Container pr10552-child-1  Removed                                                                                                                                          0.1s
 ✔ Volume pr10552_bb          Removed                                                                                                                                          0.0s
 ⠋ Volume pr10552_aa          Removing                                                                                                                                         0.0s
 ! Network pr10552_default    Resource is still in use                                                                                                                         0.0s
Error response from daemon: remove pr10552_aa: volume is in use - [8199008ae17b920f984648b9d3f03bbd620c052007157413a0a7a4d8ae37ed2c]

For now, what do you think about blocking the usage of the --rmi and --volumes flags if len(services) != 0?

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof · 2023-05-24T14:31:17Z

@milas good point. We can rely on errConflict to detect and handle such circumstances (which could apply to other usages, not just down with service)

pushed abc9081 for this purpose:

$ docker compose down -v --rmi all
[+] Running 3/3
 ! Volume truc_toto      Resource is still in use                                                                                                 0.0s 
 ! Image nginx:latest    Resource is still in use                                                                                                 0.0s 
 ✔ Network truc_default  Removed

milas

🥳

Paraphraser · 2023-06-22T00:43:43Z

I just tried out v2.19.0. This feature is waaay cool. I really like the way it takes account of container dependency chains and cleans up dangling networks.

👏👏👏👏👏

glours approved these changes May 10, 2023

View reviewed changes

ndeloof force-pushed the down_service branch 3 times, most recently from 370590e to d1f890f Compare May 10, 2023 17:35

ndeloof force-pushed the down_service branch from d1f890f to 5e05752 Compare May 11, 2023 09:58

milas suggested changes May 11, 2023

View reviewed changes

ndeloof force-pushed the down_service branch from 5e05752 to a12b727 Compare May 12, 2023 08:43

ndeloof force-pushed the down_service branch 3 times, most recently from 7591270 to d6aad14 Compare May 12, 2023 10:02

ndeloof requested a review from milas May 12, 2023 10:17

glours requested changes May 15, 2023

View reviewed changes

ndeloof force-pushed the down_service branch from d6aad14 to 803a967 Compare May 15, 2023 09:56

ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, laurazard and glours and removed request for a team May 15, 2023 14:34

glours approved these changes May 17, 2023

View reviewed changes

ndeloof force-pushed the down_service branch from 803a967 to 3814062 Compare May 17, 2023 09:44

ndeloof added 2 commits May 24, 2023 14:45

introduce ability to select service to be stopped by compose down

2ac9299

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

introduce WithRootNodesAndDown to walk the graph from specified nodes…

dc6281f

… and down Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

ndeloof force-pushed the down_service branch from 3814062 to dc6281f Compare May 24, 2023 12:46

milas reviewed May 24, 2023

View reviewed changes

detect conflict removing volume/image and warn user

abc9081

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>

milas approved these changes May 24, 2023

View reviewed changes

ndeloof merged commit c7afc61 into docker:v2 May 24, 2023

ndeloof deleted the down_service branch May 24, 2023 15:34

laurazard mentioned this pull request Nov 7, 2023

Docker-compose down mistaking service for command when providing -f and -p #9627

Closed

heronimus mentioned this pull request Mar 19, 2024

chore: fix issue with 'make rollup' leaving celestia-devnet container running Argus-Labs/world-engine#680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce ability to select service to be stopped by `compose down` #10552

introduce ability to select service to be stopped by `compose down` #10552

ndeloof commented May 10, 2023 •

edited

Loading

glours left a comment

codecov bot commented May 10, 2023 •

edited

Loading

milas left a comment

ndeloof commented May 11, 2023

ndeloof commented May 12, 2023

glours May 15, 2023

ndeloof May 15, 2023

ndeloof May 15, 2023

glours left a comment

milas left a comment

ndeloof commented May 24, 2023

milas left a comment

Paraphraser commented Jun 22, 2023

introduce ability to select service to be stopped by compose down #10552

introduce ability to select service to be stopped by compose down #10552

Conversation

ndeloof commented May 10, 2023 • edited Loading

glours left a comment

Choose a reason for hiding this comment

codecov bot commented May 10, 2023 • edited Loading

Codecov Report

milas left a comment

Choose a reason for hiding this comment

ndeloof commented May 11, 2023

ndeloof commented May 12, 2023

glours May 15, 2023

Choose a reason for hiding this comment

ndeloof May 15, 2023

Choose a reason for hiding this comment

ndeloof May 15, 2023

Choose a reason for hiding this comment

glours left a comment

Choose a reason for hiding this comment

milas left a comment

Choose a reason for hiding this comment

Images

Volumes

ndeloof commented May 24, 2023

milas left a comment

Choose a reason for hiding this comment

Paraphraser commented Jun 22, 2023

introduce ability to select service to be stopped by `compose down` #10552

introduce ability to select service to be stopped by `compose down` #10552

ndeloof commented May 10, 2023 •

edited

Loading

codecov bot commented May 10, 2023 •

edited

Loading