From ecf470bddd10e9dbfa8bb1e56f575cc30ef5eadd Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Thu, 4 Nov 2021 01:03:12 +0100 Subject: [PATCH] Refactor sync_environment methods * Add delete_environment method * Change return value to allow raising an exception --- .../execution_environments_controller.rb | 2 + app/models/execution_environment.rb | 29 ++++++++++++--- app/models/runner.rb | 16 +++++--- lib/runner/strategy.rb | 4 ++ lib/runner/strategy/docker_container_pool.rb | 37 ++++++++++++++----- lib/runner/strategy/null.rb | 8 +++- lib/runner/strategy/poseidon.rb | 22 +++++++++-- .../execution_environments_controller_spec.rb | 9 ++++- spec/lib/runner/strategy/poseidon_spec.rb | 8 ++-- spec/models/runner_spec.rb | 2 +- 10 files changed, 106 insertions(+), 31 deletions(-) diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb index 889dfe71..1ee81ad4 100644 --- a/app/controllers/execution_environments_controller.rb +++ b/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') diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb index 247accc8..c51d697a 100644 --- a/app/models/execution_environment.rb +++ b/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 - set_default_values_if_present(permitted_execution_time: 60, pool_size: 0) - end - private :set_default_values + after_destroy :delete_runner_environment + after_save :working_docker_image?, if: :validate_docker_image? + + 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 diff --git a/app/models/runner.rb b/app/models/runner.rb index 6e308e4f..3e005185 100644 --- a/app/models/runner.rb +++ b/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 + strategy_class.sync_environment(execution_environment) + rescue Runner::Error + # An additional error was raised during synchronization + 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.' ) - 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.' - ) end end end diff --git a/lib/runner/strategy.rb b/lib/runner/strategy.rb index ce58a947..2a03f39c 100644 --- a/lib/runner/strategy.rb +++ b/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 diff --git a/lib/runner/strategy/docker_container_pool.rb b/lib/runner/strategy/docker_container_pool.rb index 42c66f66..d97c5fce 100644 --- a/lib/runner/strategy/docker_container_pool.rb +++ b/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 - # empty for this execution environment. In this case the validation command probably was not executed. - return true unless environment.pool_size_previously_changed? - - case environment.pool_size_previously_was - when nil, 0 - false - else - true + # Force a database commit and start a new transaction. + if environment.class.connection.transaction_open? + environment.class.connection.commit_db_transaction + environment.class.connection.begin_db_transaction 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) diff --git a/lib/runner/strategy/null.rb b/lib/runner/strategy/null.rb index 38ff522f..1c5f34a8 100644 --- a/lib/runner/strategy/null.rb +++ b/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 diff --git a/lib/runner/strategy/poseidon.rb b/lib/runner/strategy/poseidon.rb index c84365bf..8a83e62f 100644 --- a/lib/runner/strategy/poseidon.rb +++ b/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}") - false + raise Runner::Error::UnexpectedResponse.new("Could not synchronize execution environment in Poseidon, got response: #{response.as_json}") rescue Faraday::Error => e - Rails.logger.warn("Could not create execution environment because of Faraday error: #{e.inspect}") - false + raise Runner::Error::FaradayError.new("Could not synchronize execution environment because of Faraday error: #{e.inspect}") + 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) diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb index 2e3de1d3..b2b376ec 100644 --- a/spec/controllers/execution_environments_controller_spec.rb +++ b/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 diff --git a/spec/lib/runner/strategy/poseidon_spec.rb b/spec/lib/runner/strategy/poseidon_spec.rb index cec90a31..fb31429b 100644 --- a/spec/lib/runner/strategy/poseidon_spec.rb +++ b/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 diff --git a/spec/models/runner_spec.rb b/spec/models/runner_spec.rb index c5d7bf90..8c156555 100644 --- a/spec/models/runner_spec.rb +++ b/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