From 0e73f027436059ee92c0d30223d5f5da3e7dbd76 Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Mon, 20 May 2024 15:03:38 +0100 Subject: [PATCH] Split lock and connection setup Allow run the pre-connect hook before the first SSH command is executed, but only run the locking in `with_lock` blocks. --- lib/kamal/cli/accessory.rb | 22 ++++++++--------- lib/kamal/cli/app.rb | 24 ++++++++++++------- lib/kamal/cli/base.rb | 47 ++++++++++++++++++++++--------------- lib/kamal/cli/env.rb | 4 ++-- lib/kamal/cli/main.rb | 48 +++++++++++++++++++------------------- lib/kamal/cli/prune.rb | 6 ++--- lib/kamal/cli/server.rb | 30 +++++++++++++----------- lib/kamal/cli/traefik.rb | 16 ++++++------- lib/kamal/commander.rb | 7 +++++- test/cli/main_test.rb | 6 +++++ test/cli/server_test.rb | 6 ++++- 11 files changed, 125 insertions(+), 91 deletions(-) diff --git a/lib/kamal/cli/accessory.rb b/lib/kamal/cli/accessory.rb index 8a4f6143..e1d31b77 100644 --- a/lib/kamal/cli/accessory.rb +++ b/lib/kamal/cli/accessory.rb @@ -1,7 +1,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "boot [NAME]", "Boot new accessory service on host (use NAME=all to boot all accessories)" def boot(name, login: true) - mutating do + with_lock do if name == "all" KAMAL.accessory_names.each { |accessory_name| boot(accessory_name) } else @@ -21,7 +21,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "upload [NAME]", "Upload accessory files to host", hide: true def upload(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do accessory.files.each do |(local, remote)| @@ -38,7 +38,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "directories [NAME]", "Create accessory directories on host", hide: true def directories(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do accessory.directories.keys.each do |host_path| @@ -51,7 +51,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "reboot [NAME]", "Reboot existing accessory on host (stop container, remove container, start new container; use NAME=all to boot all accessories)" def reboot(name) - mutating do + with_lock do if name == "all" KAMAL.accessory_names.each { |accessory_name| reboot(accessory_name) } else @@ -70,7 +70,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "start [NAME]", "Start existing accessory container on host" def start(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do execute *KAMAL.auditor.record("Started #{name} accessory"), verbosity: :debug @@ -82,7 +82,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "stop [NAME]", "Stop existing accessory container on host" def stop(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do execute *KAMAL.auditor.record("Stopped #{name} accessory"), verbosity: :debug @@ -94,7 +94,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "restart [NAME]", "Restart existing accessory container on host" def restart(name) - mutating do + with_lock do with_accessory(name) do stop(name) start(name) @@ -175,7 +175,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base option :confirmed, aliases: "-y", type: :boolean, default: false, desc: "Proceed without confirmation question" def remove(name) confirming "This will remove all containers, images and data directories for #{name}. Are you sure?" do - mutating do + with_lock do if name == "all" KAMAL.accessory_names.each { |accessory_name| remove_accessory(accessory_name) } else @@ -187,7 +187,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "remove_container [NAME]", "Remove accessory container from host", hide: true def remove_container(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do execute *KAMAL.auditor.record("Remove #{name} accessory container"), verbosity: :debug @@ -199,7 +199,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "remove_image [NAME]", "Remove accessory image from host", hide: true def remove_image(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do execute *KAMAL.auditor.record("Removed #{name} accessory image"), verbosity: :debug @@ -211,7 +211,7 @@ class Kamal::Cli::Accessory < Kamal::Cli::Base desc "remove_service_directory [NAME]", "Remove accessory directory used for uploaded files and data directories from host", hide: true def remove_service_directory(name) - mutating do + with_lock do with_accessory(name) do |accessory, hosts| on(hosts) do execute *accessory.remove_service_directory diff --git a/lib/kamal/cli/app.rb b/lib/kamal/cli/app.rb index ac2b9e40..ee8ab901 100644 --- a/lib/kamal/cli/app.rb +++ b/lib/kamal/cli/app.rb @@ -1,7 +1,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "boot", "Boot app on servers (or reboot app if already running)" def boot - mutating do + with_lock do say "Get most recent version available as an image...", :magenta unless options[:version] using_version(version_or_latest) do |version| say "Start container with version #{version} using a #{KAMAL.config.readiness_delay}s readiness delay (or reboot if already running)...", :magenta @@ -33,7 +33,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "start", "Start existing app container on servers" def start - mutating do + with_lock do on(KAMAL.hosts) do |host| roles = KAMAL.roles_on(host) @@ -47,7 +47,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "stop", "Stop app container on servers" def stop - mutating do + with_lock do on(KAMAL.hosts) do |host| roles = KAMAL.roles_on(host) @@ -135,7 +135,7 @@ class Kamal::Cli::App < Kamal::Cli::Base def stale_containers stop = options[:stop] - mutating(mutates: stop) do + with_lock_if_stopping do on(KAMAL.hosts) do |host| roles = KAMAL.roles_on(host) @@ -204,7 +204,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "remove", "Remove app containers and images from servers" def remove - mutating do + with_lock do stop remove_containers remove_images @@ -213,7 +213,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "remove_container [VERSION]", "Remove app container with given version from servers", hide: true def remove_container(version) - mutating do + with_lock do on(KAMAL.hosts) do |host| roles = KAMAL.roles_on(host) @@ -227,7 +227,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "remove_containers", "Remove all app containers from servers", hide: true def remove_containers - mutating do + with_lock do on(KAMAL.hosts) do |host| roles = KAMAL.roles_on(host) @@ -241,7 +241,7 @@ class Kamal::Cli::App < Kamal::Cli::Base desc "remove_images", "Remove all app images from servers", hide: true def remove_images - mutating do + with_lock do on(KAMAL.hosts) do execute *KAMAL.auditor.record("Removed all app images"), verbosity: :debug execute *KAMAL.app.remove_images @@ -284,4 +284,12 @@ class Kamal::Cli::App < Kamal::Cli::Base def version_or_latest options[:version] || KAMAL.config.latest_tag end + + def with_lock_if_stopping + if options[:stop] + with_lock { yield } + else + yield + end + end end diff --git a/lib/kamal/cli/base.rb b/lib/kamal/cli/base.rb index d363a8ee..e648281f 100644 --- a/lib/kamal/cli/base.rb +++ b/lib/kamal/cli/base.rb @@ -79,27 +79,27 @@ module Kamal::Cli puts " Finished all in #{sprintf("%.1f seconds", runtime)}" end - def mutating(mutates: true) - return yield if KAMAL.holding_lock? || !mutates - - run_hook "pre-connect" - - ensure_run_and_locks_directory - - acquire_lock - - begin + def with_lock + if KAMAL.holding_lock? yield - rescue - begin - release_lock - rescue => e - say "Error releasing the deploy lock: #{e.message}", :red - end - raise - end + else + ensure_run_and_locks_directory - release_lock + acquire_lock + + begin + yield + rescue + begin + release_lock + rescue => e + say "Error releasing the deploy lock: #{e.message}", :red + end + raise + end + + release_lock + end end def confirming(question) @@ -153,6 +153,15 @@ module Kamal::Cli end end + def on(*args, &block) + if !KAMAL.connected? + run_hook "pre-connect" + KAMAL.connected = true + end + + super + end + def command @kamal_command ||= begin invocation_class, invocation_commands = *first_invocation diff --git a/lib/kamal/cli/env.rb b/lib/kamal/cli/env.rb index 56ba505a..f12174a7 100644 --- a/lib/kamal/cli/env.rb +++ b/lib/kamal/cli/env.rb @@ -3,7 +3,7 @@ require "tempfile" class Kamal::Cli::Env < Kamal::Cli::Base desc "push", "Push the env file to the remote hosts" def push - mutating do + with_lock do on(KAMAL.hosts) do execute *KAMAL.auditor.record("Pushed env files"), verbosity: :debug @@ -30,7 +30,7 @@ class Kamal::Cli::Env < Kamal::Cli::Base desc "delete", "Delete the env file from the remote hosts" def delete - mutating do + with_lock do on(KAMAL.hosts) do execute *KAMAL.auditor.record("Deleted env files"), verbosity: :debug diff --git a/lib/kamal/cli/main.rb b/lib/kamal/cli/main.rb index 90e63aba..2816ab08 100644 --- a/lib/kamal/cli/main.rb +++ b/lib/kamal/cli/main.rb @@ -3,7 +3,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push" def setup print_runtime do - mutating do + with_lock do invoke_options = deploy_options say "Ensure Docker is installed...", :magenta @@ -22,22 +22,22 @@ class Kamal::Cli::Main < Kamal::Cli::Base option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push" def deploy runtime = print_runtime do - mutating do - invoke_options = deploy_options + invoke_options = deploy_options - say "Log into image registry...", :magenta - invoke "kamal:cli:registry:login", [], invoke_options + say "Log into image registry...", :magenta + invoke "kamal:cli:registry:login", [], invoke_options - if options[:skip_push] - say "Pull app image...", :magenta - invoke "kamal:cli:build:pull", [], invoke_options - else - say "Build and push app image...", :magenta - invoke "kamal:cli:build:deliver", [], invoke_options - end + if options[:skip_push] + say "Pull app image...", :magenta + invoke "kamal:cli:build:pull", [], invoke_options + else + say "Build and push app image...", :magenta + invoke "kamal:cli:build:deliver", [], invoke_options + end - run_hook "pre-deploy" + run_hook "pre-deploy" + with_lock do say "Ensure Traefik is running...", :magenta invoke "kamal:cli:traefik:boot", [], invoke_options @@ -58,17 +58,17 @@ class Kamal::Cli::Main < Kamal::Cli::Base option :skip_push, aliases: "-P", type: :boolean, default: false, desc: "Skip image build and push" def redeploy runtime = print_runtime do - mutating do - invoke_options = deploy_options + invoke_options = deploy_options - if options[:skip_push] - say "Pull app image...", :magenta - invoke "kamal:cli:build:pull", [], invoke_options - else - say "Build and push app image...", :magenta - invoke "kamal:cli:build:deliver", [], invoke_options - end + if options[:skip_push] + say "Pull app image...", :magenta + invoke "kamal:cli:build:pull", [], invoke_options + else + say "Build and push app image...", :magenta + invoke "kamal:cli:build:deliver", [], invoke_options + end + with_lock do run_hook "pre-deploy" say "Detect stale containers...", :magenta @@ -85,7 +85,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base def rollback(version) rolled_back = false runtime = print_runtime do - mutating do + with_lock do invoke_options = deploy_options KAMAL.config.version = version @@ -193,7 +193,7 @@ class Kamal::Cli::Main < Kamal::Cli::Base option :confirmed, aliases: "-y", type: :boolean, default: false, desc: "Proceed without confirmation question" def remove confirming "This will remove all containers and images. Are you sure?" do - mutating do + with_lock do invoke "kamal:cli:traefik:remove", [], options.without(:confirmed) invoke "kamal:cli:app:remove", [], options.without(:confirmed) invoke "kamal:cli:accessory:remove", [ "all" ], options diff --git a/lib/kamal/cli/prune.rb b/lib/kamal/cli/prune.rb index 498e4ec4..7635e97d 100644 --- a/lib/kamal/cli/prune.rb +++ b/lib/kamal/cli/prune.rb @@ -1,7 +1,7 @@ class Kamal::Cli::Prune < Kamal::Cli::Base desc "all", "Prune unused images and stopped containers" def all - mutating do + with_lock do containers images end @@ -9,7 +9,7 @@ class Kamal::Cli::Prune < Kamal::Cli::Base desc "images", "Prune unused images" def images - mutating do + with_lock do on(KAMAL.hosts) do execute *KAMAL.auditor.record("Pruned images"), verbosity: :debug execute *KAMAL.prune.dangling_images @@ -24,7 +24,7 @@ class Kamal::Cli::Prune < Kamal::Cli::Base retain = options.fetch(:retain, KAMAL.config.retain_containers) raise "retain must be at least 1" if retain < 1 - mutating do + with_lock do on(KAMAL.hosts) do execute *KAMAL.auditor.record("Pruned containers"), verbosity: :debug execute *KAMAL.prune.app_containers(retain: retain) diff --git a/lib/kamal/cli/server.rb b/lib/kamal/cli/server.rb index fe0ae62f..b545050f 100644 --- a/lib/kamal/cli/server.rb +++ b/lib/kamal/cli/server.rb @@ -23,25 +23,27 @@ class Kamal::Cli::Server < Kamal::Cli::Base desc "bootstrap", "Set up Docker to run Kamal apps" def bootstrap - missing = [] + with_lock do + missing = [] - on(KAMAL.hosts | KAMAL.accessory_hosts) do |host| - unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false) - if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false) - info "Missing Docker on #{host}. Installing…" - execute *KAMAL.docker.install - else - missing << host + on(KAMAL.hosts | KAMAL.accessory_hosts) do |host| + unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false) + if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false) + info "Missing Docker on #{host}. Installing…" + execute *KAMAL.docker.install + else + missing << host + end end + + execute(*KAMAL.server.ensure_run_directory) end - execute(*KAMAL.server.ensure_run_directory) - end + if missing.any? + raise "Docker is not installed on #{missing.join(", ")} and can't be automatically installed without having root access and either `wget` or `curl`. Install Docker manually: https://docs.docker.com/engine/install/" + end - if missing.any? - raise "Docker is not installed on #{missing.join(", ")} and can't be automatically installed without having root access and either `wget` or `curl`. Install Docker manually: https://docs.docker.com/engine/install/" + run_hook "docker-setup" end - - run_hook "docker-setup" end end diff --git a/lib/kamal/cli/traefik.rb b/lib/kamal/cli/traefik.rb index c13e2d80..d192ee1a 100644 --- a/lib/kamal/cli/traefik.rb +++ b/lib/kamal/cli/traefik.rb @@ -1,7 +1,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "boot", "Boot Traefik on servers" def boot - mutating do + with_lock do on(KAMAL.traefik_hosts) do execute *KAMAL.registry.login execute *KAMAL.traefik.start_or_run @@ -14,7 +14,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base option :confirmed, aliases: "-y", type: :boolean, default: false, desc: "Proceed without confirmation question" def reboot confirming "This will cause a brief outage on each host. Are you sure?" do - mutating do + with_lock do host_groups = options[:rolling] ? KAMAL.traefik_hosts : [ KAMAL.traefik_hosts ] host_groups.each do |hosts| host_list = Array(hosts).join(",") @@ -34,7 +34,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "start", "Start existing Traefik container on servers" def start - mutating do + with_lock do on(KAMAL.traefik_hosts) do execute *KAMAL.auditor.record("Started traefik"), verbosity: :debug execute *KAMAL.traefik.start @@ -44,7 +44,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "stop", "Stop existing Traefik container on servers" def stop - mutating do + with_lock do on(KAMAL.traefik_hosts) do execute *KAMAL.auditor.record("Stopped traefik"), verbosity: :debug execute *KAMAL.traefik.stop, raise_on_non_zero_exit: false @@ -54,7 +54,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "restart", "Restart existing Traefik container on servers" def restart - mutating do + with_lock do stop start end @@ -91,7 +91,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "remove", "Remove Traefik container and image from servers" def remove - mutating do + with_lock do stop remove_container remove_image @@ -100,7 +100,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "remove_container", "Remove Traefik container from servers", hide: true def remove_container - mutating do + with_lock do on(KAMAL.traefik_hosts) do execute *KAMAL.auditor.record("Removed traefik container"), verbosity: :debug execute *KAMAL.traefik.remove_container @@ -110,7 +110,7 @@ class Kamal::Cli::Traefik < Kamal::Cli::Base desc "remove_image", "Remove Traefik image from servers", hide: true def remove_image - mutating do + with_lock do on(KAMAL.traefik_hosts) do execute *KAMAL.auditor.record("Removed traefik image"), verbosity: :debug execute *KAMAL.traefik.remove_image diff --git a/lib/kamal/commander.rb b/lib/kamal/commander.rb index 9937cb69..c28fda82 100644 --- a/lib/kamal/commander.rb +++ b/lib/kamal/commander.rb @@ -2,12 +2,13 @@ require "active_support/core_ext/enumerable" require "active_support/core_ext/module/delegation" class Kamal::Commander - attr_accessor :verbosity, :holding_lock + attr_accessor :verbosity, :holding_lock, :connected delegate :hosts, :roles, :primary_host, :primary_role, :roles_on, :traefik_hosts, :accessory_hosts, to: :specifics def initialize self.verbosity = :info self.holding_lock = false + self.connected = false @specifics = nil end @@ -137,6 +138,10 @@ class Kamal::Commander self.holding_lock end + def connected? + self.connected + end + private # Lazy setup of SSHKit def configure_sshkit_with(config) diff --git a/test/cli/main_test.rb b/test/cli/main_test.rb index 358a7394..f7e479d7 100644 --- a/test/cli/main_test.rb +++ b/test/cli/main_test.rb @@ -92,6 +92,9 @@ class CliMainTest < CliTestCase test "deploy when locked" do Thread.report_on_exception = false + SSHKit::Backend::Abstract.any_instance.stubs(:execute) + Dir.stubs(:chdir) + SSHKit::Backend::Abstract.any_instance.stubs(:execute) .with { |*args| args == [ :mkdir, "-p", ".kamal" ] } @@ -113,6 +116,9 @@ class CliMainTest < CliTestCase test "deploy error when locking" do Thread.report_on_exception = false + SSHKit::Backend::Abstract.any_instance.stubs(:execute) + Dir.stubs(:chdir) + SSHKit::Backend::Abstract.any_instance.stubs(:execute) .with { |*args| args == [ :mkdir, "-p", ".kamal" ] } diff --git a/test/cli/server_test.rb b/test/cli/server_test.rb index c3dbb0bb..5d9fec4d 100644 --- a/test/cli/server_test.rb +++ b/test/cli/server_test.rb @@ -16,13 +16,15 @@ class CliServerTest < CliTestCase end test "bootstrap already installed" do + stub_setup SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "-v", raise_on_non_zero_exit: false).returns(true).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:mkdir, "-p", ".kamal").returns("").at_least_once - assert_equal "", run_command("bootstrap") + assert_equal "Acquiring the deploy lock...\nReleasing the deploy lock...", run_command("bootstrap") end test "bootstrap install as non-root user" do + stub_setup SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "-v", raise_on_non_zero_exit: false).returns(false).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with('[ "${EUID:-$(id -u)}" -eq 0 ] || command -v sudo >/dev/null || command -v su >/dev/null', raise_on_non_zero_exit: false).returns(false).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:mkdir, "-p", ".kamal").returns("").at_least_once @@ -33,11 +35,13 @@ class CliServerTest < CliTestCase end test "bootstrap install as root user" do + stub_setup SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:docker, "-v", raise_on_non_zero_exit: false).returns(false).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with('[ "${EUID:-$(id -u)}" -eq 0 ] || command -v sudo >/dev/null || command -v su >/dev/null', raise_on_non_zero_exit: false).returns(true).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:sh, "-c", "'curl -fsSL https://get.docker.com || wget -O - https://get.docker.com || echo \"exit 1\"'", "|", :sh).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with(:mkdir, "-p", ".kamal").returns("").at_least_once Kamal::Commands::Hook.any_instance.stubs(:hook_exists?).returns(true) + SSHKit::Backend::Abstract.any_instance.expects(:execute).with(".kamal/hooks/pre-connect", anything).at_least_once SSHKit::Backend::Abstract.any_instance.expects(:execute).with(".kamal/hooks/docker-setup", anything).at_least_once run_command("bootstrap").tap do |output|