diff --git a/lib/dash/cli/proxy.rb b/lib/dash/cli/proxy.rb index 8c7c2c95..242af63c 100644 --- a/lib/dash/cli/proxy.rb +++ b/lib/dash/cli/proxy.rb @@ -414,6 +414,12 @@ def loadbalancer(status) if DASH.config.proxy.load_balancing? on(DASH.config.proxy.effective_loadbalancer) do |host| execute *DASH.registry.login + # start_or_run falls through to `docker run` on a host with no + # container, so this is a volume-creating path too and gets the same + # bridge boot and reboot do (zoolutions/dash#168). It also makes the + # apps-config directory the bind mount would otherwise have docker + # create root-owned. + execute *DASH.loadbalancer.prepare_boot execute *DASH.loadbalancer.start_or_run end else diff --git a/lib/dash/cli/proxy/loadbalancer_reboot.rb b/lib/dash/cli/proxy/loadbalancer_reboot.rb index 47d741b2..e60fdab9 100644 --- a/lib/dash/cli/proxy/loadbalancer_reboot.rb +++ b/lib/dash/cli/proxy/loadbalancer_reboot.rb @@ -21,6 +21,14 @@ def run execute *DASH.registry.login ensure_network + # After the network the bridge attaches the legacy network's containers to, + # and before anything that could create the new container or let `docker + # run --volume` create dash-loadbalancer-config empty: adopt the legacy + # volume's routing table, dynamic domains and ACME cache. Carries the + # apps-config mkdir this reboot paid a round trip for anyway + # (zoolutions/dash#168, see Dash::Commands::Loadbalancer#legacy_rename). + execute *DASH.loadbalancer.prepare_boot + info "Stopping and removing #{DASH.loadbalancer.container_name} on #{host}, if running..." execute *DASH.loadbalancer.stop, raise_on_non_zero_exit: false execute *DASH.loadbalancer.remove_container @@ -32,7 +40,6 @@ def run execute *DASH.loadbalancer.remove_proxy_secrets_file, raise_on_non_zero_exit: false end - execute *DASH.loadbalancer.ensure_apps_config_directory Dash::Cli::Proxy::LoadbalancerClaim.new(host, sshkit).claim_run_config(replace: true) execute *DASH.loadbalancer.run diff --git a/lib/dash/cli/proxy/reboot.rb b/lib/dash/cli/proxy/reboot.rb index 583d125d..29c977d3 100644 --- a/lib/dash/cli/proxy/reboot.rb +++ b/lib/dash/cli/proxy/reboot.rb @@ -37,7 +37,12 @@ def pull_image def replace_container execute *proxy.ensure_proxy_directory - execute *proxy.ensure_apps_config_directory + # Before the new container - and the new config volume `docker run + # --volume` would otherwise create empty - exists: bring a host still on + # pre-rename identity across. Carries the apps-config mkdir this reboot + # paid a round trip for anyway, so the bridge costs none here either + # (zoolutions/dash#168, see Dash::Cli::Proxy::LegacyRename). + execute *proxy.prepare_boot sync_proxy_secrets if proxy.port_holder? diff --git a/lib/dash/commands/loadbalancer.rb b/lib/dash/commands/loadbalancer.rb index 2c592d83..7e41761b 100644 --- a/lib/dash/commands/loadbalancer.rb +++ b/lib/dash/commands/loadbalancer.rb @@ -199,18 +199,16 @@ def legacy_rename_marker # 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 + # this bridge ever runs on a host, the marker is written despite the legacy volume's + # routing table and ACME cache never having been copied. The fix is upstream of the + # heuristic rather than in it - every path that can create the container, and with it + # the volume `docker run --volume` auto-creates empty, runs the bridge first: + # `boot`, both reboots (Dash::Cli::Proxy::Reboot, Dash::Cli::Proxy::LoadbalancerReboot) + # and `dash proxy loadbalancer start` (zoolutions/dash#168). + # + # If a host is somehow in that state anyway - an operator's own `docker run`, a + # volume created by hand - recovery is to copy the legacy volume's contents over, + # then remove .legacy-renamed under this host's loadbalancer directory so this # re-evaluates. def mark_legacy_renamed combine \ diff --git a/lib/dash/commands/proxy.rb b/lib/dash/commands/proxy.rb index e555876c..48d3aec8 100644 --- a/lib/dash/commands/proxy.rb +++ b/lib/dash/commands/proxy.rb @@ -101,6 +101,16 @@ def legacy_rename # first deploy. The subshell groups create-and-copy because `&&` and `||` # share precedence and associate left — without it a host that already has # the new volume would still run the copy over live state. + # + # The source volume is still mounted by the legacy container while this runs - + # deliberately, on every path (see Dash::Cli::Proxy::LegacyRename's step order), and + # safe because every writer into it renames into place: the routing table through + # writeFileAtomic, the dynamic domain and redirect state through their own temp + + # rename, the response cache through CreateTemp + Rename, and the ACME cache through + # autocert.DirCache. `cp -a` reads a complete file either way. Skew across files is + # possible and harmless - an unused certificate, or a route whose certificate reissues - + # and --recheck-targets-on-restore re-verifies the targets on the way back up + # (zoolutions/dash#169 review). def copy_legacy_config_volume(volume: Dash::Configuration::Proxy::CONFIG_VOLUME, legacy: Dash::Configuration::Proxy::LEGACY_CONFIG_VOLUME) copy_legacy_volume(legacy: legacy, volume: volume, image: proxy_image) end diff --git a/test/cli/cli_test_case.rb b/test/cli/cli_test_case.rb index 253d4de5..c29f0e37 100644 --- a/test/cli/cli_test_case.rb +++ b/test/cli/cli_test_case.rb @@ -66,11 +66,16 @@ def recorded_captures # 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. + # Each round trip is tagged with the host it went to. `on` runs hosts in parallel + # threads, so the order ACROSS hosts is whatever the scheduler chose that run - a pin + # that spells out host A's sequence and then host B's held only while the threads + # happened not to overlap (CI seed 59404 interleaved them). Only the order within one + # host's list is the gem's to promise; group by host and assert there. 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::Printer.any_instance.stubs(:execute_command).with { |cmd| round_trips << [ cmd.host.to_s, 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 } + .with { |*args| round_trips << [ SSHKit::Backend.current.host.to_s, args.reject { |arg| arg.is_a?(Hash) }.join(" ") ]; false } begin yield diff --git a/test/cli/proxy_test.rb b/test/cli/proxy_test.rb index ab544531..cf3536d3 100644 --- a/test/cli/proxy_test.rb +++ b/test/cli/proxy_test.rb @@ -78,6 +78,34 @@ class CliProxyTest < CliTestCase end end + # zoolutions/dash#168, the loadbalancer half: a dedicated LB host reached by `reboot` + # before it was ever booted would get `dash-loadbalancer-config` created empty by + # `docker run`, and its routing table, dynamic domains and ACME cache would never be + # adopted from `kamal-loadbalancer-config`. + test "reboot routes the loadbalancer host through the stage-3c bridge before creating the container" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("reboot", "-y", fixture: :with_loadbalancer).tap do |output| + create = output.index("docker network create dash on lb.example.com") + bridge = output.index("test -f .dash/loadbalancer/.legacy-renamed || (") + copy = output.index("docker volume inspect dash-loadbalancer-config > /dev/null 2>&1 || ! docker volume inspect kamal-loadbalancer-config") + run = output.index("docker run --name load-balancer") + + assert bridge, "reboot must send the loadbalancer host the bridge: #{output}" + assert create && copy && run, output + assert create < bridge, "the bridge attaches containers to the dash network, which has to exist first" + assert bridge < run, "the volume copy must land before docker run can create the volume empty" + end + end + + test "reboot carries the loadbalancer bridge in the apps-config round trip it already paid" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("reboot", "-y", fixture: :with_loadbalancer).tap do |output| + assert_match(/test -f \.dash\/loadbalancer\/\.legacy-renamed \|\| \(.*\) && mkdir -p \.dash\/proxy\/apps-config on lb\.example\.com/, output) + end + end + # SSHKit prefixes the first word of every command with /usr/bin/env, and # `env !` is exit 127 — so no chain the gem emits may start with `!`. test "no command sent to a host starts with shell negation" do @@ -115,8 +143,11 @@ class CliProxyTest < CliTestCase 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" + assert_equal %w[ 1.1.1.1 1.1.1.2 ], round_trips.keys.sort, "deploy_simple has two proxy hosts" + round_trips.each do |host, sequence| + assert_equal [ "docker network create dash" ] + PROXY_BOOT_ROUND_TRIPS_PER_HOST, sequence, + "#{host}: the network sweep, then the pinned sequence" + end end # Nothing changes for a host still running kamal-proxy: the three bridge steps travel @@ -136,6 +167,32 @@ class CliProxyTest < CliTestCase end end + # zoolutions/dash#168. `reboot` creates the new container, and `docker run --volume` + # auto-creates the new config volume empty if it is not there yet - so a host rebooted + # before it was ever booted would take the bridge's volume-existence guard out from + # under it, adopting nothing and recording itself migrated. The bridge belongs on this + # path too, ahead of anything that can create the new identity. + test "reboot routes a host through the stage-3c bridge before replacing the container" do + run_command("reboot", "-y").tap do |output| + bridge = output.index("test -f .dash/proxy/.legacy-renamed || (") + stop = output.index("docker container stop --time 40 dash-proxy on 1.1.1.1") + run = output.index("| xargs docker run --name dash-proxy") + + assert bridge, "reboot must send the bridge: #{output}" + assert stop && run, output + assert bridge < stop, "the bridge decides before the running proxy is taken down: #{output}" + assert bridge < run, "the new volume must not exist before the legacy one is copied into it" + end + end + + # The bridge rides the apps-config mkdir the reboot already paid a round trip for, so + # closing the gap costs none - the same fold boot got in zoolutions/dash#167. + test "reboot carries the bridge in the apps-config round trip it already paid" do + run_command("reboot", "-y").tap do |output| + assert_match(/test -f \.dash\/proxy\/\.legacy-renamed \|\| \(.*\) && mkdir -p \.dash\/proxy\/apps-config on 1\.1\.1\.1/, 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 @@ -1053,6 +1110,22 @@ class CliProxyTest < CliTestCase end end + # `start_or_run` falls through to `docker run` on a host with no container, which is + # the same volume-creating path `reboot` takes - so this admin command gets the bridge + # too, the last thing that can create dash-loadbalancer-config (zoolutions/dash#168). + test "loadbalancer start routes through the stage-3c bridge before it can run" do + Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) + + run_command("loadbalancer", "start", fixture: :with_loadbalancer).tap do |output| + bridge = output.index("test -f .dash/loadbalancer/.legacy-renamed || (") + start = output.index("docker container start load-balancer || docker run --name load-balancer") + + assert bridge, "loadbalancer start must send the bridge: #{output}" + assert start, output + assert bridge < start, "the volume copy has to land before docker run could create the volume empty" + end + end + test "loadbalancer stop" do Dash::Configuration::Proxy.any_instance.unstub(:load_balancing?) @@ -1535,13 +1608,16 @@ def stub_proxy_drift(version: Dash::Configuration::Proxy::Run::MINIMUM_VERSION) 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. + # Every round trip a proxy boot spends, per host: executes and captures interleaved in + # the order that host issued them, 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. Keyed by host because hosts run in parallel and only each host's own + # order is deterministic (see CliTestCase#recorded_commands_and_captures). def recorded_proxy_round_trips recorded_commands_and_captures { yield } - .reject { |command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) } - .map { |command| elide(command) } + .reject { |_host, command| command.match?(/\.dash\/lock-|mv \.kamal \.dash/) } + .group_by(&:first) + .transform_values { |pairs| pairs.map { |_host, command| elide(command) } } end # Collapses the parts that carry a digest, a boot-config read or the whole stage-3c diff --git a/test/test_helper.rb b/test/test_helper.rb index 1ccb51b8..7d5d6045 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -74,6 +74,21 @@ class ActiveSupport::TestCase # when it is on PATH, so a developer who has it installed would see different advice # than CI does. Pin it off; test/dockerfile/hadolint_test.rb turns it back on. Dash::Dockerfile::Hadolint.stubs(:available?).returns(false) + + # Fourth path, this one internal to the suite: a CLI test that runs a command with + # --quiet or -v leaves that verbosity behind on BOTH the DASH singleton and SSHKit's + # global output_verbosity (Cli::Base#initialize_commander, then + # Commander#configure_sshkit_with). Nothing restores either between tests - + # Commander#reset would, but only `dash alias` calls it - so + # `dash app stale_containers --quiet` in test/cli/app_test.rb silences every later + # SSHKit.config.output.info in the process. Whether that mattered depended on the + # seed: CI seed 36230 put it ahead of + # test/cli/healthcheck/progress_reporter_test.rb and took three of its assertions + # down to "", while the other three Ruby versions' seeds passed the same commit. + # Pin the default so the order cannot decide; a test that wants another verbosity + # still sets it itself. + DASH.verbosity = :info + SSHKit.config.output_verbosity = :info end # Dash::Commands::Base#ensure_run_directory — the one-shot .kamal -> .dash