Refactor sync_environment methods

* Add delete_environment method
* Change return value to allow raising an exception

app/controllers/execution_environments_controller.rb

@@ -177,6 +177,8 @@ class ExecutionEnvironmentsController < ApplicationController
 
     success = ExecutionEnvironment.all.map do |execution_environment|
       Runner.strategy_class.sync_environment(execution_environment)
+    rescue Runner::Error
+      false
     end
     if success.all?
       redirect_to ExecutionEnvironment, notice: t('execution_environments.index.synchronize_all.success')

app/models/execution_environment.rb

@@ -33,10 +33,11 @@ class ExecutionEnvironment < ApplicationRecord
   before_validation :clean_exposed_ports
   validates :exposed_ports, array: {numericality: {greater_than_or_equal_to: 0, less_than: 65_536, only_integer: true}}
 
-  def set_default_values
+  after_destroy :delete_runner_environment
-    set_default_values_if_present(permitted_execution_time: 60, pool_size: 0)
+  after_save :working_docker_image?, if: :validate_docker_image?
-  end
+
-  private :set_default_values
+  after_rollback :delete_runner_environment, on: :create
+  after_rollback :sync_runner_environment, on: %i[update destroy]
 
   def to_s
     name
@@ -86,5 +87,23 @@ class ExecutionEnvironment < ApplicationRecord
   rescue Runner::Error => e
     errors.add(:docker_image, "error: #{e}")
   end
-  private :working_docker_image?
+
+  def delete_runner_environment
+    Runner.strategy_class.remove_environment(self)
+  rescue Runner::Error => e
+    unless errors.include?(:docker_image)
+      errors.add(:docker_image, "error: #{e}")
+      raise ActiveRecord::RecordInvalid.new(self)
+    end
+  end
+
+  def sync_runner_environment
+    previous_saved_environment = self.class.find(id)
+    Runner.strategy_class.sync_environment(previous_saved_environment)
+  rescue Runner::Error => e
+    unless errors.include?(:docker_image)
+      errors.add(:docker_image, "error: #{e}")
+      raise ActiveRecord::RecordInvalid.new(self)
+    end
+  end
 end

app/models/runner.rb

@@ -140,16 +140,20 @@ class Runner < ApplicationRecord
     rescue Runner::Error::EnvironmentNotFound
       # Whenever the environment could not be found by the runner management, we
       # try to synchronize it and then forward a more specific error to our callee.
-      if strategy_class.sync_environment(execution_environment)
+      begin
-        raise Runner::Error::EnvironmentNotFound.new(
+        strategy_class.sync_environment(execution_environment)
-          "The execution environment with id #{execution_environment.id} was not found yet by the runner management. "\
+      rescue Runner::Error
-          'It has been successfully synced now so that the next request should be successful.'
+        # An additional error was raised during synchronization
-        )
-      else
         raise Runner::Error::EnvironmentNotFound.new(
           "The execution environment with id #{execution_environment.id} was not found by the runner management. "\
           'In addition, it could not be synced so that this probably indicates a permanent error.'
         )
+      else
+        # No error was raised during synchronization
+        raise Runner::Error::EnvironmentNotFound.new(
+          "The execution environment with id #{execution_environment.id} was not found yet by the runner management. "\
+          'It has been successfully synced now so that the next request should be successful.'
+        )
       end
     end
   end

lib/runner/strategy.rb

@@ -13,6 +13,10 @@ class Runner::Strategy
     raise NotImplementedError
   end
 
+  def self.remove_environment(_environment)
+    raise NotImplementedError
+  end
+
   def self.request_from_management(_environment)
     raise NotImplementedError
   end

lib/runner/strategy/docker_container_pool.rb

@@ -15,16 +15,35 @@ class Runner::Strategy::DockerContainerPool < Runner::Strategy
   end
 
   def self.sync_environment(environment)
-    # There is no dedicated sync mechanism yet. However, we need to emit a warning when the pool was previously
+    # Force a database commit and start a new transaction.
-    # empty for this execution environment. In this case the validation command probably was not executed.
+    if environment.class.connection.transaction_open?
-    return true unless environment.pool_size_previously_changed?
+      environment.class.connection.commit_db_transaction
-
+      environment.class.connection.begin_db_transaction
-    case environment.pool_size_previously_was
-      when nil, 0
-        false
-      else
-        true
     end
+
+    url = "#{config[:url]}/docker_container_pool/refill_environment/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Refilling execution environment at #{url}" }
+    response = Faraday.post(url)
+    return true if response.success?
+
+    raise Runner::Error::UnexpectedResponse.new("Could not refill execution environment in DockerContainerPool, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished refilling environment" }
+  end
+
+  def self.remove_environment(environment)
+    url = "#{config[:url]}/docker_container_pool/purge_environment/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Purging execution environment at #{url}" }
+    response = Faraday.delete(url)
+    return true if response.success?
+
+    raise Runner::Error::UnexpectedResponse.new("Could not delete execution environment in DockerContainerPool, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Request to DockerContainerPool failed: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished purging environment" }
   end
 
   def self.request_from_management(environment)

