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

@@ -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

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

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)
+      def with_lock
-        return yield if KAMAL.holding_lock? || !mutates
+        if KAMAL.holding_lock?
-
-        run_hook "pre-connect"
-
-        ensure_run_and_locks_directory
-
-        acquire_lock
-
-        begin
           yield
-        rescue
+        else
-          begin
+          ensure_run_and_locks_directory
-            release_lock
-          rescue => e
-            say "Error releasing the deploy lock: #{e.message}", :red
-          end
-          raise
-        end
 
-        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

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
 

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
+      say "Log into image registry...", :magenta
-        invoke "kamal:cli:registry:login", [], invoke_options
+      invoke "kamal:cli:registry:login", [], invoke_options
 
-        if options[:skip_push]
+      if options[:skip_push]
-          say "Pull app image...", :magenta
+        say "Pull app image...", :magenta
-          invoke "kamal:cli:build:pull", [], invoke_options
+        invoke "kamal:cli:build:pull", [], invoke_options
-        else
+      else
-          say "Build and push app image...", :magenta
+        say "Build and push app image...", :magenta
-          invoke "kamal:cli:build:deliver", [], invoke_options
+        invoke "kamal:cli:build:deliver", [], invoke_options
-        end
+      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]
+      if options[:skip_push]
-          say "Pull app image...", :magenta
+        say "Pull app image...", :magenta
-          invoke "kamal:cli:build:pull", [], invoke_options
+        invoke "kamal:cli:build:pull", [], invoke_options
-        else
+      else
-          say "Build and push app image...", :magenta
+        say "Build and push app image...", :magenta
-          invoke "kamal:cli:build:deliver", [], invoke_options
+        invoke "kamal:cli:build:deliver", [], invoke_options
-        end
+      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

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)

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|
+      on(KAMAL.hosts | KAMAL.accessory_hosts) do |host|
-      unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false)
+        unless execute(*KAMAL.docker.installed?, raise_on_non_zero_exit: false)
-        if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false)
+          if execute(*KAMAL.docker.superuser?, raise_on_non_zero_exit: false)
-          info "Missing Docker on #{host}. Installing…"
+            info "Missing Docker on #{host}. Installing…"
-          execute *KAMAL.docker.install
+            execute *KAMAL.docker.install
-        else
+          else
-          missing << host
+            missing << host
+          end
         end
+
+        execute(*KAMAL.server.ensure_run_directory)
       end
 
-      execute(*KAMAL.server.ensure_run_directory)
+      if missing.any?
-    end
+        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?
+      run_hook "docker-setup"
-      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
-
-    run_hook "docker-setup"
   end
 end

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

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)

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" ] }
 

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|