From 095e56600a0ed72c1ffd5ac26639883e8b194d06 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 18:57:40 +0200 Subject: [PATCH 1/2] perf(proxy): stop paying the stage-3c bridge on a host that has already migrated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary `Ensure dash-proxy` is the most expensive row of a deploy by round trips, and on a host that has already been through the 3c rename most of what it did was checking, again, that it had. The four bridge commands become one, guarded on a marker in the run directory: `test -f .dash/proxy/.legacy-renamed || ( bridge && copy && replace && mark )`. A migrated host — and a host installed fresh on 4.x that never had a kamal-proxy — runs no docker command for the bridge at all. The command is folded into the apps-config `mkdir -p` the host pays anyway, so the round trip goes too. The marker is written only on verified absence of both legacy containers, never on the chain's exit status: the removals end in `|| true`, and a host whose stop failed must retry next deploy rather than record itself as done. A failed volume copy still aborts the boot through the && chain, exactly as today. `container_id`, `config_digest` and `version` were three docker invocations against the same container; one `docker inspect --format` returns all three, parsed by Dash::Commands::Proxy::State. Drift owns the capture and the minimum-version gate reads the tag off it. `dash doctor`'s drift check gets the same 2 -> 1 for free. Nothing changes for a host still running kamal-proxy: the same three steps, in the same documented order, before anything reads the new container, volume or network. Per proxy host, with a running proxy: 12 -> 6 round trips. On the load balancer host: 10 -> 7. ## Test Coverage - commands/proxy_test: the marker guard, the documented step order, per-step subshells, the verified-absence marker write, the copy left free to fail, the apps-config fold, the inspect format, and State's parsing (tag past a registry port, no container, unlabelled container) - commands/loadbalancer_test: the same for the two-step loadbalancer variant, whose marker is verified on the volume instead - cli/proxy_test: the per-host round-trip sequence pinned exactly, and a still-legacy host getting the whole bridge in order ## Verification - [x] bundle exec rubocop --parallel passes - [x] bin/test (unit + integration) passes - [x] generated shell exercised against real sh and bash with a fake docker Refs #160 --- lib/dash/cli/proxy.rb | 25 ++--- lib/dash/cli/proxy/drift.rb | 19 +++- lib/dash/cli/proxy/legacy_rename.rb | 29 ++---- lib/dash/commands/base.rb | 15 ++- lib/dash/commands/loadbalancer.rb | 40 ++++++++ lib/dash/commands/proxy.rb | 64 ++++++++++++- lib/dash/commands/proxy/state.rb | 31 +++++++ lib/dash/configuration/proxy.rb | 3 + test/cli/proxy_test.rb | 138 +++++++++++++++++++--------- test/commands/loadbalancer_test.rb | 44 +++++++++ test/commands/proxy_test.rb | 108 ++++++++++++++++++++++ 11 files changed, 435 insertions(+), 81 deletions(-) create mode 100644 lib/dash/commands/proxy/state.rb diff --git a/lib/dash/cli/proxy.rb b/lib/dash/cli/proxy.rb index d0df5c8e..8c7c2c95 100644 --- a/lib/dash/cli/proxy.rb +++ b/lib/dash/cli/proxy.rb @@ -22,7 +22,9 @@ def boot execute *DASH.registry.login # Before anything reads the new container, volume or network: bring a - # host still on pre-rename identity across. A no-op once it has been. + # host still on pre-rename identity across, and make the apps-config + # directory in the same round trip. Nothing but a `test -f` once it has + # been - see Dash::Cli::Proxy::LegacyRename. Dash::Cli::Proxy::LegacyRename.new(host, self).run proxy = DASH.proxy(host) @@ -34,7 +36,8 @@ def boot else stale_hosts << host.to_s if drift.drifted? - version = capture_with_info(*proxy.version).strip.presence + # The tag off the inspect the drift check already made, not a read of its own. + version = drift.version if version && Dash::Utils.older_version?(version, Dash::Configuration::Proxy::Run::MINIMUM_VERSION) raise "dash-proxy version #{version} is too old, run `dash proxy reboot` in order to update to at least #{Dash::Configuration::Proxy::Run::MINIMUM_VERSION}" @@ -48,7 +51,6 @@ def boot execute *proxy.remove_proxy_secrets_file, raise_on_non_zero_exit: false end - execute *proxy.ensure_apps_config_directory execute *proxy.start_holder_or_run if proxy.port_holder? execute *proxy.start_or_run(digest: drift.expected_digest) end @@ -77,10 +79,10 @@ def boot execute *DASH.registry.login # Bring a pre-rename host across before the container can be created - # against an empty volume or a network nothing else joined. A no-op - # once it has been. - execute *DASH.docker.connect_legacy_network_containers - execute *DASH.loadbalancer.copy_legacy_config_volume + # against an empty volume or a network nothing else joined, and make + # the apps-config directory in the same round trip. Nothing but a + # `test -f` once it has been. + execute *DASH.loadbalancer.prepare_boot # The load balancer terminates TLS and owns the cache, so its host # needs the proxy secrets (acme credentials, cache store) just like @@ -92,8 +94,6 @@ def boot execute *DASH.loadbalancer.remove_proxy_secrets_file, raise_on_non_zero_exit: false end - execute *DASH.loadbalancer.ensure_apps_config_directory - # TLS terminates at the load balancer, so the TLS material the app # hosts get - custom certificates and the mTLS client CA - must # reach this host too; the LB container reads it through the same @@ -105,10 +105,11 @@ def boot # The same drift detection the proxy hosts get: a loadbalancer booted # with a different config digest reboots below (or warns, when # automatic reboot is off) instead of serving a stale config forever. - container_id = capture_with_info(*DASH.loadbalancer.container_id, raise_on_non_zero_exit: false).strip - current_digest = capture_with_info(*DASH.loadbalancer.config_digest, raise_on_non_zero_exit: false).strip + state = Dash::Commands::Proxy::State.parse( + capture_with_info(*DASH.loadbalancer.inspect_state, raise_on_non_zero_exit: false) + ) - if container_id.present? && current_digest != DASH.loadbalancer_config.run_config_digest + if state.exists? && state.digest.to_s != DASH.loadbalancer_config.run_config_digest if auto_reboot # Leave the old loadbalancer serving until its reboot below. lb_drifted << host.to_s diff --git a/lib/dash/cli/proxy/drift.rb b/lib/dash/cli/proxy/drift.rb index cf3fb9a3..16020c4d 100644 --- a/lib/dash/cli/proxy/drift.rb +++ b/lib/dash/cli/proxy/drift.rb @@ -7,8 +7,23 @@ def initialize(host, sshkit) @sshkit = sshkit end + # One `docker inspect` for everything a boot asks about the running proxy: whether it + # exists, which image tag it runs, and the digest it was booted with. Captured once per + # instance - `dash proxy boot` reads all three off it, and `dash doctor` only the first. + def state + @state ||= Dash::Commands::Proxy::State.parse( + capture_with_info(*proxy.inspect_state, raise_on_non_zero_exit: false) + ) + end + def container_exists? - capture_with_info(*proxy.container_id, raise_on_non_zero_exit: false).strip.present? + state.exists? + end + + # The tag the running proxy was booted from, for the minimum-version gate. Nil when + # nothing is running - a host with no proxy has no version to be too old. + def version + state.version end # A proxy container has drifted when it was started with a different config @@ -30,7 +45,7 @@ def expected_digest private def current_digest - capture_with_info(*proxy.config_digest, raise_on_non_zero_exit: false).strip + state.digest.to_s end def proxy diff --git a/lib/dash/cli/proxy/legacy_rename.rb b/lib/dash/cli/proxy/legacy_rename.rb index 235d1e63..c3612f98 100644 --- a/lib/dash/cli/proxy/legacy_rename.rb +++ b/lib/dash/cli/proxy/legacy_rename.rb @@ -23,6 +23,13 @@ # so a second deploy is a no-op. Nothing here removes the legacy network or # volume: an operator who wants them gone removes them by hand, and stage 3d # deletes this class outright. +# +# All three travel as one command (Dash::Commands::Proxy#prepare_boot), guarded on a +# marker the host writes once it is verifiably past the rename - so a migrated host, and +# a host installed fresh on 4.x that never had a kamal-proxy, run no docker command here +# at all. The round trip itself is one the host already pays: the command carries the +# apps-config `mkdir -p` too, which reads nothing the bridge writes. Stage 3d keeps the +# mkdir and deletes the rest. class Dash::Cli::Proxy::LegacyRename attr_reader :host, :sshkit delegate :execute, to: :sshkit @@ -33,26 +40,6 @@ def initialize(host, sshkit) end def run - bridge_network - adopt_config_volume - replace_legacy_container + execute *DASH.proxy(host).prepare_boot end - - private - def bridge_network - execute *DASH.docker.connect_legacy_network_containers - end - - def adopt_config_volume - execute *DASH.proxy(host).copy_legacy_config_volume - end - - # The drain timeout the proxy is configured with, so a busy host is not cut - # off mid-request any more abruptly than a normal reboot would. - def replace_legacy_container - proxy = DASH.proxy(host) - - execute *proxy.remove_legacy_container(timeout: DASH.config.drain_timeout) - execute *proxy.remove_legacy_holder_container - end end diff --git a/lib/dash/commands/base.rb b/lib/dash/commands/base.rb index dab1de77..9075fd05 100644 --- a/lib/dash/commands/base.rb +++ b/lib/dash/commands/base.rb @@ -114,6 +114,13 @@ def chain(*commands) combine *commands, by: ";" end + # One subshell around an && chain. Composing two builders that each mix && and || + # cannot be done flat - the operators share precedence and associate left, so the + # second builder's guards re-associate across the first one's. + def group(*commands) + [ "(", *combine(*commands), ")" ] + end + def pipe(*commands) combine *commands, by: "|" end @@ -150,13 +157,19 @@ def copy_legacy_volume(legacy:, volume:, image:) any \ volume_exists(volume), negate(volume_exists(legacy)), - [ "(", *combine(docker(:volume, :create, volume), copy_between_volumes(legacy, volume, image: image)), ")" ] + group(docker(:volume, :create, volume), copy_between_volumes(legacy, volume, image: image)) end def negate(command) [ "!", *command ] end + # The docker builders (network create, the stage-3c network bridge) for callers that + # compose them into a command of their own rather than executing them on their own. + def docker_commands + @docker_commands ||= Dash::Commands::Docker.new(config) + end + def volume_exists(name) docker :volume, :inspect, name, ">", "/dev/null", "2>&1" end diff --git a/lib/dash/commands/loadbalancer.rb b/lib/dash/commands/loadbalancer.rb index 84f9da5f..17cab051 100644 --- a/lib/dash/commands/loadbalancer.rb +++ b/lib/dash/commands/loadbalancer.rb @@ -47,6 +47,28 @@ def copy_legacy_config_volume copy_legacy_volume(legacy: legacy_config_volume_name, volume: config_volume_name, image: loadbalancer_config.run.image) end + # Everything this host needs before anything reads its container, volume or network, + # in the one round trip it already pays for the apps-config directory. Same shape as + # Dash::Commands::Proxy#prepare_boot, including why the guard has to be the first word. + def prepare_boot + combine legacy_rename, ensure_apps_config_directory + end + + # The loadbalancer's half of the stage-3c bridge - network, then volume - skipped + # outright by a host that has already been through it. No legacy container is replaced + # here (a dedicated loadbalancer host never ran one under a name this gem knows), so + # the marker is verified on the volume instead: the new one exists, or there was never + # a legacy one to adopt. Stage 3d deletes this with the rest of the bridge. + def legacy_rename + any \ + [ :test, "-f", legacy_rename_marker ], + group( + group(docker_commands.connect_legacy_network_containers), + group(copy_legacy_config_volume), + group(any(mark_legacy_renamed, [ :true ])) + ) + end + def deploy(targets: []) docker :exec, container_name, "dash-proxy", "deploy", loadbalancer_config.config.service, *loadbalancer_config.deploy_command_args(targets: targets) @@ -76,6 +98,12 @@ def config_digest docker :inspect, container_name, "--format", Dash::Commands::Proxy::CONFIG_DIGEST_FORMAT end + # One read for container id, image tag and config digest - parsed by + # Dash::Commands::Proxy::State, same as the per-host proxy's. + def inspect_state + docker :inspect, container_name, "--format", Dash::Commands::Proxy::STATE_FORMAT + end + def container_id(only_running: false) container_id_for(container_name: container_name, only_running: only_running) end @@ -163,6 +191,18 @@ def container_name end private + # Stage 3c. 3d deletes both of these with the rest of the bridge. + def legacy_rename_marker + File.join loadbalancer_config.directory, Dash::Configuration::Proxy::LEGACY_RENAME_MARKER + end + + def mark_legacy_renamed + combine \ + group(any(volume_exists(config_volume_name), negate(volume_exists(legacy_config_volume_name)))), + make_directory(loadbalancer_config.directory), + [ :touch, legacy_rename_marker ] + end + def run_args loadbalancer_config.run_args end diff --git a/lib/dash/commands/proxy.rb b/lib/dash/commands/proxy.rb index 21877851..d60ceb47 100644 --- a/lib/dash/commands/proxy.rb +++ b/lib/dash/commands/proxy.rb @@ -12,8 +12,13 @@ class Dash::Commands::Proxy < Dash::Commands::Base # Both the legacy constant and the fallback go away in stage 3d. LEGACY_CONFIG_DIGEST_LABEL = "org.kamal.proxy-config-digest" - CONFIG_DIGEST_FORMAT = "'{{ with index .Config.Labels \"#{CONFIG_DIGEST_LABEL}\" }}{{ . }}" \ - "{{ else }}{{ index .Config.Labels \"#{LEGACY_CONFIG_DIGEST_LABEL}\" }}{{ end }}'" + CONFIG_DIGEST_TEMPLATE = "{{ with index .Config.Labels \"#{CONFIG_DIGEST_LABEL}\" }}{{ . }}" \ + "{{ else }}{{ index .Config.Labels \"#{LEGACY_CONFIG_DIGEST_LABEL}\" }}{{ end }}" + + CONFIG_DIGEST_FORMAT = "'#{CONFIG_DIGEST_TEMPLATE}'" + + # Everything Dash::Cli::Proxy::Drift and the minimum-version gate need, in one format. + STATE_FORMAT = "'{{.Id}} {{.Config.Image}} #{CONFIG_DIGEST_TEMPLATE}'" def initialize(config, host:) super(config) @@ -42,6 +47,41 @@ def run(digest: nil, name: nil) # destination not existing, so a second deploy is a no-op. Stage 3d deletes # them along with the legacy constants they read. + # Everything a proxy host needs before anything reads its container, volume or + # network, in the one round trip it already pays for the apps-config directory. + # + # `a || b && c` is `(a || b) && c`, so the mkdir runs whichever way the guard went - + # and the guard has to be the first word rather than a parenthesised group, because + # SSHKit prefixes the first word with /usr/bin/env and passes only `test` through. + # Stage 3d drops the legacy_rename half and leaves the mkdir. + def prepare_boot + combine legacy_rename, ensure_apps_config_directory + end + + # The whole stage-3c bridge as one command, skipped outright by a host that has + # already been through it - or was installed fresh on 4.x and never had a kamal-proxy. + # The three steps keep their own bodies and their documented order (see + # Dash::Cli::Proxy::LegacyRename); each is wrapped in its own subshell because they + # all mix && and || at one precedence level, and composing them flat would + # re-associate across the volume copy's guard - the chain 4.0.0 got wrong. + # + # The marker is written on verified absence of both legacy containers, never on the + # chain's exit status: the two removals end in `|| true`, so a host whose stop failed + # would otherwise record itself as migrated and never retry. Its own `|| true` keeps + # that failure as quiet as it is today, while a failed volume copy still exits + # non-zero through the && chain and aborts the boot exactly as it does now. + def legacy_rename + any \ + [ :test, "-f", legacy_rename_marker ], + group( + group(docker_commands.connect_legacy_network_containers), + group(copy_legacy_config_volume), + group(remove_legacy_container(timeout: config.drain_timeout)), + group(remove_legacy_holder_container), + group(any(mark_legacy_renamed, [ :true ])) + ) + end + # Copies the pre-rename config volume into the new one, before anything # starts. The volume holds the routing table and the ACME account and # certificate cache; losing it means re-issuing every certificate and @@ -116,6 +156,13 @@ def config_digest docker :inspect, container_name, "--format", CONFIG_DIGEST_FORMAT end + # One read for container id, image tag and config digest - parsed by + # Dash::Commands::Proxy::State. Capture it with raise_on_non_zero_exit: false; + # a host with no proxy container inspects to nothing, which is an answer. + def inspect_state + docker :inspect, container_name, "--format", STATE_FORMAT + end + def container_id(only_running: false) container_id_for(container_name: container_name, only_running: only_running) end @@ -327,6 +374,19 @@ def container_exists(name) docker :container, :inspect, name, ">", "/dev/null", "2>&1" end + # Stage 3c. 3d deletes both of these with the rest of the bridge. + def legacy_rename_marker + File.join config.proxy_boot.host_directory, Dash::Configuration::Proxy::LEGACY_RENAME_MARKER + end + + def mark_legacy_renamed + combine \ + negate(container_exists(Dash::Configuration::Proxy::LEGACY_CONTAINER_NAME)), + negate(container_exists(Dash::Configuration::Proxy::LEGACY_HOLDER_CONTAINER_NAME)), + make_directory(config.proxy_boot.host_directory), + [ :touch, legacy_rename_marker ] + end + # The image the volume copy borrows. The proxy this gem is pinned to is # already pulled by the time the copy runs, and `rake release` gates on # MINIMUM_VERSION being published, so this is always resolvable — unlike diff --git a/lib/dash/commands/proxy/state.rb b/lib/dash/commands/proxy/state.rb new file mode 100644 index 00000000..3d2e6c94 --- /dev/null +++ b/lib/dash/commands/proxy/state.rb @@ -0,0 +1,31 @@ +# What one `docker inspect` of a proxy container tells a boot: whether it exists, which +# image tag it runs, and the config digest it was booted with. Three questions that used +# to cost three round trips each (`container_id`, `version`, `config_digest`). +# +# Produced by Dash::Commands::Proxy#inspect_state and its loadbalancer twin, both of which +# are captured with raise_on_non_zero_exit: false - a host with no container inspects to +# empty output, which parses to a state that simply does not exist. +class Dash::Commands::Proxy::State + attr_reader :id, :image, :digest + + def self.parse(output) + id, image, digest = output.to_s.strip.split(" ", 3) + new(id: id, image: image, digest: digest) + end + + def initialize(id: nil, image: nil, digest: nil) + @id = id.presence + @image = image.presence + @digest = digest.presence + end + + def exists? + id.present? + end + + # The tag, read the way Dash::Commands::Proxy#version reads it - everything past the + # LAST colon, so a registry host carrying a port does not get mistaken for the version. + def version + image&.split(":")&.last + end +end diff --git a/lib/dash/configuration/proxy.rb b/lib/dash/configuration/proxy.rb index 14c5d6af..c04ff908 100644 --- a/lib/dash/configuration/proxy.rb +++ b/lib/dash/configuration/proxy.rb @@ -23,6 +23,9 @@ class Dash::Configuration::Proxy LEGACY_LOADBALANCER_CONTAINER_NAME = "kamal-loadbalancer" LEGACY_HOLDER_CONTAINER_NAME = "kamal-proxy-net" LEGACY_NETWORK = "kamal" + # Written into the run directory once a host is verifiably past the 3c rename, so the + # bridge costs it nothing but a `test -f` on every deploy after. Deleted in stage 3d. + LEGACY_RENAME_MARKER = ".legacy-renamed" LEGACY_CONFIG_VOLUME = "kamal-proxy-config" LEGACY_LOADBALANCER_CONFIG_VOLUME = "kamal-loadbalancer-config" LEGACY_IMAGE_TITLE = "kamal-proxy" diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index f23cb15d..2ad07cf9 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -88,6 +88,54 @@ class CliProxyTest < CliTestCase end end + # zoolutions/dash#160. `Ensure dash-proxy` is the most expensive row of a deploy by + # round trips, and on a host that has already been through the stage-3c rename most of + # what it did was checking, again, that it had. Every round trip the boot spends on a + # proxy host is pinned here so a regression cannot land unnoticed - and so a deliberate + # reduction has to be explained in the commit that edits this list. + # + # On main this host paid eleven, plus the shared `docker network create`: the login, + # four separate bridge commands, the secrets file, the apps-config mkdir, start_or_run, + # and four captures (container id, config digest, boot config, image tag). The bridge is + # now one command the host skips inside, carried by the apps-config round trip it pays + # anyway, and the three reads about the running container are one inspect. + PROXY_BOOT_ROUND_TRIPS_PER_HOST = [ + "docker login -u \"user\" -p \"pw\"", + "test -f .dash/proxy/.legacy-renamed || ( ... ) && mkdir -p .dash/proxy/apps-config", + "docker inspect dash-proxy --format '{{.Id}} {{.Config.Image}} ...'", + "echo $(cat .dash/proxy/options ...)", + "rm .dash/proxy/secrets.env", + "docker container start dash-proxy || echo $(cat .dash/proxy/options ...) | xargs docker run ..." + ].freeze + + test "boot issues no round trip beyond the pinned per-host sequence" do + SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("") + stub_proxy_state "dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION} " \ + "#{Dash::Configuration::Proxy::Run.digest("")}" + + round_trips = recorded_proxy_round_trips { run_command("boot", fixture: :simple) } + + assert_equal [ "docker network create dash" ] * 2 + PROXY_BOOT_ROUND_TRIPS_PER_HOST * 2, round_trips, + "deploy_simple has two proxy hosts: the network sweep, then the pinned sequence twice" + end + + # Nothing changes for a host still running kamal-proxy: the three bridge steps travel + # in the same command now, in the same documented order, behind the marker guard. + test "boot sends a still-legacy host the whole bridge, in its documented order" do + stub_no_proxy_drift + + run_command("boot").tap do |output| + guard = output.index("test -f .dash/proxy/.legacy-renamed || (") + network = output.index("( docker network inspect kamal > /dev/null 2>&1") + volume = output.index("( docker volume inspect dash-proxy-config > /dev/null 2>&1") + container = output.index("( docker container inspect kamal-proxy > /dev/null 2>&1 && docker container stop") + + assert guard && network && volume && container, output + assert guard < network, "the marker decides before any docker call: #{output}" + assert network < volume && volume < container, "the bridge keeps its documented order: #{output}" + end + end + test "boot takes the server lock so concurrent destinations serialise on the shared proxy" do run_command("boot").tap do |output| assert_match "Acquiring the server lock...", output @@ -320,9 +368,6 @@ class CliProxyTest < CliTestCase test "boot with port_holder ensures the holder before starting the proxy" do stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format '{{.Config.Image}}'", "|", :awk, "-F:", "'{print $NF}'") - .returns(Dash::Configuration::Proxy::Run::MINIMUM_VERSION) run_command("boot", fixture: :with_proxy_port_holder).tap do |output| assert_match "docker container start dash-proxy-net || docker run --name dash-proxy-net --network dash --detach --restart unless-stopped --publish 80:80 --publish 443:443 --log-opt max-size=10m ghcr.io/zoolutions/dash-proxy:#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION} dash-proxy hold on 1.1.1.1", output @@ -332,12 +377,8 @@ class CliProxyTest < CliTestCase test "boot with matching digest does not reboot" do SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("") - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :container, :ls, "--all", "--filter", "'name=^dash-proxy$'", "--quiet", raise_on_non_zero_exit: false) - .returns("abc123") - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format", Dash::Commands::Proxy::CONFIG_DIGEST_FORMAT, raise_on_non_zero_exit: false) - .returns(Dash::Configuration::Proxy::Run.digest("")) + stub_proxy_state "dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION} " \ + "#{Dash::Configuration::Proxy::Run.digest("")}" run_command("boot").tap do |output| assert_match "docker container start dash-proxy ||", output @@ -355,10 +396,7 @@ class CliProxyTest < CliTestCase test "boot old version" do Thread.report_on_exception = false stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format '{{.Config.Image}}'", "|", :awk, "-F:", "'{print $NF}'") - .returns("v0.0.1") - .at_least_once + stub_proxy_state "dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:v0.0.1 #{Dash::Configuration::Proxy::Run.digest("")}" exception = assert_raises do run_command("boot").tap do |output| @@ -375,10 +413,8 @@ class CliProxyTest < CliTestCase test "boot correct version" do Thread.report_on_exception = false stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format '{{.Config.Image}}'", "|", :awk, "-F:", "'{print $NF}'") - .returns(Dash::Configuration::Proxy::Run::MINIMUM_VERSION) - .at_least_once + stub_proxy_state "dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:" \ + "#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION} #{Dash::Configuration::Proxy::Run.digest("")}" run_command("boot").tap do |output| assert_match "docker login", output @@ -508,10 +544,6 @@ class CliProxyTest < CliTestCase .returns("12345678\n#{Dash::Commands::App::BOOT_STATE_SEPARATOR}\n12345678") stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format '{{.Config.Image}}'", "|", :awk, "-F:", "'{print $NF}'") - .returns(Dash::Configuration::Proxy::Run::MINIMUM_VERSION) - stub_readiness_wait "no-healthcheck:running", expect: true stub_run_capture id: "12345678" # the proxy target, printed by the run itself @@ -522,11 +554,9 @@ class CliProxyTest < CliTestCase assert_match "docker container stop dash-proxy", output assert_match "docker container prune --force --filter label=org.opencontainers.image.title=dash-proxy", output assert_match "docker image prune --all --force --filter label=org.opencontainers.image.title=dash-proxy", output - assert_match "/usr/bin/env mkdir -p .dash", output assert_match "docker network create dash", output assert_match "docker login -u [REDACTED] -p [REDACTED]", output assert_match "docker container start dash-proxy || echo $(cat .dash/proxy/options 2> /dev/null || echo \"--publish 80:80 --publish 443:443 --log-opt max-size=10m\") $(cat .dash/proxy/image 2> /dev/null || echo \"ghcr.io/zoolutions/dash-proxy\"):$(cat .dash/proxy/image_version 2> /dev/null || echo \"#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION}\") $(cat .dash/proxy/run_command 2> /dev/null || echo \"\") | xargs docker run --name dash-proxy --network dash --detach --restart unless-stopped --volume dash-proxy-config:/home/dash-proxy/.config/dash-proxy", output - assert_match "/usr/bin/env mkdir -p .dash", output assert_match %r{docker rename app-web-latest app-web-latest_replaced_.*}, output assert_match "Booted app version latest\" >> .dash/app-audit.log && mkdir -p .dash/apps/app/env/roles", output assert_match "Uploading \"\\n\" to .dash/apps/app/env/roles/web.env", output @@ -534,7 +564,7 @@ class CliProxyTest < CliTestCase assert_match "docker exec dash-proxy dash-proxy deploy app-web --target=\"12345678:80\" --deploy-timeout=\"6s\" --drain-timeout=\"30s\" --buffer-requests --buffer-responses --log-request-header=\"Cache-Control\" --log-request-header=\"Last-Modified\" --log-request-header=\"User-Agent\"", output 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 .dash", output + assert_match ") && mkdir -p .dash/proxy/apps-config", 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 @@ -547,10 +577,6 @@ class CliProxyTest < CliTestCase SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("12345678") stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.expects(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format '{{.Config.Image}}'", "|", :awk, "-F:", "'{print $NF}'") - .returns(Dash::Configuration::Proxy::Run::MINIMUM_VERSION) - stub_readiness_wait "no-healthcheck:running", expect: true stub_run_capture id: "12345678" # the proxy target, printed by the run itself @@ -1490,11 +1516,9 @@ def stub_loadbalancer_registry(service_owner: "", run_config: "") .returns(run_config) end - # Allow the drift-detection captures without triggering a reboot. + # Allow the drift-detection capture without triggering a reboot. def stub_no_proxy_drift - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :container, :ls, "--all", "--filter", "'name=^dash-proxy$'", "--quiet", raise_on_non_zero_exit: false) - .returns("") + stub_proxy_state("dash-proxy", "") SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) .with { |*args| args.first == :echo } .returns("") @@ -1503,22 +1527,50 @@ def stub_no_proxy_drift # Simulate an existing proxy container running with a stale config digest. def stub_loadbalancer_drift SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("") - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :container, :ls, "--all", "--filter", "'name=^load-balancer$'", "--quiet", raise_on_non_zero_exit: false) - .returns("abc123") - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :inspect, "load-balancer", "--format", Dash::Commands::Proxy::CONFIG_DIGEST_FORMAT, raise_on_non_zero_exit: false) - .returns("stale-digest") + stub_proxy_state("load-balancer", "abc123 ghcr.io/zoolutions/dash-proxy:#{Dash::Configuration::Proxy::Run::MINIMUM_VERSION} stale-digest") end - def stub_proxy_drift + def stub_proxy_drift(version: Dash::Configuration::Proxy::Run::MINIMUM_VERSION) SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info).returns("") + stub_proxy_state("dash-proxy", "abc123 ghcr.io/zoolutions/dash-proxy:#{version} stale-digest") + end + + # Every round trip a proxy boot spends, executes and captures interleaved in issue + # order, with the deploy's own run-directory and lock commands filtered out and the + # long ones elided - the count and the order are what this pins, not the shell. + def recorded_proxy_round_trips + round_trips = [] + SSHKit::Backend::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << cmd.to_command; true } SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :container, :ls, "--all", "--filter", "'name=^dash-proxy$'", "--quiet", raise_on_non_zero_exit: false) - .returns("abc123") + .with { |*args| round_trips << args.reject { |arg| arg.is_a?(Hash) }.join(" "); false } + + begin + yield + ensure + SSHKit::Backend::Printer.any_instance.unstub(:execute_command) + end + + round_trips.reject { |command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) }.map { |command| elide(command) } + end + + # Collapses the parts that carry a digest, a boot-config read or the whole stage-3c + # bridge - asserted in full elsewhere, and unreadable in a sequence. + def elide(command) + command + .sub(/(test -f \.dash\/proxy\/\.legacy-renamed \|\| \().*(\) && mkdir)/, '\\1 ... \\2') + .sub(/(docker inspect dash-proxy --format '\{\{\.Id\}\} \{\{\.Config\.Image\}\}).*'/, "\\1 ...'") + .sub(/\Aecho \$\(cat \.dash\/proxy\/options .*/, "echo $(cat .dash/proxy/options ...)") + .sub(/(docker container start dash-proxy \|\| echo \$\(cat \.dash\/proxy\/options ).*(\| xargs docker run ).*/, + "docker container start dash-proxy || echo $(cat .dash/proxy/options ...) | xargs docker run ...") + .sub(/\A\/usr\/bin\/env /, "") + end + + # The one inspect a boot makes about the running container: id, image tag and config + # digest in one line (Dash::Commands::Proxy::State). + def stub_proxy_state(container_name, output) SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with(:docker, :inspect, "dash-proxy", "--format", Dash::Commands::Proxy::CONFIG_DIGEST_FORMAT, raise_on_non_zero_exit: false) - .returns("stale-digest") + .with(:docker, :inspect, container_name, "--format", Dash::Commands::Proxy::STATE_FORMAT, raise_on_non_zero_exit: false) + .returns(output) end # Swallow the deploy-lock release; the server-lock hosts get their own diff --git a/test/commands/loadbalancer_test.rb b/test/commands/loadbalancer_test.rb index ef4d3b2b..a66d6907 100644 --- a/test/commands/loadbalancer_test.rb +++ b/test/commands/loadbalancer_test.rb @@ -335,6 +335,50 @@ class CommandsLoadbalancerTest < ActiveSupport::TestCase assert_match "--volume kamal-proxy-config:/from --volume dash-proxy-config:/to", command end + # --- Stage 3c: the bridge, folded into one guarded round trip (zoolutions/dash#160) --- + + test "legacy_rename skips the bridge behind a marker in the loadbalancer directory" do + command = new_command.legacy_rename.join(" ") + + assert command.start_with?("test -f .dash/loadbalancer/.legacy-renamed || ( "), command + assert command.end_with?(")"), command + end + + test "legacy_rename bridges the network before adopting the volume" do + command = new_command.legacy_rename.join(" ") + + bridge = command.index("docker network inspect kamal > /dev/null 2>&1") + copy = command.index("docker volume inspect dash-loadbalancer-config > /dev/null 2>&1") + + assert bridge, command + assert copy && bridge < copy, "the network bridge has to land before the volume copy: #{command}" + end + + # The loadbalancer branch replaces no legacy container, so the volume is the only + # thing its bridge can verify: the new one exists, or there was never a legacy one. + test "legacy_rename writes the marker only once the volume is settled" do + command = new_command.legacy_rename.join(" ") + + assert_match "( ( docker volume inspect dash-loadbalancer-config > /dev/null 2>&1 " \ + "|| ! docker volume inspect kamal-loadbalancer-config > /dev/null 2>&1 ) " \ + "&& mkdir -p .dash/loadbalancer && touch .dash/loadbalancer/.legacy-renamed || true )", command + end + + test "prepare_boot carries the apps-config directory in the bridge round trip" do + command = new_command.prepare_boot.join(" ") + + assert command.start_with?("test -f .dash/loadbalancer/.legacy-renamed ||"), command + assert command.end_with?(") && mkdir -p .dash/proxy/apps-config"), command + end + + test "inspect_state reads id, image and config digest in one format" do + assert_equal \ + "docker inspect load-balancer --format '{{.Id}} {{.Config.Image}} " \ + "{{ with index .Config.Labels \"org.dash.proxy-config-digest\" }}{{ . }}" \ + "{{ else }}{{ index .Config.Labels \"org.kamal.proxy-config-digest\" }}{{ end }}'", + new_command.inspect_state.join(" ") + end + test "remove_container prunes both the current and the legacy title" do assert_equal \ "docker container prune --force --filter label=org.opencontainers.image.title=dash-loadbalancer && " \ diff --git a/test/commands/proxy_test.rb b/test/commands/proxy_test.rb index cdba7232..45075e99 100644 --- a/test/commands/proxy_test.rb +++ b/test/commands/proxy_test.rb @@ -538,4 +538,112 @@ def new_command assert_match "docker container rm --force kamal-proxy-net", command assert command.end_with?("|| true") end + + # --- Stage 3c: the bridge, folded into one guarded round trip (zoolutions/dash#160) --- + + test "legacy_rename skips the whole bridge behind a marker in the run directory" do + command = new_command.legacy_rename.join(" ") + + assert command.start_with?("test -f .dash/proxy/.legacy-renamed || ( "), + "a migrated host must decide with the marker, before any docker call: #{command}" + assert command.end_with?(")"), command + end + + test "legacy_rename runs the three bridge steps in the documented order" do + command = new_command.legacy_rename.join(" ") + + bridge = command.index("docker network inspect kamal > /dev/null 2>&1") + copy = command.index("docker volume inspect dash-proxy-config > /dev/null 2>&1") + replace = command.index("docker container inspect kamal-proxy > /dev/null 2>&1 && docker container stop") + holder = command.index("docker container inspect kamal-proxy-net") + + assert bridge, command + assert copy && bridge < copy, "the network bridge has to land before the volume copy: #{command}" + assert replace && copy < replace, "the volume copy has to land before the container is replaced: #{command}" + assert holder && replace < holder, command + end + + # Each builder already mixes && and || at one precedence level, so composing them + # flat would re-associate across their internals - and the volume copy's guard is + # exactly the chain 4.0.0 lost every host's routing table by getting wrong. + test "legacy_rename isolates every composed step in its own subshell" do + command = new_command.legacy_rename.join(" ") + + assert_match "( docker network inspect kamal > /dev/null 2>&1", command + assert_match "&& ( docker volume inspect dash-proxy-config", command + assert_match "&& ( docker container inspect kamal-proxy > /dev/null 2>&1", command + assert_match "&& ( docker container inspect kamal-proxy-net", command + end + + # The two removals end in `|| true`, so the chain's exit status says nothing about + # whether the legacy container is actually gone. Ask docker instead. + test "legacy_rename writes the marker only once both legacy containers are gone" do + command = new_command.legacy_rename.join(" ") + + assert_match "( ! docker container inspect kamal-proxy > /dev/null 2>&1 " \ + "&& ! docker container inspect kamal-proxy-net > /dev/null 2>&1 " \ + "&& mkdir -p .dash/proxy && touch .dash/proxy/.legacy-renamed || true )", command + end + + # A failed volume copy must still abort the boot: it is the one step whose failure + # is not masked today, and recording it as migrated would strand the routing table. + test "legacy_rename leaves the volume copy free to fail the boot" do + copy = new_command.legacy_rename.join(" ")[/\( docker volume inspect dash-proxy-config.*?'cp -a \/from\/\. \/to\/' \) \)/m] + + assert copy, new_command.legacy_rename.join(" ") + refute_match "|| true", copy, "a failed copy must fail the deploy, not vanish into || true" + end + + # Step 3: the apps-config mkdir is a round trip the host pays anyway, and it reads + # nothing the bridge writes - so a migrated host pays zero round trips for the bridge. + test "prepare_boot carries the apps-config directory in the bridge round trip" do + command = new_command.prepare_boot.join(" ") + + assert command.start_with?("test -f .dash/proxy/.legacy-renamed ||"), command + assert command.end_with?(") && mkdir -p .dash/proxy/apps-config"), + "`a || b && c` is `(a || b) && c`, so the mkdir runs either way: #{command}" + end + + # One inspect for what used to be container_id, config_digest and version. + test "inspect_state reads id, image and config digest in one format" do + assert_equal \ + "docker inspect dash-proxy --format '{{.Id}} {{.Config.Image}} " \ + "{{ with index .Config.Labels \"org.dash.proxy-config-digest\" }}{{ . }}" \ + "{{ else }}{{ index .Config.Labels \"org.kamal.proxy-config-digest\" }}{{ end }}'", + new_command.inspect_state.join(" ") + end + + test "State parses the three fields and derives the version from the image tag" do + state = Dash::Commands::Proxy::State.parse("abc123 ghcr.io/zoolutions/dash-proxy:v1.2.3 deadbeef\n") + + assert state.exists? + assert_equal "abc123", state.id + assert_equal "v1.2.3", state.version + assert_equal "deadbeef", state.digest + end + + # A registry host carries a port, so the tag is what follows the LAST colon - + # the same rule `version`'s `awk -F: '{print $NF}'` applies. + test "State reads the tag past a registry port" do + state = Dash::Commands::Proxy::State.parse("abc123 registry:4443/ghcr.io/zoolutions/dash-proxy:v1.2.3 deadbeef") + + assert_equal "v1.2.3", state.version + end + + test "State of a host with no proxy container is empty, not an error" do + state = Dash::Commands::Proxy::State.parse("") + + assert_not state.exists? + assert_nil state.version + assert_nil state.digest + end + + # A container booted before the digest label existed inspects to a trailing blank + # field - it has to read as "no digest", which is drift, not as a missing container. + test "State of an unlabelled container exists but carries no digest" do + state = Dash::Commands::Proxy::State.parse("abc123 ghcr.io/zoolutions/dash-proxy:v1.2.3 ") + + assert state.exists? + assert_nil state.digest + end end From 1014e5a4f645cbc9f0ded342735ca787d48cb80d Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sat, 12 Sep 2026 19:17:18 +0200 Subject: [PATCH 2/2] fix: address PR review feedback - Marker check fails closed on a docker error: `mark_legacy_renamed` used a negated `docker container inspect`, whose exit code cannot distinguish "no such container" from "the daemon couldn't be asked." A transient docker error read as confirmed absence and would permanently mark a host migrated with the legacy container still running. Replaced with `confirmed_empty?`, a new Dash::Commands::Base helper built on the existing list-based `container_id_for` (which exits 0 on no match and non-zero only on a genuine failure): `result=$(list) && [ -z "$result" ]` propagates the list's own exit status, verified against real sh and bash. - Documented, and opened zoolutions/dash#168 for, the loadbalancer's narrower risk: its marker is verified on volume existence (no legacy container to check there), which a `dash proxy reboot` run before the bridge has ever executed on a host could satisfy without the legacy volume ever being copied. Root cause is LoadbalancerReboot#run (and the per-host Reboot) never routing through the bridge - pre-existing on `main`, and a fix belongs in a PR that touches those classes, not this one (scoped to `boot`'s round trips per #160). - Extracted `recorded_commands_and_captures` into CliTestCase, shared by the Printer/capture_with_info stubbing `recorded_commands` and `recorded_captures` already do separately; `recorded_proxy_round_trips` now composes it and keeps only the proxy-specific redaction. Addresses cubic-dev-ai review on PR #167. --- lib/dash/commands/base.rb | 12 ++++++++++++ lib/dash/commands/loadbalancer.rb | 16 ++++++++++++++++ lib/dash/commands/proxy.rb | 6 ++++-- test/cli/cli_test_case.rb | 22 ++++++++++++++++++++++ test/cli/proxy_test.rb | 15 +++------------ test/commands/proxy_test.rb | 14 ++++++++++++-- 6 files changed, 69 insertions(+), 16 deletions(-) diff --git a/lib/dash/commands/base.rb b/lib/dash/commands/base.rb index 9075fd05..0e1f2117 100644 --- a/lib/dash/commands/base.rb +++ b/lib/dash/commands/base.rb @@ -40,6 +40,18 @@ def container_id_for(container_name:, only_running: false) docker :container, :ls, *("--all" unless only_running), "--filter", "'name=^#{container_name}$'", "--quiet" end + # True only when `list_command`'s own output is confirmed empty - never inferred from + # a failure. `docker container inspect name > /dev/null 2>&1` (negated) cannot tell + # "no such container" from "the daemon could not be asked" - both exit non-zero - so a + # transient failure there reads as confirmed absence. A `list` exits 0 whichever way + # the match went and non-zero only on a genuine failure, so `result=$(list) && [ -z + # "$result" ]` fails closed: `result=$(list)` carries list's own exit status (POSIX; + # verified against sh and bash), so a failed list stops the chain before the test runs. + # `list_command` must be a listing (docker container/volume ls), never an inspect. + def confirmed_empty?(list_command) + [ "result=$(#{list_command.join(" ")})", "&&", "[", "-z", "\"$result\"", "]" ] + end + def make_directory_for(remote_file) make_directory Pathname.new(remote_file).dirname.to_s end diff --git a/lib/dash/commands/loadbalancer.rb b/lib/dash/commands/loadbalancer.rb index 17cab051..2c592d83 100644 --- a/lib/dash/commands/loadbalancer.rb +++ b/lib/dash/commands/loadbalancer.rb @@ -196,6 +196,22 @@ def legacy_rename_marker File.join loadbalancer_config.directory, Dash::Configuration::Proxy::LEGACY_RENAME_MARKER end + # Verified on the volume existing, not on a container being gone - the loadbalancer + # replaces no legacy container, so this is the only signal its bridge has. That makes + # it foolable in one specific way: if `dash-loadbalancer-config` comes to exist before + # this bridge ever runs on a host - e.g. `dash proxy reboot` invoked directly against a + # dedicated LB host that has never been through `dash proxy boot`, since + # Dash::Cli::Proxy::LoadbalancerReboot#run boots the container without calling this + # bridge first - the marker is written despite the legacy volume's routing table and + # ACME cache never having been copied. That gap predates this fold: on `main`, + # `copy_legacy_config_volume`'s own guard (`volume_exists(new) || ...`) already skips + # the copy for good in that state, silently, every deploy - this only turns a + # per-deploy re-check into a cached one. Closing it means routing `reboot` through the + # same bridge, a `Dash::Cli::Proxy::LoadbalancerReboot` change, out of scope here + # (zoolutions/dash#160 is `boot`'s round trips) - tracked in zoolutions/dash#168. + # Recovery today is the same either way: copy the legacy volume's contents over by + # hand, then remove .legacy-renamed under this host's loadbalancer directory so this + # re-evaluates. def mark_legacy_renamed combine \ group(any(volume_exists(config_volume_name), negate(volume_exists(legacy_config_volume_name)))), diff --git a/lib/dash/commands/proxy.rb b/lib/dash/commands/proxy.rb index d60ceb47..e555876c 100644 --- a/lib/dash/commands/proxy.rb +++ b/lib/dash/commands/proxy.rb @@ -379,10 +379,12 @@ def legacy_rename_marker File.join config.proxy_boot.host_directory, Dash::Configuration::Proxy::LEGACY_RENAME_MARKER end + # confirmed_empty?, not a negated inspect: a docker error while checking must never + # read as "confirmed gone" (zoolutions/dash#167 review). def mark_legacy_renamed combine \ - negate(container_exists(Dash::Configuration::Proxy::LEGACY_CONTAINER_NAME)), - negate(container_exists(Dash::Configuration::Proxy::LEGACY_HOLDER_CONTAINER_NAME)), + confirmed_empty?(container_id_for(container_name: Dash::Configuration::Proxy::LEGACY_CONTAINER_NAME)), + confirmed_empty?(container_id_for(container_name: Dash::Configuration::Proxy::LEGACY_HOLDER_CONTAINER_NAME)), make_directory(config.proxy_boot.host_directory), [ :touch, legacy_rename_marker ] end diff --git a/test/cli/cli_test_case.rb b/test/cli/cli_test_case.rb index acdd55f0..253d4de5 100644 --- a/test/cli/cli_test_case.rb +++ b/test/cli/cli_test_case.rb @@ -59,6 +59,28 @@ def recorded_captures captures end + # #recorded_commands and #recorded_captures each answer one half of what a host was + # asked to do; a caller that needs both interleaved in the order they actually ran - + # a round-trip count, where an execute and a capture cost the same SSH round trip - + # cannot get that by nesting the two, since each keeps its own array. This shares one + # array between both stubs instead. Formats captures without their trailing options + # hash (`raise_on_non_zero_exit: false` and friends), which reads as noise next to a + # shell command; #recorded_captures keeps the hash for its own callers. + def recorded_commands_and_captures + round_trips = [] + SSHKit::Backend::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << cmd.to_command; true } + SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) + .with { |*args| round_trips << args.reject { |arg| arg.is_a?(Hash) }.join(" "); false } + + begin + yield + ensure + SSHKit::Backend::Printer.any_instance.unstub(:execute_command) + end + + round_trips + end + # The id `docker run --detach` prints, which a boot reads instead of asking docker for # the container id in a round trip of its own. def stub_run_capture(id: "123") diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index 2ad07cf9..ab544531 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -1539,18 +1539,9 @@ def stub_proxy_drift(version: Dash::Configuration::Proxy::Run::MINIMUM_VERSION) # order, with the deploy's own run-directory and lock commands filtered out and the # long ones elided - the count and the order are what this pins, not the shell. def recorded_proxy_round_trips - round_trips = [] - SSHKit::Backend::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << cmd.to_command; true } - SSHKit::Backend::Abstract.any_instance.stubs(:capture_with_info) - .with { |*args| round_trips << args.reject { |arg| arg.is_a?(Hash) }.join(" "); false } - - begin - yield - ensure - SSHKit::Backend::Printer.any_instance.unstub(:execute_command) - end - - round_trips.reject { |command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) }.map { |command| elide(command) } + recorded_commands_and_captures { yield } + .reject { |command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) } + .map { |command| elide(command) } end # Collapses the parts that carry a digest, a boot-config read or the whole stage-3c diff --git a/test/commands/proxy_test.rb b/test/commands/proxy_test.rb index 45075e99..a1a969e0 100644 --- a/test/commands/proxy_test.rb +++ b/test/commands/proxy_test.rb @@ -580,11 +580,21 @@ def new_command test "legacy_rename writes the marker only once both legacy containers are gone" do command = new_command.legacy_rename.join(" ") - assert_match "( ! docker container inspect kamal-proxy > /dev/null 2>&1 " \ - "&& ! docker container inspect kamal-proxy-net > /dev/null 2>&1 " \ + assert_match "( result=$(docker container ls --all --filter 'name=^kamal-proxy$' --quiet) && [ -z \"$result\" ] " \ + "&& result=$(docker container ls --all --filter 'name=^kamal-proxy-net$' --quiet) && [ -z \"$result\" ] " \ "&& mkdir -p .dash/proxy && touch .dash/proxy/.legacy-renamed || true )", command end + # A docker error while checking (daemon busy, permission denied) is not proof the + # container is gone - unlike a negated `inspect`, which cannot tell the two apart. + test "legacy_rename's marker check fails closed on a docker error, not just a miss" do + command = new_command.legacy_rename.join(" ") + + assert_match "result=$(docker container ls --all --filter 'name=^kamal-proxy$' --quiet) && [ -z \"$result\" ]", command + refute_match "! docker container inspect", command, + "a negated inspect can't tell \"not found\" from \"the daemon couldn't be asked\": #{command}" + end + # A failed volume copy must still abort the boot: it is the one step whose failure # is not masked today, and recording it as migrated would strand the routing table. test "legacy_rename leaves the volume copy free to fail the boot" do