Merge pull request #1079 from openHPI/sync_execution_environments

Sync execution environments

app/controllers/execution_environments_controller.rb

@@ -15,9 +15,7 @@ class ExecutionEnvironmentsController < ApplicationController
   def create
     @execution_environment = ExecutionEnvironment.new(execution_environment_params)
     authorize!
-    create_and_respond(object: @execution_environment) do
-      sync_to_runner_management
-    end
+    create_and_respond(object: @execution_environment)
   end
 
   def destroy
@@ -135,8 +133,8 @@ class ExecutionEnvironmentsController < ApplicationController
   def set_docker_images
     @docker_images ||= ExecutionEnvironment.pluck(:docker_image)
     @docker_images += Runner.strategy_class.available_images
-  rescue Runner::Error::InternalServerError => e
-    flash[:warning] = e.message
+  rescue Runner::Error => e
+    flash[:warning] = html_escape e.message
   ensure
     @docker_images = @docker_images.sort.uniq
   end
@@ -165,9 +163,7 @@ class ExecutionEnvironmentsController < ApplicationController
   end
 
   def update
-    update_and_respond(object: @execution_environment, params: execution_environment_params) do
-      sync_to_runner_management
-    end
+    update_and_respond(object: @execution_environment, params: execution_environment_params)
   end
 
   def sync_all_to_runner_management
@@ -175,20 +171,40 @@ class ExecutionEnvironmentsController < ApplicationController
 
     return unless Runner.management_active?
 
-    success = ExecutionEnvironment.all.map do |execution_environment|
-      Runner.strategy_class.sync_environment(execution_environment)
+    success = []
+
+    begin
+      # Get a list of all existing execution environments and mark them as a potential candidate for removal
+      environments_to_remove = Runner.strategy_class.environments.pluck(:id)
+      success << true
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while getting all execution environments: #{e.message}" }
+      environments_to_remove = []
+      success << false
     end
+
+    success += ExecutionEnvironment.all.map do |execution_environment|
+      # Sync all current execution environments and prevent deletion of those just synced
+      environments_to_remove -= [execution_environment.id]
+      Runner.strategy_class.sync_environment(execution_environment)
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while synchronizing execution environment with id #{execution_environment.id}: #{e.message}" }
+      false
+    end
+
+    success += environments_to_remove.map do |execution_environment_id|
+      # Remove execution environments not synced. We temporarily use a record which is not persisted
+      execution_environment = ExecutionEnvironment.new(id: execution_environment_id)
+      Runner.strategy_class.remove_environment(execution_environment)
+    rescue Runner::Error => e
+      Rails.logger.debug { "Runner error while deleting execution environment with id #{execution_environment.id}: #{e.message}" }
+      false
+    end
+
     if success.all?
       redirect_to ExecutionEnvironment, notice: t('execution_environments.index.synchronize_all.success')
     else
       redirect_to ExecutionEnvironment, alert: t('execution_environments.index.synchronize_all.failure')
     end
   end
-
-  def sync_to_runner_management
-    unless Runner.management_active? && Runner.strategy_class.sync_environment(@execution_environment)
-      t('execution_environments.form.errors.not_synced_to_runner_management')
-    end
-  end
-  private :sync_to_runner_management
 end

app/models/execution_environment.rb

@@ -19,8 +19,9 @@ class ExecutionEnvironment < ApplicationRecord
 
   scope :with_exercises, -> { where('id IN (SELECT execution_environment_id FROM exercises)') }
 
+  before_validation :clean_exposed_ports
+
   validate :valid_test_setup?
-  validate :working_docker_image?, if: :validate_docker_image?
   validates :docker_image, presence: true
   validates :memory_limit,
     numericality: {greater_than_or_equal_to: MINIMUM_MEMORY_LIMIT, only_integer: true}, presence: true
@@ -30,13 +31,13 @@ class ExecutionEnvironment < ApplicationRecord
   validates :pool_size, numericality: {only_integer: true}, presence: true
   validates :run_command, presence: true
   validates :cpu_limit, presence: true, numericality: {greater_than: 0, only_integer: true}
-  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
@@ -58,10 +59,15 @@ class ExecutionEnvironment < ApplicationRecord
     exposed_ports.join(', ')
   end
 
+  private
+
+  def set_default_values
+    set_default_values_if_present(permitted_execution_time: 60, pool_size: 0)
+  end
+
   def clean_exposed_ports
     self.exposed_ports = exposed_ports.uniq.sort
   end
-  private :clean_exposed_ports
 
   def valid_test_setup?
     if test_command? ^ testing_framework?
@@ -70,21 +76,49 @@ class ExecutionEnvironment < ApplicationRecord
           attribute: I18n.t('activerecord.attributes.execution_environment.testing_framework')))
     end
   end
-  private :valid_test_setup?
 
   def validate_docker_image?
-    docker_image.present? && !Rails.env.test?
+    # We only validate the code execution with the provided image if there is at least one container to test with.
+    pool_size.positive? && docker_image.present? && !Rails.env.test?
   end
-  private :validate_docker_image?
 
   def working_docker_image?
-    runner = Runner.for(author, self)
-    output = runner.execute_command(VALIDATION_COMMAND)
-    errors.add(:docker_image, "error: #{output[:stderr]}") if output[:stderr].present?
-  rescue Runner::Error::NotAvailable => e
-    Rails.logger.info("The Docker image could not be verified: #{e}")
-  rescue Runner::Error => e
-    errors.add(:docker_image, "error: #{e}")
+    sync_runner_environment
+    retries = 0
+    begin
+      runner = Runner.for(author, self)
+      output = runner.execute_command(VALIDATION_COMMAND)
+      errors.add(:docker_image, "error: #{output[:stderr]}") if output[:stderr].present?
+    rescue Runner::Error => e
+      # In case of an Runner::Error, we retry multiple times before giving up.
+      # The time between each retry increases to allow the runner management to catch up.
+      if retries < 5 && !Rails.env.test?
+        retries += 1
+        sleep retries
+        retry
+      elsif errors.exclude?(:docker_image)
+        errors.add(:docker_image, "error: #{e}")
+        raise ActiveRecord::RecordInvalid.new(self)
+      end
+    end
+  end
+
+  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
-  private :working_docker_image?
 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
+        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