lib/runner/strategy/null.rb

@@ -5,7 +5,13 @@
 class Runner::Strategy::Null < Runner::Strategy
   def self.initialize_environment; end
 
-  def self.sync_environment(_environment); end
+  def self.sync_environment(_environment)
+    raise Runner::Error.new
+  end
+
+  def self.remove_environment(_environment)
+    raise Runner::Error.new
+  end
 
   def self.request_from_management(_environment)
     SecureRandom.uuid

lib/runner/strategy/poseidon.rb

@@ -21,14 +21,28 @@ class Runner::Strategy::Poseidon < Runner::Strategy
 
   def self.sync_environment(environment)
     url = "#{config[:url]}/execution-environments/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Synchronizing execution environment at #{url}" }
     response = http_connection.put url, environment.to_json
     return true if [201, 204].include? response.status
 
-    Rails.logger.warn("Could not create execution environment in Poseidon, got response: #{response.as_json}")
+    raise Runner::Error::UnexpectedResponse.new("Could not synchronize execution environment in Poseidon, got response: #{response.as_json}")
-    false
   rescue Faraday::Error => e
-    Rails.logger.warn("Could not create execution environment because of Faraday error: #{e.inspect}")
+    raise Runner::Error::FaradayError.new("Could not synchronize execution environment because of Faraday error: #{e.inspect}")
-    false
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished synchronizing execution environment" }
+  end
+
+  def self.remove_environment(environment)
+    url = "#{config[:url]}/execution-environments/#{environment.id}"
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Deleting execution environment at #{url}" }
+    response = http_connection.delete url
+    return true if response.status == 204
+
+    raise Runner::Error::UnexpectedResponse.new("Could not delete execution environment in Poseidon, got response: #{response.as_json}")
+  rescue Faraday::Error => e
+    raise Runner::Error::FaradayError.new("Could not delete execution environment because of Faraday error: #{e.inspect}")
+  ensure
+    Rails.logger.debug { "#{Time.zone.now.getutc.inspect}: Finished deleting execution environment" }
   end
 
   def self.request_from_management(environment)

spec/controllers/execution_environments_controller_spec.rb

@@ -46,7 +46,10 @@ describe ExecutionEnvironmentsController do
   end
 
   describe 'DELETE #destroy' do
-    before { delete :destroy, params: {id: execution_environment.id} }
+    before do
+      allow(Runner.strategy_class).to receive(:remove_environment).and_return(true)
+      delete :destroy, params: {id: execution_environment.id}
+    end
 
     expect_assigns(execution_environment: :execution_environment)
 
@@ -55,6 +58,10 @@ describe ExecutionEnvironmentsController do
       expect { delete :destroy, params: {id: execution_environment.id} }.to change(ExecutionEnvironment, :count).by(-1)
     end
 
+    it 'removes the execution environment from the runner management' do
+      expect(Runner.strategy_class).to have_received(:remove_environment)
+    end
+
     expect_redirect(:execution_environments)
   end
 

spec/lib/runner/strategy/poseidon_spec.rb

@@ -141,11 +141,11 @@ describe Runner::Strategy::Poseidon do
     end
 
     shared_examples 'returns false when the api request failed' do |status|
-      it "returns false on status #{status}" do
+      it "raises an exception on status #{status}" do
         faraday_connection = instance_double 'Faraday::Connection'
         allow(described_class).to receive(:http_connection).and_return(faraday_connection)
         allow(faraday_connection).to receive(:put).and_return(Faraday::Response.new(status: status))
-        expect(action.call).to be_falsey
+        expect { action.call }.to raise_exception Runner::Error::UnexpectedResponse
       end
     end
 
@@ -157,11 +157,11 @@ describe Runner::Strategy::Poseidon do
       include_examples 'returns false when the api request failed', status
     end
 
-    it 'returns false if Faraday raises an error' do
+    it 'raises an exception if Faraday raises an error' do
       faraday_connection = instance_double 'Faraday::Connection'
       allow(described_class).to receive(:http_connection).and_return(faraday_connection)
       allow(faraday_connection).to receive(:put).and_raise(Faraday::TimeoutError)
-      expect(action.call).to be_falsey
+      expect { action.call }.to raise_exception Runner::Error::FaradayError
     end
   end
 

spec/models/runner_spec.rb

@@ -233,7 +233,7 @@ describe Runner do
       end
 
       it 'raises an error when the environment could not be synced' do
-        allow(strategy_class).to receive(:sync_environment).with(runner.execution_environment).and_return(false)
+        allow(strategy_class).to receive(:sync_environment).with(runner.execution_environment).and_raise(Runner::Error::EnvironmentNotFound)
         expect { runner.send(:request_new_id) }.to raise_error(Runner::Error::EnvironmentNotFound, /#{environment_id}.*could not be synced/)
       end
     end