From c1ac401a49fe612f6a5ff73707db1b2b5648a8f5 Mon Sep 17 00:00:00 2001
From: Sebastian Serth <Sebastian.Serth@hpi.de>
Date: Thu, 4 Nov 2021 01:04:52 +0100
Subject: [PATCH] Add retries to working_docker_image?

* Also synchronize images during save
---
 .../execution_environments_controller.rb      | 15 ++------
 app/models/execution_environment.rb           | 25 +++++++++----
 .../execution_environments_controller_spec.rb | 36 ++++++++++++++-----
 spec/models/execution_environment_spec.rb     |  6 ++--
 4 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb
index 1ee81ad4..4dc00502 100644
--- a/app/controllers/execution_environments_controller.rb
+++ b/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
@@ -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
@@ -186,11 +182,4 @@ class ExecutionEnvironmentsController < ApplicationController
       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
diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb
index c51d697a..b5f16157 100644
--- a/app/models/execution_environment.rb
+++ b/app/models/execution_environment.rb
@@ -79,13 +79,24 @@ class ExecutionEnvironment < ApplicationRecord
   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
diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb
index b2b376ec..9aee3e5a 100644
--- a/spec/controllers/execution_environments_controller_spec.rb
+++ b/spec/controllers/execution_environments_controller_spec.rb
@@ -8,7 +8,6 @@ describe ExecutionEnvironmentsController do
 
   before do
     allow(controller).to receive(:current_user).and_return(user)
-    allow(controller).to receive(:sync_to_runner_management).and_return(nil)
     allow(Runner.strategy_class).to receive(:available_images).and_return([])
   end
 
@@ -16,7 +15,14 @@ describe ExecutionEnvironmentsController do
     context 'with a valid execution environment' do
       let(:perform_request) { proc { post :create, params: {execution_environment: FactoryBot.build(:ruby).attributes} } }
 
-      before { perform_request.call }
+      before do
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        runner = instance_double 'runner'
+        allow(Runner).to receive(:for).and_return(runner)
+        allow(runner).to receive(:execute_command).and_return({})
+        perform_request.call
+      end
 
       expect_assigns(docker_images: Array)
       expect_assigns(execution_environment: ExecutionEnvironment)
@@ -26,21 +32,25 @@ describe ExecutionEnvironmentsController do
       end
 
       it 'registers the execution environment with the runner management' do
-        expect(controller).to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).to have_received(:sync_environment)
       end
 
       expect_redirect(ExecutionEnvironment.last)
     end
 
     context 'with an invalid execution environment' do
-      before { post :create, params: {execution_environment: {}} }
+      before do
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        post :create, params: {execution_environment: {}}
+      end
 
       expect_assigns(execution_environment: ExecutionEnvironment)
       expect_status(200)
       expect_template(:new)
 
       it 'does not register the execution environment with the runner management' do
-        expect(controller).not_to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).not_to have_received(:sync_environment)
       end
     end
   end
@@ -171,7 +181,11 @@ describe ExecutionEnvironmentsController do
   describe 'PUT #update' do
     context 'with a valid execution environment' do
       before do
-        allow(controller).to receive(:sync_to_runner_management).and_return(nil)
+        allow(Rails.env).to receive(:test?).and_return(false, true)
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        runner = instance_double 'runner'
+        allow(Runner).to receive(:for).and_return(runner)
+        allow(runner).to receive(:execute_command).and_return({})
         put :update, params: {execution_environment: FactoryBot.attributes_for(:ruby), id: execution_environment.id}
       end
 
@@ -180,19 +194,23 @@ describe ExecutionEnvironmentsController do
       expect_redirect(:execution_environment)
 
       it 'updates the execution environment at the runner management' do
-        expect(controller).to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).to have_received(:sync_environment)
       end
     end
 
     context 'with an invalid execution environment' do
-      before { put :update, params: {execution_environment: {name: ''}, id: execution_environment.id} }
+      before do
+        allow(Runner.strategy_class).to receive(:sync_environment).and_return(true)
+        allow(Rails.env).to receive(:test?).and_return(true, false, true)
+        put :update, params: {execution_environment: {name: ''}, id: execution_environment.id}
+      end
 
       expect_assigns(execution_environment: ExecutionEnvironment)
       expect_status(200)
       expect_template(:edit)
 
       it 'does not update the execution environment at the runner management' do
-        expect(controller).not_to have_received(:sync_to_runner_management)
+        expect(Runner.strategy_class).not_to have_received(:sync_environment)
       end
     end
   end
diff --git a/spec/models/execution_environment_spec.rb b/spec/models/execution_environment_spec.rb
index 5b41f4b8..8bfb0b07 100644
--- a/spec/models/execution_environment_spec.rb
+++ b/spec/models/execution_environment_spec.rb
@@ -8,7 +8,7 @@ describe ExecutionEnvironment do
   it 'validates that the Docker image works' do
     allow(execution_environment).to receive(:validate_docker_image?).and_return(true)
     allow(execution_environment).to receive(:working_docker_image?).and_return(true)
-    execution_environment.update(docker_image: FactoryBot.attributes_for(:ruby)[:docker_image])
+    execution_environment.update(FactoryBot.build(:ruby).attributes)
     expect(execution_environment).to have_received(:working_docker_image?)
   end
 
@@ -146,10 +146,12 @@ describe ExecutionEnvironment do
   end
 
   describe '#working_docker_image?' do
+    let(:execution_environment) { FactoryBot.create(:ruby) }
     let(:working_docker_image?) { execution_environment.send(:working_docker_image?) }
     let(:runner) { instance_double 'runner' }
 
     before do
+      allow(execution_environment).to receive(:sync_runner_environment).and_return(true)
       allow(Runner).to receive(:for).with(execution_environment.author, execution_environment).and_return runner
     end
 
@@ -176,7 +178,7 @@ describe ExecutionEnvironment do
     context 'when the Docker client produces an error' do
       it 'adds an error' do
         allow(runner).to receive(:execute_command).and_raise(Runner::Error)
-        working_docker_image?
+        expect { working_docker_image? }.to raise_error(ActiveRecord::RecordInvalid)
         expect(execution_environment.errors[:docker_image]).to be_present
       end
     end