From ba3476cfec9645670b2ab7a7b1babc3b368fce35 Mon Sep 17 00:00:00 2001 From: Hauke Klement Date: Thu, 19 Mar 2015 14:33:51 +0100 Subject: [PATCH] some refactoring --- app/controllers/submissions_controller.rb | 47 +++++++-------- lib/docker_client.rb | 31 +++++----- lib/docker_container_mixin.rb | 2 +- .../submissions_controller_spec.rb | 58 +++++++++++++++++-- spec/lib/docker_client_spec.rb | 13 ++--- spec/lib/docker_container_mixin_spec.rb | 32 ++++++---- 6 files changed, 121 insertions(+), 62 deletions(-) diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 2feb452c..23a6dbf8 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -5,7 +5,6 @@ class SubmissionsController < ApplicationController include SubmissionParameters include SubmissionScoring - around_action :with_server_sent_events, only: :run before_action :set_submission, only: [:download_file, :render_file, :run, :score, :show, :statistics, :stop, :test] before_action :set_docker_client, only: [:run, :test] before_action :set_files, only: [:download_file, :render_file, :show] @@ -47,22 +46,24 @@ class SubmissionsController < ApplicationController end def run - container_id = nil - stderr = '' - output = @docker_client.execute_run_command(@submission, params[:filename]) do |stream, chunk| - unless container_id - container_id = @docker_client.container_id - @server_sent_event.write({id: container_id, ports: @docker_client.assigned_ports}, event: 'info') + with_server_sent_events do |server_sent_event| + container_info_sent = false + stderr = '' + output = @docker_client.execute_run_command(@submission, params[:filename]) do |stream, chunk| + unless container_info_sent + server_sent_event.write({id: @docker_client.container.try(:id), port_bindings: @docker_client.container.try(:port_bindings)}, event: 'info') + container_info_sent = true + end + server_sent_event.write({stream => chunk}, event: 'output') + stderr += chunk if stream == :stderr end - @server_sent_event.write({stream => chunk}, event: 'output') - stderr += chunk if stream == :stderr - end - @server_sent_event.write(output, event: 'status') - if stderr.present? - if hint = Whistleblower.new(execution_environment: @submission.execution_environment).generate_hint(stderr) - @server_sent_event.write(hint, event: 'hint') - else - store_error(stderr) + server_sent_event.write(output, event: 'status') + if stderr.present? + if hint = Whistleblower.new(execution_environment: @submission.execution_environment).generate_hint(stderr) + server_sent_event.write(hint, event: 'hint') + else + store_error(stderr) + end end end end @@ -114,7 +115,7 @@ class SubmissionsController < ApplicationController end def store_error(stderr) - ::Error.create(execution_environment_id: @submission.exercise.execution_environment_id, message: stderr) + ::Error.create(execution_environment_id: @submission.execution_environment.id, message: stderr) end private :store_error @@ -125,16 +126,16 @@ class SubmissionsController < ApplicationController def with_server_sent_events response.headers['Content-Type'] = 'text/event-stream' - @server_sent_event = SSE.new(response.stream) - @server_sent_event.write(nil, event: 'start') - yield - @server_sent_event.write({code: 200}, event: 'close') + server_sent_event = SSE.new(response.stream) + server_sent_event.write(nil, event: 'start') + yield(server_sent_event) if block_given? + server_sent_event.write({code: 200}, event: 'close') rescue => exception logger.error(exception.message) logger.error(exception.backtrace.join("\n")) - @server_sent_event.write({code: 500}, event: 'close') + server_sent_event.write({code: 500}, event: 'close') ensure - @server_sent_event.close + server_sent_event.close end private :with_server_sent_events end diff --git a/lib/docker_client.rb b/lib/docker_client.rb index a7a77e6c..e5224236 100644 --- a/lib/docker_client.rb +++ b/lib/docker_client.rb @@ -7,8 +7,7 @@ class DockerClient MINIMUM_MEMORY_LIMIT = 4 RETRY_COUNT = 2 - attr_reader :assigned_ports - attr_reader :container_id + attr_reader :container def self.check_availability! Timeout.timeout(config[:connection_timeout]) { Docker.version } @@ -58,8 +57,8 @@ class DockerClient (tries += 1) <= RETRY_COUNT ? retry : raise(error) end - def create_workspace(container) - @submission.collect_files.each do |file| + def create_workspace_files(container, submission) + submission.collect_files.each do |file| FileUtils.mkdir_p(File.join(self.class.local_workspace_path(container), file.path || '')) if file.file_type.binary? copy_file_to_workspace(container: container, file: file) @@ -68,7 +67,7 @@ class DockerClient end end end - private :create_workspace + private :create_workspace_files def create_workspace_file(options = {}) file = File.new(local_file_path(options), 'w') @@ -79,31 +78,29 @@ class DockerClient def self.destroy_container(container) container.stop.kill - (container.port_bindings.try(:values) || []).each do |configuration| - port = configuration.first['HostPort'].to_i - PortPool.release(port) - end + container.port_bindings.values.each { |port| PortPool.release(port) } FileUtils.rm_rf(local_workspace_path(container)) if local_workspace_path(container) container.delete(force: true) end def execute_arbitrary_command(command, &block) + execute_command(command, nil, block) + end + + def execute_command(command, before_execution_block, output_consuming_block) tries ||= 0 - container = DockerContainerPool.get_container(@execution_environment) - @container_id = container.id - send_command(command, container, &block) + @container = DockerContainerPool.get_container(@execution_environment) + before_execution_block.try(:call) + send_command(command, @container, &output_consuming_block) rescue Excon::Errors::SocketError => error (tries += 1) <= RETRY_COUNT ? retry : raise(error) end [:run, :test].each do |cause| define_method("execute_#{cause}_command") do |submission, filename, &block| - container = DockerContainerPool.get_container(submission.execution_environment) - @container_id = container.id - @submission = submission - create_workspace(container) command = submission.execution_environment.send(:"#{cause}_command") % command_substitutions(filename) - send_command(command, container, &block) + create_workspace_files = proc { create_workspace_files(container, submission) } + execute_command(command, create_workspace_files, block) end end diff --git a/lib/docker_container_mixin.rb b/lib/docker_container_mixin.rb index 0563831f..a8d3c8a4 100644 --- a/lib/docker_container_mixin.rb +++ b/lib/docker_container_mixin.rb @@ -4,6 +4,6 @@ module DockerContainerMixin end def port_bindings - json['HostConfig']['PortBindings'] + json['HostConfig']['PortBindings'].try(:map) { |key, value| [key.to_i, value.first['HostPort'].to_i] }.to_h end end diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index 36e20fb5..92643f08 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -125,6 +125,7 @@ describe SubmissionsController do describe 'GET #run' do let(:filename) { submission.collect_files.detect(&:main_file?).name_with_extension } + let(:request) { get :run, filename: filename, id: submission.id } before(:each) do expect_any_instance_of(ActionController::Live::SSE).to receive(:write).at_least(3).times @@ -133,27 +134,27 @@ describe SubmissionsController do context 'when no errors occur during execution' do before(:each) do expect_any_instance_of(DockerClient).to receive(:execute_run_command).with(submission, filename).and_return({}) - get :run, filename: filename, id: submission.id + request end expect_assigns(docker_client: DockerClient) - expect_assigns(server_sent_event: ActionController::Live::SSE) expect_assigns(submission: :submission) expect_content_type('text/event-stream') expect_status(200) end context 'when an error occurs during execution' do - let(:hint) { "Your object 'main' of class 'Object' does not understand the method 'foo'." } let(:stderr) { "undefined method `foo' for main:Object (NoMethodError)" } before(:each) do expect_any_instance_of(DockerClient).to receive(:execute_run_command).with(submission, filename).and_yield(:stderr, stderr) end - after(:each) { get :run, filename: filename, id: submission.id } + after(:each) { request } context 'when the error is covered by a hint' do + let(:hint) { "Your object 'main' of class 'Object' does not understand the method 'foo'." } + before(:each) do expect_any_instance_of(Whistleblower).to receive(:generate_hint).with(stderr).and_return(hint) end @@ -236,4 +237,53 @@ describe SubmissionsController do expect_json expect_status(200) end + + describe '#with_server_sent_events' do + let(:response) { ActionController::TestResponse.new } + before(:each) { allow(controller).to receive(:response).and_return(response) } + + context 'when no error occurs' do + after(:each) { controller.send(:with_server_sent_events) } + + it 'uses server-sent events' do + expect(ActionController::Live::SSE).to receive(:new).and_call_original + end + + it "writes a 'start' event" do + allow_any_instance_of(ActionController::Live::SSE).to receive(:write) + expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with(nil, event: 'start') + end + + it "writes a 'close' event" do + allow_any_instance_of(ActionController::Live::SSE).to receive(:write) + expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with({code: 200}, event: 'close') + end + + it 'closes the stream' do + expect_any_instance_of(ActionController::Live::SSE).to receive(:close).and_call_original + end + end + + context 'when an error occurs' do + after(:each) { controller.send(:with_server_sent_events) { fail } } + + it 'uses server-sent events' do + expect(ActionController::Live::SSE).to receive(:new).and_call_original + end + + it "writes a 'start' event" do + allow_any_instance_of(ActionController::Live::SSE).to receive(:write) + expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with(nil, event: 'start') + end + + it "writes a 'close' event" do + allow_any_instance_of(ActionController::Live::SSE).to receive(:write) + expect_any_instance_of(ActionController::Live::SSE).to receive(:write).with({code: 500}, event: 'close') + end + + it 'closes the stream' do + expect_any_instance_of(ActionController::Live::SSE).to receive(:close).and_call_original + end + end + end end diff --git a/spec/lib/docker_client_spec.rb b/spec/lib/docker_client_spec.rb index 84dc518f..c87ef0eb 100644 --- a/spec/lib/docker_client_spec.rb +++ b/spec/lib/docker_client_spec.rb @@ -118,15 +118,14 @@ describe DockerClient, docker: true do end end - describe '#create_workspace' do + describe '#create_workspace_files' do let(:container) { double } before(:each) do - docker_client.instance_variable_set(:@submission, submission) expect(container).to receive(:binds).at_least(:once).and_return(["#{workspace_path}:#{DockerClient::CONTAINER_WORKSPACE_PATH}"]) end - after(:each) { docker_client.send(:create_workspace, container) } + after(:each) { docker_client.send(:create_workspace_files, container, submission) } it 'creates submission-specific directories' do expect(Dir).to receive(:mkdir).at_least(:once) @@ -233,8 +232,8 @@ describe DockerClient, docker: true do expect(DockerContainerPool).to receive(:get_container).with(submission.execution_environment).and_call_original end - it 'creates the workspace' do - expect(docker_client).to receive(:create_workspace) + it 'creates the workspace files' do + expect(docker_client).to receive(:create_workspace_files) end it 'executes the run command' do @@ -251,8 +250,8 @@ describe DockerClient, docker: true do expect(DockerContainerPool).to receive(:get_container).with(submission.execution_environment).and_call_original end - it 'creates the workspace' do - expect(docker_client).to receive(:create_workspace) + it 'creates the workspace files' do + expect(docker_client).to receive(:create_workspace_files) end it 'executes the test command' do diff --git a/spec/lib/docker_container_mixin_spec.rb b/spec/lib/docker_container_mixin_spec.rb index 80c7ced3..2ae95e50 100644 --- a/spec/lib/docker_container_mixin_spec.rb +++ b/spec/lib/docker_container_mixin_spec.rb @@ -1,18 +1,30 @@ require 'rails_helper' describe DockerContainerMixin do - [:binds, :port_bindings].each do |method| - describe "##{method}" do - let(:data) { [] } + describe '#binds' do + let(:binds) { [] } - it 'is defined for Docker::Container' do - expect(Docker::Container.instance_methods).to include(method) - end + it 'is defined for Docker::Container' do + expect(Docker::Container.instance_methods).to include(:binds) + end - it 'returns the correct information' do - expect(CONTAINER).to receive(:json).and_return('HostConfig' => {method.to_s.camelize => data}) - expect(CONTAINER.send(method)).to eq(data) - end + it 'returns the correct information' do + expect(CONTAINER).to receive(:json).and_return('HostConfig' => {'Binds' => binds}) + expect(CONTAINER.binds).to eq(binds) + end + end + + describe '#port_bindings' do + let(:port) { 1234 } + let(:port_bindings) { {"#{port}/tcp" => [{'HostIp' => '', 'HostPort' => port.to_s}]} } + + it 'is defined for Docker::Container' do + expect(Docker::Container.instance_methods).to include(:port_bindings) + end + + it 'returns the correct information' do + expect(CONTAINER).to receive(:json).and_return('HostConfig' => {'PortBindings' => port_bindings}) + expect(CONTAINER.port_bindings).to eq(port => port) end end end