From 5f0815b140c55a69a8e1d48b25bcc1ec0446ceae Mon Sep 17 00:00:00 2001 From: Hauke Klement Date: Fri, 6 Feb 2015 15:59:51 +0100 Subject: [PATCH] implemented pooling for Docker containers --- Gemfile | 2 + Gemfile.lock | 7 + .../execution_environments_controller.rb | 5 +- app/models/execution_environment.rb | 13 +- .../execution_environments/_form.html.slim | 3 + .../execution_environments/show.html.slim | 2 +- config/docker.yml.erb | 5 + config/initializers/docker.rb | 3 + config/locales/de.yml | 1 + config/locales/en.yml | 1 + ...add_pool_size_to_execution_environments.rb | 11 + db/schema.rb | 3 +- lib/docker_client.rb | 115 +++----- lib/docker_container_pool.rb | 52 ++++ .../execution_environments_controller_spec.rb | 2 +- spec/factories/execution_environment.rb | 9 + spec/lib/docker_client_spec.rb | 262 ++++++++++-------- spec/lib/docker_container_pool_spec.rb | 121 ++++++++ spec/models/execution_environment_spec.rb | 35 ++- spec/support/docker.rb | 9 +- 20 files changed, 453 insertions(+), 208 deletions(-) create mode 100644 config/initializers/docker.rb create mode 100644 db/migrate/20150204080832_add_pool_size_to_execution_environments.rb create mode 100644 lib/docker_container_pool.rb create mode 100644 spec/lib/docker_container_pool_spec.rb diff --git a/Gemfile b/Gemfile index be18fee1..4e51d225 100644 --- a/Gemfile +++ b/Gemfile @@ -5,6 +5,8 @@ gem 'bcrypt', '~> 3.1.7' gem 'bootstrap-will_paginate' gem 'carrierwave' gem 'coffee-rails', '~> 4.0.0' +gem 'concurrent-ruby' +gem 'concurrent-ruby-ext', platform: :ruby gem 'docker-api', require: 'docker' gem 'factory_girl_rails', '~> 4.0' gem 'forgery' diff --git a/Gemfile.lock b/Gemfile.lock index 52c15ad5..0a482d48 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -84,6 +84,10 @@ GEM execjs coffee-script-source (1.9.0) colorize (0.7.5) + concurrent-ruby (0.8.0) + ref (~> 1.0, >= 1.0.5) + concurrent-ruby-ext (0.8.0) + concurrent-ruby (~> 0.8.0) database_cleaner (1.4.0) diff-lcs (1.2.5) docile (1.1.5) @@ -198,6 +202,7 @@ GEM i18n polyamorous (~> 1.1) rdoc (4.2.0) + ref (1.0.5) rspec (3.1.0) rspec-core (~> 3.1.0) rspec-expectations (~> 3.1.0) @@ -306,6 +311,8 @@ DEPENDENCIES carrierwave codeclimate-test-reporter coffee-rails (~> 4.0.0) + concurrent-ruby + concurrent-ruby-ext database_cleaner docker-api factory_girl_rails (~> 4.0) diff --git a/app/controllers/execution_environments_controller.rb b/app/controllers/execution_environments_controller.rb index 8cc58670..8a59cba7 100644 --- a/app/controllers/execution_environments_controller.rb +++ b/app/controllers/execution_environments_controller.rb @@ -25,11 +25,11 @@ class ExecutionEnvironmentsController < ApplicationController def execute_command @docker_client = DockerClient.new(execution_environment: @execution_environment, user: current_user) - render(json: @docker_client.execute_command(params[:command])) + render(json: @docker_client.execute_arbitrary_command(params[:command])) end def execution_environment_params - params[:execution_environment].permit(:docker_image, :exposed_ports, :editor_mode, :file_extension, :help, :indent_size, :name, :permitted_execution_time, :run_command, :test_command, :testing_framework).merge(user_id: current_user.id, user_type: current_user.class.name) + params[:execution_environment].permit(:docker_image, :exposed_ports, :editor_mode, :file_extension, :help, :indent_size, :name, :permitted_execution_time, :pool_size, :run_command, :test_command, :testing_framework).merge(user_id: current_user.id, user_type: current_user.class.name) end private :execution_environment_params @@ -44,6 +44,7 @@ class ExecutionEnvironmentsController < ApplicationController end def set_docker_images + DockerClient.check_availability! @docker_images = DockerClient.image_tags.sort rescue DockerClient::Error => error @docker_images = [] diff --git a/app/models/execution_environment.rb b/app/models/execution_environment.rb index 78c871e7..5d06a06a 100644 --- a/app/models/execution_environment.rb +++ b/app/models/execution_environment.rb @@ -3,6 +3,8 @@ class ExecutionEnvironment < ActiveRecord::Base VALIDATION_COMMAND = 'whoami' + after_initialize :set_default_values + has_many :exercises has_many :hints @@ -13,8 +15,15 @@ class ExecutionEnvironment < ActiveRecord::Base validates :docker_image, presence: true validates :name, presence: true validates :permitted_execution_time, numericality: {only_integer: true}, presence: true + validates :pool_size, numericality: {only_integer: true}, presence: true validates :run_command, presence: true + def set_default_values + self.permitted_execution_time ||= 60 + self.pool_size ||= 0 + end + private :set_default_values + def to_s name end @@ -27,13 +36,13 @@ class ExecutionEnvironment < ActiveRecord::Base private :valid_test_setup? def validate_docker_image? - docker_image.present? && Rails.env != 'test' + docker_image.present? && !Rails.env.test? end private :validate_docker_image? def working_docker_image? DockerClient.pull(docker_image) unless DockerClient.image_tags.include?(docker_image) - output = DockerClient.new(execution_environment: self).execute_command(VALIDATION_COMMAND) + output = DockerClient.new(execution_environment: self).execute_arbitrary_command(VALIDATION_COMMAND) errors.add(:docker_image, "error: #{output[:stderr]}") if output[:stderr].present? rescue DockerClient::Error => error errors.add(:docker_image, "error: #{error}") diff --git a/app/views/execution_environments/_form.html.slim b/app/views/execution_environments/_form.html.slim index 425a1a3e..126fa1b4 100644 --- a/app/views/execution_environments/_form.html.slim +++ b/app/views/execution_environments/_form.html.slim @@ -17,6 +17,9 @@ .form-group = f.label(:permitted_execution_time) = f.number_field(:permitted_execution_time, class: 'form-control', min: 1) + .form-group + = f.label(:pool_size) + = f.number_field(:pool_size, class: 'form-control', min: 0) .form-group = f.label(:run_command) = f.text_field(:run_command, class: 'form-control', placeholder: 'command %{filename}', required: true) diff --git a/app/views/execution_environments/show.html.slim b/app/views/execution_environments/show.html.slim index 21eee840..9c4f0652 100644 --- a/app/views/execution_environments/show.html.slim +++ b/app/views/execution_environments/show.html.slim @@ -4,7 +4,7 @@ h1 = row(label: 'execution_environment.name', value: @execution_environment.name) = row(label: 'execution_environment.user', value: link_to(@execution_environment.author, @execution_environment.author)) -- [:docker_image, :exposed_ports, :permitted_execution_time, :run_command, :test_command].each do |attribute| +- [:docker_image, :exposed_ports, :permitted_execution_time, :pool_size, :run_command, :test_command].each do |attribute| = row(label: "execution_environment.#{attribute}", value: @execution_environment.send(attribute)) = row(label: 'execution_environment.testing_framework', value: @testing_framework_adapter.try(:framework_name)) = row(label: 'execution_environment.help', value: render_markdown(@execution_environment.help)) diff --git a/config/docker.yml.erb b/config/docker.yml.erb index d140e017..68d50367 100644 --- a/config/docker.yml.erb +++ b/config/docker.yml.erb @@ -1,5 +1,8 @@ default: &default connection_timeout: 3 + pool: + active: false + interval: 60 ports: !ruby/range 4500..4600 development: @@ -9,6 +12,8 @@ development: production: <<: *default + pool: + active: true workspace_root: <%= Rails.root.join('tmp', 'files', Rails.env) %> test: diff --git a/config/initializers/docker.rb b/config/initializers/docker.rb new file mode 100644 index 00000000..09a57d06 --- /dev/null +++ b/config/initializers/docker.rb @@ -0,0 +1,3 @@ +DockerClient.initialize_environment +DockerContainerPool.start_refill_task if DockerContainerPool.config[:active] +at_exit { DockerContainerPool.clean_up } unless Rails.env.test? diff --git a/config/locales/de.yml b/config/locales/de.yml index f7b8aa7a..708767cc 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -13,6 +13,7 @@ de: help: Hilfetext name: Name permitted_execution_time: Erlaubte Ausführungszeit (in Sekunden) + pool_size: Docker-Container-Pool-Größe run_command: Ausführungsbefehl test_command: Testbefehl testing_framework: Testing-Framework diff --git a/config/locales/en.yml b/config/locales/en.yml index d2e64b3a..37b49018 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -13,6 +13,7 @@ en: help: Help Text name: Name permitted_execution_time: Permitted Execution Time (in Seconds) + pool_size: Docker Container Pool Size run_command: Run Command test_command: Test Command testing_framework: Testing Framework diff --git a/db/migrate/20150204080832_add_pool_size_to_execution_environments.rb b/db/migrate/20150204080832_add_pool_size_to_execution_environments.rb new file mode 100644 index 00000000..0980984f --- /dev/null +++ b/db/migrate/20150204080832_add_pool_size_to_execution_environments.rb @@ -0,0 +1,11 @@ +class AddPoolSizeToExecutionEnvironments < ActiveRecord::Migration + def change + add_column :execution_environments, :pool_size, :integer + + reversible do |direction| + direction.up do + ExecutionEnvironment.update_all(pool_size: 0) + end + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 3c8ff948..9d0c1bf2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150128093003) do +ActiveRecord::Schema.define(version: 20150204080832) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -44,6 +44,7 @@ ActiveRecord::Schema.define(version: 20150128093003) do t.integer "permitted_execution_time" t.integer "user_id" t.string "user_type" + t.integer "pool_size" end create_table "exercises", force: true do |t| diff --git a/lib/docker_client.rb b/lib/docker_client.rb index f9bdf050..d1a20e71 100644 --- a/lib/docker_client.rb +++ b/lib/docker_client.rb @@ -1,3 +1,5 @@ +require 'concurrent' + class DockerClient CONTAINER_WORKSPACE_PATH = '/workspace' LOCAL_WORKSPACE_ROOT = Rails.root.join('tmp', 'files', Rails.env) @@ -5,23 +7,12 @@ class DockerClient attr_reader :assigned_ports attr_reader :container_id - def bound_folders - @submission ? ["#{remote_workspace_path}:#{CONTAINER_WORKSPACE_PATH}"] : [] - end - private :bound_folders - def self.check_availability! - initialize_environment Timeout::timeout(config[:connection_timeout]) { Docker.version } rescue Excon::Errors::SocketError, Timeout::Error raise Error.new("The Docker host at #{Docker.url} is not reachable!") end - def clean_workspace - FileUtils.rm_rf(local_workspace_path) - end - private :clean_workspace - def command_substitutions(filename) {class_name: File.basename(filename, File.extname(filename)).camelize, filename: filename} end @@ -32,28 +23,29 @@ class DockerClient end def copy_file_to_workspace(options = {}) - FileUtils.cp(options[:file].native_file.path, File.join(local_workspace_path, options[:file].path || '', options[:file].name_with_extension)) + FileUtils.cp(options[:file].native_file.path, File.join(self.class.local_workspace_path(options[:container]), options[:file].path || '', options[:file].name_with_extension)) end - def create_container(options = {}) - Docker::Container.create('Cmd' => options[:command], 'Image' => @image.info['RepoTags'].first) + def self.create_container(execution_environment) + container = Docker::Container.create('Image' => find_image_by_tag(execution_environment.docker_image).info['RepoTags'].first, 'OpenStdin' => true, 'StdinOnce' => true) + container.start('Binds' => mapped_directories, 'PortBindings' => mapped_ports(execution_environment)) + container end - private :create_container - def create_workspace + def create_workspace(container) @submission.collect_files.each do |file| - FileUtils.mkdir_p(File.join(local_workspace_path, file.path || '')) + FileUtils.mkdir_p(File.join(self.class.local_workspace_path(container), file.path || '')) if file.file_type.binary? - copy_file_to_workspace(file: file) + copy_file_to_workspace(container: container, file: file) else - create_workspace_file(file: file) + create_workspace_file(container: container, file: file) end end end private :create_workspace def create_workspace_file(options = {}) - file = File.new(File.join(local_workspace_path, options[:file].path || '', options[:file].name_with_extension), 'w') + file = File.new(File.join(self.class.local_workspace_path(options[:container]), options[:file].path || '', options[:file].name_with_extension), 'w') file.write(options[:file].content) file.close end @@ -65,51 +57,43 @@ class DockerClient port = configuration.first['HostPort'].to_i PortPool.release(port) end + FileUtils.rm_rf(local_workspace_path(container)) container.delete(force: true) end - def execute_command(command, &block) - container = create_container(command: ['bash', '-c', command]) + def execute_arbitrary_command(command, &block) + container = DockerContainerPool.get_container(@execution_environment) @container_id = container.id - start_container(container, &block) + send_command(command, container, &block) end - def execute_in_workspace(submission, &block) - @submission = submission - create_workspace - block.call - ensure - clean_workspace if @submission - end - private :execute_in_workspace - - def execute_run_command(submission, filename, &block) - execute_in_workspace(submission) do - execute_command(@execution_environment.run_command % command_substitutions(filename), &block) + [: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) end end - def execute_test_command(submission, filename) - execute_in_workspace(submission) do - execute_command(@execution_environment.test_command % command_substitutions(filename)) - end - end - - def find_image_by_tag(tag) + def self.find_image_by_tag(tag) Docker::Image.all.detect { |image| image.info['RepoTags'].flatten.include?(tag) } end - private :find_image_by_tag + + def self.generate_remote_workspace_path + File.join(config[:workspace_root], SecureRandom.uuid) + end def self.image_tags - check_availability! Docker::Image.all.map { |image| image.info['RepoTags'] }.flatten.reject { |tag| tag.include?('') } end def initialize(options = {}) - self.class.check_availability! @execution_environment = options[:execution_environment] @user = options[:user] - @image = find_image_by_tag(@execution_environment.docker_image) + @image = self.class.find_image_by_tag(@execution_environment.docker_image) raise Error.new("Cannot find image #{@execution_environment.docker_image}!") unless @image end @@ -118,38 +102,33 @@ class DockerClient raise Error.new('Docker configuration missing!') end Docker.url = config[:host] if config[:host] + check_availability! + FileUtils.mkdir_p(LOCAL_WORKSPACE_ROOT) end - def local_workspace_path - File.join(LOCAL_WORKSPACE_ROOT, @submission.id.to_s) + def self.local_workspace_path(container) + Pathname.new(container.binds.first.split(':').first.sub(config[:workspace_root], LOCAL_WORKSPACE_ROOT.to_s)) end - private :local_workspace_path - def mapped_ports - @assigned_ports = [] - (@execution_environment.exposed_ports || '').gsub(/\s/, '').split(',').map do |port| - @assigned_ports << PortPool.available_port - ["#{port}/tcp", [{'HostPort' => @assigned_ports.last.to_s}]] + def self.mapped_directories + ["#{generate_remote_workspace_path}:#{CONTAINER_WORKSPACE_PATH}"] + end + + def self.mapped_ports(execution_environment) + (execution_environment.exposed_ports || '').gsub(/\s/, '').split(',').map do |port| + ["#{port}/tcp", [{'HostPort' => PortPool.available_port.to_s}]] end.to_h end - private :mapped_ports def self.pull(docker_image) `docker pull #{docker_image}` if docker_image end - def remote_workspace_path - File.join(self.class.config[:workspace_root], @submission.id.to_s) - end - private :remote_workspace_path - - def start_container(container, &block) + def send_command(command, container, &block) Timeout::timeout(@execution_environment.permitted_execution_time) do - container.start('Binds' => bound_folders, 'PortBindings' => mapped_ports) - container.wait(@execution_environment.permitted_execution_time) stderr = [] stdout = [] - container.streaming_logs(stderr: true, stdout: true) do |stream, chunk| + container.attach(stdin: StringIO.new(command)) do |stream, chunk| block.call(stream, chunk) if block_given? if stream == :stderr stderr.push(chunk) @@ -159,15 +138,13 @@ class DockerClient end {status: :ok, stderr: stderr.join, stdout: stdout.join} end - rescue Docker::Error::TimeoutError, Timeout::Error + rescue Timeout::Error {status: :timeout} ensure - self.class.destroy_container(container) + Concurrent::Future.execute { self.class.destroy_container(container) } end - private :start_container + private :send_command end class DockerClient::Error < RuntimeError end - -DockerClient.initialize_environment diff --git a/lib/docker_container_pool.rb b/lib/docker_container_pool.rb new file mode 100644 index 00000000..a2029447 --- /dev/null +++ b/lib/docker_container_pool.rb @@ -0,0 +1,52 @@ +require 'concurrent/future' +require 'concurrent/timer_task' +require 'concurrent/utilities' + +class DockerContainerPool + @containers = ThreadSafe::Hash[ExecutionEnvironment.all.map { |execution_environment| [execution_environment.id, ThreadSafe::Array.new] }] + + def self.clean_up + @refill_task.try(:shutdown) + @containers.each do |key, value| + while !value.empty? do + DockerClient.destroy_container(value.shift) + end + end + end + + def self.config + @config ||= CodeOcean::Config.new(:docker).read(erb: true)[:pool] + end + + def self.create_container(execution_environment) + DockerClient.create_container(execution_environment) + end + + def self.get_container(execution_environment) + if config[:active] + @containers[execution_environment.id].try(:shift) || create_container(execution_environment) + else + create_container(execution_environment) + end + end + + def self.quantities + @containers.map { |key, value| [key, value.length] }.to_h + end + + def self.refill + ExecutionEnvironment.all.each do |execution_environment| + refill_count = execution_environment.pool_size - @containers[execution_environment.id].length + if refill_count > 0 + Concurrent::Future.execute do + @containers[execution_environment.id] += refill_count.times.map { create_container(execution_environment) } + end + end + end + end + + def self.start_refill_task + @refill_task = Concurrent::TimerTask.new(execution_interval: config[:interval], run_now: true) { refill } + @refill_task.execute + end +end diff --git a/spec/controllers/execution_environments_controller_spec.rb b/spec/controllers/execution_environments_controller_spec.rb index 27be49e3..7e380008 100644 --- a/spec/controllers/execution_environments_controller_spec.rb +++ b/spec/controllers/execution_environments_controller_spec.rb @@ -61,7 +61,7 @@ describe ExecutionEnvironmentsController do before(:each) do expect(DockerClient).to receive(:new).with(execution_environment: execution_environment, user: user).and_call_original - expect_any_instance_of(DockerClient).to receive(:execute_command).with(command) + expect_any_instance_of(DockerClient).to receive(:execute_arbitrary_command).with(command) post :execute_command, command: command, id: execution_environment.id end diff --git a/spec/factories/execution_environment.rb b/spec/factories/execution_environment.rb index 9f533d55..7c9344a4 100644 --- a/spec/factories/execution_environment.rb +++ b/spec/factories/execution_environment.rb @@ -5,6 +5,7 @@ FactoryGirl.define do help name 'CoffeeScript' permitted_execution_time 10.seconds + pool_size 0 run_command 'coffee' singleton_execution_environment end @@ -15,6 +16,7 @@ FactoryGirl.define do help name 'HTML5' permitted_execution_time 10.seconds + pool_size 0 run_command 'touch' singleton_execution_environment test_command 'rspec %{filename} --format documentation' @@ -27,6 +29,7 @@ FactoryGirl.define do help name 'Java 8' permitted_execution_time 10.seconds + pool_size 0 run_command 'make run' singleton_execution_environment test_command 'make test CLASS_NAME="%{class_name}" FILENAME="%{filename}"' @@ -39,6 +42,7 @@ FactoryGirl.define do help name 'JRuby 1.7' permitted_execution_time 10.seconds + pool_size 0 run_command 'ruby %{filename}' singleton_execution_environment test_command 'rspec %{filename} --format documentation' @@ -51,6 +55,7 @@ FactoryGirl.define do help name 'Node.js' permitted_execution_time 10.seconds + pool_size 0 run_command 'node %{filename}' singleton_execution_environment end @@ -61,6 +66,7 @@ FactoryGirl.define do help name 'Python 2.7' permitted_execution_time 10.seconds + pool_size 0 run_command 'python %{filename}' singleton_execution_environment test_command 'python -m unittest --verbose %{module_name}' @@ -73,6 +79,7 @@ FactoryGirl.define do help name 'Ruby 2.1' permitted_execution_time 10.seconds + pool_size 0 run_command 'ruby %{filename}' singleton_execution_environment test_command 'rspec %{filename} --format documentation' @@ -86,6 +93,7 @@ FactoryGirl.define do help name 'Sinatra' permitted_execution_time 15.minutes + pool_size 0 run_command 'ruby %{filename}' singleton_execution_environment test_command 'rspec %{filename} --format documentation' @@ -98,6 +106,7 @@ FactoryGirl.define do help name 'SQLite' permitted_execution_time 1.minute + pool_size 0 run_command 'sqlite3 /database.db -init %{filename} -html' singleton_execution_environment test_command 'ruby %{filename}' diff --git a/spec/lib/docker_client_spec.rb b/spec/lib/docker_client_spec.rb index d27284a4..71b490a5 100644 --- a/spec/lib/docker_client_spec.rb +++ b/spec/lib/docker_client_spec.rb @@ -4,28 +4,11 @@ require 'seeds_helper' describe DockerClient, docker: true do let(:command) { 'whoami' } let(:docker_client) { DockerClient.new(execution_environment: FactoryGirl.build(:ruby), user: FactoryGirl.build(:admin)) } + let(:execution_environment) { FactoryGirl.build(:ruby) } let(:image) { double } let(:submission) { FactoryGirl.create(:submission) } let(:workspace_path) { '/tmp' } - describe '#bound_folders' do - context 'when executing a submission' do - before(:each) { docker_client.instance_variable_set(:@submission, submission) } - - it 'returns a submission-specific mapping' do - mapping = docker_client.send(:bound_folders).first - expect(mapping).to include(submission.id.to_s) - expect(mapping).to end_with(DockerClient::CONTAINER_WORKSPACE_PATH) - end - end - - context 'when executing a single command' do - it 'returns an empty mapping' do - expect(docker_client.send(:bound_folders)).to eq([]) - end - end - end - describe '.check_availability!' do context 'when a socket error occurs' do it 'raises an error' do @@ -42,111 +25,126 @@ describe DockerClient, docker: true do end end - describe '#clean_workspace' do - it 'removes the submission-specific directory' do - expect(docker_client).to receive(:local_workspace_path).and_return(workspace_path) - expect(FileUtils).to receive(:rm_rf).with(workspace_path) - docker_client.send(:clean_workspace) + describe '.create_container' do + after(:each) { DockerClient.create_container(execution_environment) } + + it 'uses the correct Docker image' do + expect(DockerClient).to receive(:find_image_by_tag).with(execution_environment.docker_image).and_call_original end - end - describe '#create_container' do - let(:image_tag) { 'tag' } - before(:each) { docker_client.instance_variable_set(:@image, image) } + it 'creates a container waiting for input' do + expect(Docker::Container).to receive(:create).with('Image' => kind_of(String), 'OpenStdin' => true, 'StdinOnce' => true).and_call_original + end - it 'creates a container' do - expect(image).to receive(:info).and_return({'RepoTags' => [image_tag]}) - expect(Docker::Container).to receive(:create).with('Cmd' => command, 'Image' => image_tag) - docker_client.send(:create_container, command: command) + it 'starts the container' do + expect_any_instance_of(Docker::Container).to receive(:start) + end + + it 'configures mapped directories' do + expect(DockerClient).to receive(:mapped_directories).and_call_original + expect_any_instance_of(Docker::Container).to receive(:start).with(hash_including('Binds' => kind_of(Array))) + end + + it 'configures mapped ports' do + expect(DockerClient).to receive(:mapped_ports).with(execution_environment).and_call_original + expect_any_instance_of(Docker::Container).to receive(:start).with(hash_including('PortBindings' => kind_of(Hash))) end end describe '#create_workspace' do - before(:each) { docker_client.instance_variable_set(:@submission, submission) } + 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) } it 'creates submission-specific directories' do - expect(docker_client).to receive(:local_workspace_path).at_least(:once).and_return(workspace_path) expect(Dir).to receive(:mkdir).at_least(:once) - docker_client.send(:create_workspace) + end + + it 'copies binary files' do + submission.collect_files.select { |file| file.file_type.binary? }.each do |file| + expect(docker_client).to receive(:copy_file_to_workspace).with(container: container, file: file) + end + end + + it 'creates non-binary files' do + submission.collect_files.reject { |file| file.file_type.binary? }.each do |file| + expect(docker_client).to receive(:create_workspace_file).with(container: container, file: file) + end end end describe '#create_workspace_file' do let(:file) { FactoryGirl.build(:file, content: 'puts 42') } let(:file_path) { File.join(workspace_path, file.name_with_extension) } + after(:each) { File.delete(file_path) } it 'creates a file' do - expect(docker_client).to receive(:local_workspace_path).and_return(workspace_path) - docker_client.send(:create_workspace_file, file: file) + expect(DockerClient).to receive(:local_workspace_path).and_return(workspace_path) + docker_client.send(:create_workspace_file, container: CONTAINER, file: file) expect(File.exist?(file_path)).to be true expect(File.new(file_path, 'r').read).to eq(file.content) - File.delete(file_path) end end describe '.destroy_container' do - let(:container) { docker_client.send(:create_container, {command: command}) } + let(:container) { DockerClient.send(:create_container, execution_environment) } after(:each) { DockerClient.destroy_container(container) } it 'stops the container' do expect(container).to receive(:stop).and_return(container) end - it 'kills the container' do + it 'kills running processes' do expect(container).to receive(:kill) end it 'releases allocated ports' do - expect(container).to receive(:json).at_least(:once).and_return({'HostConfig' => {'PortBindings' => {foo: [{'HostPort' => '42'}]}}}) + expect(container).to receive(:port_bindings).at_least(:once).and_return(foo: [{'HostPort' => '42'}]) expect(PortPool).to receive(:release) end + it 'removes the mapped directory' do + expect(DockerClient).to receive(:local_workspace_path).and_return(workspace_path) + expect(FileUtils).to receive(:rm_rf).with(workspace_path) + end + it 'deletes the container' do - expect(container).to receive(:delete) + expect(container).to receive(:delete).with(force: true) end end - describe '#execute_command' do - after(:each) { docker_client.send(:execute_command, command) } + describe '#execute_arbitrary_command' do + after(:each) { docker_client.execute_arbitrary_command(command) } - it 'creates a container' do - expect(docker_client).to receive(:create_container).with(command: ['bash', '-c', command]).and_call_original + it 'takes a container from the pool' do + expect(DockerContainerPool).to receive(:get_container).and_call_original end - it 'starts the container' do - expect(docker_client).to receive(:start_container) + it 'sends the command' do + expect(docker_client).to receive(:send_command).with(command, kind_of(Docker::Container)) end end - describe '#execute_in_workspace' do - let(:block) { Proc.new do; end } - let(:execute_in_workspace) { docker_client.send(:execute_in_workspace, submission, &block) } - after(:each) { execute_in_workspace } + describe '#execute_run_command' do + let(:filename) { submission.exercise.files.detect { |file| file.role == 'main_file' }.name_with_extension } + after(:each) { docker_client.send(:execute_run_command, submission, filename) } + + it 'takes a container from the pool' 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) end - it 'calls the block' do - expect(block).to receive(:call) - end - - it 'cleans the workspace' do - expect(docker_client).to receive(:clean_workspace) - end - end - - describe '#execute_run_command' do - let(:block) { Proc.new {} } - let(:filename) { submission.exercise.files.detect { |file| file.role == 'main_file' }.name_with_extension } - after(:each) { docker_client.send(:execute_run_command, submission, filename, &block) } - - it 'is executed in the workspace' do - expect(docker_client).to receive(:execute_in_workspace) - end - it 'executes the run command' do - expect(docker_client).to receive(:execute_command).with(kind_of(String), &block) + expect(submission.execution_environment).to receive(:run_command).and_call_original + expect(docker_client).to receive(:send_command).with(kind_of(String), kind_of(Docker::Container)) end end @@ -154,23 +152,36 @@ describe DockerClient, docker: true do let(:filename) { submission.exercise.files.detect { |file| file.role == 'teacher_defined_test' }.name_with_extension } after(:each) { docker_client.send(:execute_test_command, submission, filename) } - it 'is executed in the workspace' do - expect(docker_client).to receive(:execute_in_workspace) + it 'takes a container from the pool' 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) end it 'executes the test command' do - expect(docker_client).to receive(:execute_command).with(kind_of(String)) + expect(submission.execution_environment).to receive(:test_command).and_call_original + expect(docker_client).to receive(:send_command).with(kind_of(String), kind_of(Docker::Container)) + end + end + + describe '.generate_remote_workspace_path' do + it 'includes the correct workspace root' do + expect(DockerClient.generate_remote_workspace_path).to start_with(DockerClient.config[:workspace_root]) + end + + it 'includes a UUID' do + expect(SecureRandom).to receive(:uuid).and_call_original + DockerClient.generate_remote_workspace_path end end describe '.initialize_environment' do - let(:config) { {connection_timeout: 3, host: 'tcp://8.8.8.8:2375', workspace_root: '/'} } - context 'with complete configuration' do - before(:each) { expect(DockerClient).to receive(:config).at_least(:once).and_return(config) } - - it 'does not raise an error' do - expect { DockerClient.initialize_environment }.not_to raise_error + it 'creates the file directory' do + expect(FileUtils).to receive(:mkdir_p).with(DockerClient::LOCAL_WORKSPACE_ROOT) + DockerClient.initialize_environment end end @@ -183,75 +194,92 @@ describe DockerClient, docker: true do end end - describe '#local_workspace_path' do - before(:each) { docker_client.instance_variable_set(:@submission, submission) } + describe '.local_workspace_path' do + let(:container) { DockerClient.create_container(execution_environment) } + let(:local_workspace_path) { DockerClient.local_workspace_path(container) } - it 'includes the correct workspace root' do - expect(docker_client.send(:local_workspace_path)).to start_with(DockerClient::LOCAL_WORKSPACE_ROOT.to_s) + it 'returns a path' do + expect(local_workspace_path).to be_a(Pathname) end - it 'is submission-specific' do - expect(docker_client.send(:local_workspace_path)).to end_with(submission.id.to_s) + it 'includes the correct workspace root' do + expect(local_workspace_path.to_s).to start_with(DockerClient::LOCAL_WORKSPACE_ROOT.to_s) end end - describe '#remote_workspace_path' do - before(:each) { docker_client.instance_variable_set(:@submission, submission) } - - it 'includes the correct workspace root' do - expect(docker_client.send(:remote_workspace_path)).to start_with(DockerClient.config[:workspace_root]) - end - - it 'is submission-specific' do - expect(docker_client.send(:remote_workspace_path)).to end_with(submission.id.to_s) + describe '.mapped_directories' do + it 'returns a unique mapping' do + expect(DockerClient).to receive(:generate_remote_workspace_path).and_return(workspace_path) + mapping = DockerClient.send(:mapped_directories).first + expect(mapping).to start_with(workspace_path) + expect(mapping).to end_with(DockerClient::CONTAINER_WORKSPACE_PATH) end end - describe '#start_container' do - let(:container) { docker_client.send(:create_container, command: command) } - let(:start_container) { docker_client.send(:start_container, container) } + describe '.mapped_ports' do + context 'with exposed ports' do + before(:each) { execution_environment.exposed_ports = '3000' } - it 'configures bound folders' do - expect(container).to receive(:start).with(hash_including('Binds' => kind_of(Array))).and_call_original - start_container + it 'returns a mapping' do + expect(DockerClient.mapped_ports(execution_environment)).to be_a(Hash) + expect(DockerClient.mapped_ports(execution_environment).length).to eq(1) + end + + it 'retrieves available ports' do + expect(PortPool).to receive(:available_port) + DockerClient.mapped_ports(execution_environment) + end end - it 'configures bound ports' do - expect(container).to receive(:start).with(hash_including('PortBindings' => kind_of(Hash))).and_call_original - start_container + context 'without exposed ports' do + it 'returns an empty mapping' do + expect(DockerClient.mapped_ports(execution_environment)).to eq({}) + end + end + end + + describe '#send_command' do + let(:block) { Proc.new {} } + let(:container) { DockerClient.create_container(execution_environment) } + let(:send_command) { docker_client.send(:send_command, command, container, &block) } + after(:each) { send_command } + + it 'limits the execution time' do + expect(Timeout).to receive(:timeout).at_least(:once).with(kind_of(Numeric)).and_call_original end - it 'starts the container' do - expect(container).to receive(:start).and_call_original - start_container + it 'provides the command to be executed as input' do + expect(container).to receive(:attach).with(stdin: kind_of(StringIO)) end - it 'waits for the container to terminate' do - expect(container).to receive(:wait).with(kind_of(Numeric)).and_call_original - start_container + it 'calls the block' do + expect(block).to receive(:call) end context 'when a timeout occurs' do - before(:each) { expect(container).to receive(:wait).and_raise(Docker::Error::TimeoutError) } + before(:each) { expect(container).to receive(:attach).and_raise(Timeout::Error) } - it 'kills the container' do - expect(container).to receive(:kill) - start_container + it 'destroys the container asynchronously' do + expect(Concurrent::Future).to receive(:execute) end it 'returns a corresponding status' do - expect(start_container[:status]).to eq(:timeout) + expect(send_command[:status]).to eq(:timeout) end end context 'when the container terminates timely' do + it 'destroys the container asynchronously' do + expect(Concurrent::Future).to receive(:execute) + end + it "returns the container's output" do - expect(start_container[:stderr]).to be_blank - expect(start_container[:stdout]).to start_with('root') + expect(send_command[:stderr]).to be_blank + expect(send_command[:stdout]).to start_with('root') end it 'returns a corresponding status' do - expect(start_container[:status]).to eq(:ok) + expect(send_command[:status]).to eq(:ok) end end end diff --git a/spec/lib/docker_container_pool_spec.rb b/spec/lib/docker_container_pool_spec.rb new file mode 100644 index 00000000..08d70660 --- /dev/null +++ b/spec/lib/docker_container_pool_spec.rb @@ -0,0 +1,121 @@ +require 'rails_helper' + +describe DockerContainerPool do + let(:container) { double } + + def reload_class + load('docker_container_pool.rb') + end + private :reload_class + + before(:each) do + @execution_environment = FactoryGirl.create(:ruby) + reload_class + end + + it 'uses thread-safe data structures' do + expect(DockerContainerPool.instance_variable_get(:@containers)).to be_a(ThreadSafe::Hash) + expect(DockerContainerPool.instance_variable_get(:@containers)[@execution_environment.id]).to be_a(ThreadSafe::Array) + end + + describe '.clean_up' do + before(:each) { DockerContainerPool.instance_variable_set(:@refill_task, double) } + after(:each) { DockerContainerPool.clean_up } + + it 'stops the refill task' do + expect(DockerContainerPool.instance_variable_get(:@refill_task)).to receive(:shutdown) + end + + it 'destroys all containers' do + DockerContainerPool.instance_variable_get(:@containers).each do |key, value| + value.each do |container| + expect(DockerClient).to receive(:destroy_container).with(container) + end + end + end + end + + describe '.get_container' do + context 'when active' do + before(:each) do + expect(DockerContainerPool).to receive(:config).and_return(active: true) + end + + context 'with an available container' do + before(:each) { DockerContainerPool.instance_variable_get(:@containers)[@execution_environment.id].push(container) } + + it 'takes a container from the pool' do + expect(DockerContainerPool).not_to receive(:create_container).with(@execution_environment) + expect(DockerContainerPool.get_container(@execution_environment)).to eq(container) + end + end + + context 'without an available container' do + before(:each) do + expect(DockerContainerPool.instance_variable_get(:@containers)[@execution_environment.id]).to be_empty + end + + it 'creates a new container' do + expect(DockerContainerPool).to receive(:create_container).with(@execution_environment) + DockerContainerPool.get_container(@execution_environment) + end + end + end + + context 'when inactive' do + before(:each) do + expect(DockerContainerPool).to receive(:config).and_return(active: false) + end + + it 'creates a new container' do + expect(DockerContainerPool).to receive(:create_container).with(@execution_environment) + DockerContainerPool.get_container(@execution_environment) + end + end + end + + describe '.quantities' do + it 'maps execution environments to quantities of available containers' do + expect(DockerContainerPool.quantities.keys).to eq(ExecutionEnvironment.all.map(&:id)) + expect(DockerContainerPool.quantities.values.uniq).to eq([0]) + end + end + + describe '.refill' do + after(:each) { DockerContainerPool.refill } + + it 'regards all execution environments' do + ExecutionEnvironment.all.each do |execution_environment| + expect(DockerContainerPool.instance_variable_get(:@containers)).to receive(:[]).with(execution_environment.id).and_call_original + end + end + + context 'with something to refill' do + before(:each) { @execution_environment.update(pool_size: 1) } + + it 'works asynchronously' do + expect(Concurrent::Future).to receive(:execute) + end + end + + context 'with nothing to refill' do + before(:each) { @execution_environment.update(pool_size: 0) } + + it 'does nothing' do + expect(Concurrent::Future).not_to receive(:execute) + end + end + end + + describe '.start_refill_task' do + after(:each) { DockerContainerPool.start_refill_task } + + it 'creates an asynchronous task' do + expect(Concurrent::TimerTask).to receive(:new).and_call_original + end + + it 'executes the task' do + expect_any_instance_of(Concurrent::TimerTask).to receive(:execute) + end + end +end diff --git a/spec/models/execution_environment_spec.rb b/spec/models/execution_environment_spec.rb index 8465abd1..f347f2b0 100644 --- a/spec/models/execution_environment_spec.rb +++ b/spec/models/execution_environment_spec.rb @@ -4,10 +4,9 @@ describe ExecutionEnvironment do let(:execution_environment) { ExecutionEnvironment.create } it 'validates that the Docker image works', docker: true do - expect(execution_environment).to receive(:working_docker_image?).and_call_original expect(execution_environment).to receive(:validate_docker_image?).and_return(true) - execution_environment.update(docker_image: 'invalid') - expect(execution_environment.errors[:docker_image]).to be_present + expect(execution_environment).to receive(:working_docker_image?) + execution_environment.update(docker_image: FactoryGirl.attributes_for(:ruby)[:docker_image]) end it 'validates the presence of a Docker image name' do @@ -18,15 +17,26 @@ describe ExecutionEnvironment do expect(execution_environment.errors[:name]).to be_present end - it 'validates the numericality of a permitted run time' do + it 'validates the numericality of the permitted run time' do execution_environment.update(permitted_execution_time: Math::PI) expect(execution_environment.errors[:permitted_execution_time]).to be_present end it 'validates the presence of a permitted run time' do + execution_environment.update(permitted_execution_time: nil) expect(execution_environment.errors[:permitted_execution_time]).to be_present end + it 'validates the numericality of the pool size' do + execution_environment.update(pool_size: Math::PI) + expect(execution_environment.errors[:pool_size]).to be_present + end + + it 'validates the presence of a pool size' do + execution_environment.update(pool_size: nil) + expect(execution_environment.errors[:pool_size]).to be_present + end + it 'validates the presence of a run command' do expect(execution_environment.errors[:run_command]).to be_present end @@ -38,39 +48,40 @@ describe ExecutionEnvironment do describe '#validate_docker_image?' do it 'is false in the test environment' do + expect(Rails.env.test?).to be true expect(execution_environment.send(:validate_docker_image?)).to be false end it 'is false without a Docker image' do - allow(Rails).to receive(:env).and_return('production') + expect(execution_environment.docker_image).to be_blank expect(execution_environment.send(:validate_docker_image?)).to be false end it 'is true otherwise' do - execution_environment.docker_image = DockerClient.image_tags.first - expect(Rails).to receive(:env).and_return('production') + execution_environment.docker_image = FactoryGirl.attributes_for(:ruby)[:docker_image] + allow(Rails.env).to receive(:test?).and_return(false) expect(execution_environment.send(:validate_docker_image?)).to be true end end describe '#working_docker_image?', docker: true do let(:working_docker_image?) { execution_environment.send(:working_docker_image?) } - before(:each) { expect_any_instance_of(DockerClient).to receive(:find_image_by_tag).and_return(Object.new) } + before(:each) { expect(DockerClient).to receive(:find_image_by_tag).and_return(Object.new) } it 'instantiates a Docker client' do expect(DockerClient).to receive(:new).with(execution_environment: execution_environment).and_call_original - expect_any_instance_of(DockerClient).to receive(:execute_command).and_return({}) + expect_any_instance_of(DockerClient).to receive(:execute_arbitrary_command).and_return({}) working_docker_image? end it 'executes the validation command' do - expect_any_instance_of(DockerClient).to receive(:execute_command).with(ExecutionEnvironment::VALIDATION_COMMAND).and_return({}) + expect_any_instance_of(DockerClient).to receive(:execute_arbitrary_command).with(ExecutionEnvironment::VALIDATION_COMMAND).and_return({}) working_docker_image? end context 'when the command produces an error' do it 'adds an error' do - expect_any_instance_of(DockerClient).to receive(:execute_command).and_return({stderr: 'command not found'}) + expect_any_instance_of(DockerClient).to receive(:execute_arbitrary_command).and_return({stderr: 'command not found'}) working_docker_image? expect(execution_environment.errors[:docker_image]).to be_present end @@ -78,7 +89,7 @@ describe ExecutionEnvironment do context 'when the Docker client produces an error' do it 'adds an error' do - expect_any_instance_of(DockerClient).to receive(:execute_command).and_raise(DockerClient::Error) + expect_any_instance_of(DockerClient).to receive(:execute_arbitrary_command).and_raise(DockerClient::Error) working_docker_image? expect(execution_environment.errors[:docker_image]).to be_present end diff --git a/spec/support/docker.rb b/spec/support/docker.rb index 6e08d23d..10b50d6c 100644 --- a/spec/support/docker.rb +++ b/spec/support/docker.rb @@ -1,12 +1,15 @@ -IMAGE = Docker::Image.new(Docker::Connection.new('http://example.org', {}), 'id' => SecureRandom.hex) +CONTAINER = Docker::Container.send(:new, Docker::Connection.new('http://example.org', {}), 'id' => SecureRandom.hex) +IMAGE = Docker::Image.new(Docker::Connection.new('http://example.org', {}), 'id' => SecureRandom.hex, 'RepoTags' => [FactoryGirl.attributes_for(:ruby)[:docker_image]]) RSpec.configure do |config| config.before(:each) do |example| unless example.metadata[:docker] allow(DockerClient).to receive(:check_availability!).and_return(true) + allow(DockerClient).to receive(:create_container).and_return(CONTAINER) + allow(DockerClient).to receive(:find_image_by_tag).and_return(IMAGE) allow(DockerClient).to receive(:image_tags).and_return([IMAGE]) - allow_any_instance_of(DockerClient).to receive(:execute_command).and_return({}) - allow_any_instance_of(DockerClient).to receive(:find_image_by_tag).and_return(IMAGE) + allow(DockerClient).to receive(:local_workspace_path).and_return(Pathname.new('/tmp')) + allow_any_instance_of(DockerClient).to receive(:send_command).and_return({}) allow_any_instance_of(ExecutionEnvironment).to receive(:working_docker_image?) end end