From d1a7480de86f453c9fbff4f04da64a2cb1e96406 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 14:59:09 +0200 Subject: [PATCH] fix(prune): scope container pruning to a role and destination `Kamal::Commands::Prune#app_containers` selected removal candidates with `label=service=` alone and kept the newest `retain` of them. Two consequences: - Roles share the window. All roles deploy at the same version, so with four roles and `retain: 5` only about the last one-and-a-quarter deploys survive, and a busy sibling role pushes another role's current container out of the window. - Destinations share it too, so pruning staging could remove production's containers on a shared host. That is a correctness problem, not just disk hygiene: a container kamal-proxy has put to sleep for scale-to-zero is `exited`, so it is a removal candidate. Once it is removed the proxy still holds its reference and every wake 404s, leaving the service returning 503 until someone redeploys. Prune now runs once per role on each host, filtering on `service`, `destination` and `role` - the same filter set `Kamal::Commands::App` already uses. Since `retain` is validated to be at least 1, a role's newest container is now never removed, and a slept container is always its role's newest because sleeping happens to the current release. Refs #62 --- lib/kamal/cli/prune.rb | 11 ++++--- lib/kamal/commands/prune.rb | 18 ++++++++-- .../configuration/docs/configuration.yml | 3 +- test/cli/proxy_test.rb | 2 +- test/cli/prune_test.rb | 15 ++++++--- test/commands/prune_test.rb | 33 +++++++++++++++---- 6 files changed, 64 insertions(+), 18 deletions(-) diff --git a/lib/kamal/cli/prune.rb b/lib/kamal/cli/prune.rb index 4a2e44f24..7532ade82 100644 --- a/lib/kamal/cli/prune.rb +++ b/lib/kamal/cli/prune.rb @@ -18,16 +18,19 @@ def images end end - desc "containers", "Prune all stopped containers, except the last n (default 5)" - option :retain, type: :numeric, default: nil, desc: "Number of containers to retain" + desc "containers", "Prune all stopped containers, except the last n per role (default 5)" + option :retain, type: :numeric, default: nil, desc: "Number of containers to retain per role" def containers retain = options.fetch(:retain, KAMAL.config.retain_containers) raise "retain must be at least 1" if retain < 1 modify(lock: true) do - on(KAMAL.hosts) do + on(KAMAL.hosts) do |host| execute *KAMAL.auditor.record("Pruned containers"), verbosity: :debug - execute *KAMAL.prune.app_containers(retain: retain) + + KAMAL.roles_on(host).each do |role| + execute *KAMAL.prune.app_containers(retain: retain, role: role) + end end end end diff --git a/lib/kamal/commands/prune.rb b/lib/kamal/commands/prune.rb index 30b6eafd3..0b8b40cff 100644 --- a/lib/kamal/commands/prune.rb +++ b/lib/kamal/commands/prune.rb @@ -13,9 +13,15 @@ def tagged_images "while read image tag; do docker rmi $tag; done" end - def app_containers(retain:) + # Scoped to one role so a busy sibling role cannot push another role's newest + # container past the retain window. That matters beyond disk hygiene: a + # container kamal-proxy has put to sleep is `exited`, so it is a removal + # candidate, and once it is gone every wake 404s. With `retain >= 1` a role's + # newest container always survives, and the slept one is always the newest — + # sleeping happens to the current release. + def app_containers(retain:, role:) pipe \ - docker(:ps, "-q", "-a", *service_filter, *stopped_containers_filters), + docker(:ps, "-q", "-a", *service_filter, *destination_filter, *role_filter(role), *stopped_containers_filters), "tail -n +#{retain + 1}", "while read container_id; do docker rm $container_id; done" end @@ -35,4 +41,12 @@ def active_image_list def service_filter [ "--filter", "label=service=#{config.service}" ] end + + def destination_filter + [ "--filter", "label=destination=#{config.destination}" ] + end + + def role_filter(role) + [ "--filter", "label=role=#{role}" ] + end end diff --git a/lib/kamal/configuration/docs/configuration.yml b/lib/kamal/configuration/docs/configuration.yml index 81d2c42fa..6872417c8 100644 --- a/lib/kamal/configuration/docs/configuration.yml +++ b/lib/kamal/configuration/docs/configuration.yml @@ -136,7 +136,8 @@ allow_empty_roles: false # Retain containers # -# How many old containers and images we retain, defaults to 5: +# How many old containers we retain per role, and how many images we retain, +# defaults to 5: retain_containers: 3 # Minimum version diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index ca50b24ea..a83ddb222 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -247,7 +247,7 @@ class CliProxyTest < CliTestCase assert_match "docker container ls --all --filter 'name=^app-web-12345678$' --quiet | xargs docker stop", output assert_match "docker tag dhh/app:latest dhh/app:latest", output assert_match "/usr/bin/env mkdir -p .kamal", output - assert_match "docker ps -q -a --filter label=service=app --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done", output + assert_match "docker ps -q -a --filter label=service=app --filter label=destination= --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done", output assert_match "docker image prune --force --filter label=service=app", output assert_match "Upgraded proxy on 1.1.1.1,1.1.1.2,1.1.1.3,1.1.1.4", output end diff --git a/test/cli/prune_test.rb b/test/cli/prune_test.rb index bb4ced51a..3146cc9af 100644 --- a/test/cli/prune_test.rb +++ b/test/cli/prune_test.rb @@ -17,11 +17,11 @@ class CliPruneTest < CliTestCase test "containers" do run_command("containers").tap do |output| - assert_match /docker ps -q -a --filter label=service=app --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done on 1.1.1.\d/, output + assert_match /docker ps -q -a --filter label=service=app --filter label=destination= --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n \+6 | while read container_id; do docker rm \$container_id; done on 1.1.1.\d/, output end run_command("containers", "--retain", "10").tap do |output| - assert_match /docker ps -q -a --filter label=service=app --filter status=created --filter status=exited --filter status=dead | tail -n +11 | while read container_id; do docker rm $container_id; done on 1.1.1.\d/, output + assert_match /docker ps -q -a --filter label=service=app --filter label=destination= --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n \+11 | while read container_id; do docker rm \$container_id; done on 1.1.1.\d/, output end assert_raises(RuntimeError, "retain must be at least 1") do @@ -29,8 +29,15 @@ class CliPruneTest < CliTestCase end end + test "containers prunes every role on the host separately" do + run_command("containers", config_file: "test/fixtures/deploy_with_roles.yml").tap do |output| + assert_match "--filter label=role=web --filter status=created", output + assert_match "--filter label=role=workers --filter status=created", output + end + end + private - def run_command(*command) - stdouted { Kamal::Cli::Prune.start([ *command, "-c", "test/fixtures/deploy_with_accessories.yml" ]) } + def run_command(*command, config_file: "test/fixtures/deploy_with_accessories.yml") + stdouted { Kamal::Cli::Prune.start([ *command, "-c", config_file ]) } end end diff --git a/test/commands/prune_test.rb b/test/commands/prune_test.rb index 50e852bcb..22e8f0ab3 100644 --- a/test/commands/prune_test.rb +++ b/test/commands/prune_test.rb @@ -22,16 +22,37 @@ class CommandsPruneTest < ActiveSupport::TestCase test "app containers" do assert_equal \ - "docker ps -q -a --filter label=service=app --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done", - new_command.app_containers(retain: 5).join(" ") + "docker ps -q -a --filter label=service=app --filter label=destination= --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done", + new_command.app_containers(retain: 5, role: role(:web)).join(" ") assert_equal \ - "docker ps -q -a --filter label=service=app --filter status=created --filter status=exited --filter status=dead | tail -n +4 | while read container_id; do docker rm $container_id; done", - new_command.app_containers(retain: 3).join(" ") + "docker ps -q -a --filter label=service=app --filter label=destination= --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n +4 | while read container_id; do docker rm $container_id; done", + new_command.app_containers(retain: 3, role: role(:web)).join(" ") + end + + test "app containers are scoped to the role, so a sibling role's deploys can't push a slept container past the retain window" do + @config[:servers] = { "web" => [ "1.1.1.1" ], "workers" => [ "1.1.1.2" ] } + + assert_match "--filter label=role=workers", new_command.app_containers(retain: 5, role: role(:workers)).join(" ") + assert_no_match(/--filter label=role=web /, new_command.app_containers(retain: 5, role: role(:workers)).join(" ")) + end + + test "app containers are scoped to the destination" do + assert_equal \ + "docker ps -q -a --filter label=service=app --filter label=destination=staging --filter label=role=web --filter status=created --filter status=exited --filter status=dead | tail -n +6 | while read container_id; do docker rm $container_id; done", + new_command(destination: "staging").app_containers(retain: 5, role: role(:web, destination: "staging")).join(" ") end private - def new_command - Kamal::Commands::Prune.new(Kamal::Configuration.new(@config, version: "123")) + def new_command(destination: nil) + Kamal::Commands::Prune.new(config(destination: destination)) + end + + def config(destination: nil) + Kamal::Configuration.new(@config, version: "123", destination: destination) + end + + def role(name, destination: nil) + config(destination: destination).role(name) end end