From fae60a21e59debe757a5ba52afe93d3d6792bc6d Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Mon, 28 Sep 2020 16:16:53 +0200 Subject: [PATCH] Sync DockerClient with DockerContainerPool Fix failing specs after upgrade --- config/docker.yml.erb.example | 2 +- config/docker.yml.erb.travis | 2 +- lib/docker_client.rb | 83 ++++++++++++++++++++-------------- lib/docker_container_mixin.rb | 2 + spec/lib/docker_client_spec.rb | 59 +++++++++++++----------- 5 files changed, 84 insertions(+), 64 deletions(-) diff --git a/config/docker.yml.erb.example b/config/docker.yml.erb.example index f431da15..a5b0d7e4 100644 --- a/config/docker.yml.erb.example +++ b/config/docker.yml.erb.example @@ -52,4 +52,4 @@ staging: test: <<: *default host: tcp://127.0.0.1:2376 - workspace_root: <%= File.join('/', 'shared', Rails.env) %> + workspace_root: <%= Rails.root.join('tmp', 'files', Rails.env) %> diff --git a/config/docker.yml.erb.travis b/config/docker.yml.erb.travis index f431da15..a5b0d7e4 100644 --- a/config/docker.yml.erb.travis +++ b/config/docker.yml.erb.travis @@ -52,4 +52,4 @@ staging: test: <<: *default host: tcp://127.0.0.1:2376 - workspace_root: <%= File.join('/', 'shared', Rails.env) %> + workspace_root: <%= Rails.root.join('tmp', 'files', Rails.env) %> diff --git a/lib/docker_client.rb b/lib/docker_client.rb index cfd83fda..c1912f81 100644 --- a/lib/docker_client.rb +++ b/lib/docker_client.rb @@ -33,9 +33,9 @@ class DockerClient if local_workspace_path && Pathname.new(local_workspace_path).exist? Pathname.new(local_workspace_path).children.each do |p| p.rmtree - rescue Errno::ENOENT, Errno::EACCES => e - Raven.capture_exception(e) - Rails.logger.error("clean_container_workspace: Got #{e.class}: #{e}") + rescue Errno::ENOENT, Errno::EACCES => error + Raven.capture_exception(error) + Rails.logger.error("clean_container_workspace: Got #{error.class.to_s}: #{error.to_s}") end # FileUtils.rmdir(Pathname.new(local_workspace_path)) end @@ -55,12 +55,10 @@ class DockerClient @config ||= CodeOcean::Config.new(:docker).read(erb: true) end - def self.container_creation_options(execution_environment) + def self.container_creation_options(execution_environment, local_workspace_path) { 'Image' => find_image_by_tag(execution_environment.docker_image).info['RepoTags'].first, - 'Memory' => execution_environment.memory_limit.megabytes, 'NetworkDisabled' => !execution_environment.network_enabled?, - # 'HostConfig' => { 'CpusetCpus' => '0', 'CpuQuota' => 10000 }, # DockerClient.config['allowed_cpus'] 'OpenStdin' => true, 'StdinOnce' => true, @@ -68,14 +66,16 @@ class DockerClient 'AttachStdout' => true, 'AttachStdin' => true, 'AttachStderr' => true, - 'Tty' => true - } - end - - def self.container_start_options(execution_environment, local_workspace_path) - { + 'Tty' => true, 'Binds' => mapped_directories(local_workspace_path), - 'PortBindings' => mapped_ports(execution_environment) + 'PortBindings' => mapped_ports(execution_environment), + # Resource limitations. + 'NanoCPUs' => 4 * 1000000000, # CPU quota in units of 10^-9 CPUs. + 'PidsLimit' => 100, + 'KernelMemory' => execution_environment.memory_limit.megabytes, # if below Memory, the Docker host (!) might experience an OOM + 'Memory' => execution_environment.memory_limit.megabytes, + 'MemorySwap' => execution_environment.memory_limit.megabytes, # same value as Memory to disable Swap + 'OomScoreAdj' => 500 } end @@ -111,31 +111,32 @@ class DockerClient def self.create_container(execution_environment) tries ||= 0 - # Rails.logger.info "docker_client: self.create_container with creation options:" - # Rails.logger.info(container_creation_options(execution_environment)) - container = Docker::Container.create(container_creation_options(execution_environment)) # container.start sometimes creates the passed local_workspace_path on disk (depending on the setup). # this is however not guaranteed and caused issues on the server already. Therefore create the necessary folders manually! local_workspace_path = generate_local_workspace_path FileUtils.mkdir(local_workspace_path) - container.start(container_start_options(execution_environment, local_workspace_path)) + FileUtils.chmod_R(0777, local_workspace_path) + container = Docker::Container.create(container_creation_options(execution_environment, local_workspace_path)) + container.start container.start_time = Time.now container.status = :created + container.execution_environment = execution_environment container.re_use = true + container.docker_client = new(execution_environment: execution_environment) Thread.new do timeout = Random.rand(MINIMUM_CONTAINER_LIFETIME..MAXIMUM_CONTAINER_LIFETIME) # seconds sleep(timeout) container.re_use = false if container.status != :executing - new(execution_environment: execution_environment).kill_container(container, false) - Rails.logger.info('Killing container in status ' + container.status + ' after ' + (Time.now - container.start_time).to_s + ' seconds.') + container.docker_client.kill_container(container, false) + Rails.logger.info('Killing container in status ' + container.status.to_s + ' after ' + (Time.now - container.start_time).to_s + ' seconds.') else Thread.new do timeout = SELF_DESTROY_GRACE_PERIOD.to_i sleep(timeout) - new(execution_environment: execution_environment).kill_container(container, false) - Rails.logger.info('Force killing container in status ' + container.status + ' after ' + Time.now - container.start_time + ' seconds.') + container.docker_client.kill_container(container, false) + Rails.logger.info('Force killing container in status ' + container.status.to_s + ' after ' + (Time.now - container.start_time).to_s + ' seconds.') ensure # guarantee that the thread is releasing the DB connection after it is done ActiveRecord::Base.connection_pool.release_connection @@ -147,8 +148,8 @@ class DockerClient end container - rescue Docker::Error::NotFoundError => e - Rails.logger.error('create_container: Got Docker::Error::NotFoundError: ' + e.to_s) + rescue Docker::Error::NotFoundError => error + Rails.logger.error('create_container: Got Docker::Error::NotFoundError: ' + error.to_s) destroy_container(container) # (tries += 1) <= RETRY_COUNT ? retry : raise(error) end @@ -228,18 +229,26 @@ class DockerClient # Checks only if container assignment is not nil and not whether the container itself is still present. if container && !DockerContainerPool.config[:active] - clean_container_workspace(container) - container.stop.kill + container.kill container.port_bindings.values.each { |port| PortPool.release(port) } - container.delete(force: true, v: true) + begin + clean_container_workspace(container) + FileUtils.rmtree(local_workspace_path(container)) + rescue Errno::ENOENT, Errno::EACCES => error + Raven.capture_exception(error) + Rails.logger.error("clean_container_workspace: Got #{error.class.to_s}: #{error.to_s}") + end + + # Checks only if container assignment is not nil and not whether the container itself is still present. + container&.delete(force: true, v: true) elsif container DockerContainerPool.destroy_container(container) end - rescue Docker::Error::NotFoundError => e - Rails.logger.error('destroy_container: Rescued from Docker::Error::NotFoundError: ' + e.to_s) + rescue Docker::Error::NotFoundError => error + Rails.logger.error('destroy_container: Rescued from Docker::Error::NotFoundError: ' + error.to_s) Rails.logger.error('No further actions are done concerning that.') - rescue Docker::Error::ConflictError => e - Rails.logger.error('destroy_container: Rescued from Docker::Error::ConflictError: ' + e.to_s) + rescue Docker::Error::ConflictError => error + Rails.logger.error('destroy_container: Rescued from Docker::Error::ConflictError: ' + error.to_s) Rails.logger.error('No further actions are done concerning that.') end @@ -299,10 +308,12 @@ class DockerClient We need to start a second thread to kill the websocket connection, as it is impossible to determine whether further input is requested. """ + container.status = :executing @thread = Thread.new do - timeout = @execution_environment.permitted_execution_time.to_i # seconds + timeout = (@execution_environment.permitted_execution_time.to_i) # seconds sleep(timeout) - if container.status != :returned + container = ContainerPool.instance.translate(container.id) + if container && container.status != :available Rails.logger.info('Killing container after timeout of ' + timeout.to_s + ' seconds.') # send timeout to the tubesock socket # FIXME: 2nd thread to notify user. @@ -322,6 +333,8 @@ class DockerClient ensure ActiveRecord::Base.connection_pool.release_connection end + else + Rails.logger.info('Container' + container.to_s + ' already removed.') end ensure # guarantee that the thread is releasing the DB connection after it is done @@ -436,13 +449,13 @@ class DockerClient Rails.logger.debug('returning container ' + container.to_s) begin clean_container_workspace(container) - rescue Docker::Error::NotFoundError => e + rescue Docker::Error::NotFoundError => error # FIXME: Create new container? - Rails.logger.info('return_container: Rescued from Docker::Error::NotFoundError: ' + e.to_s) + Rails.logger.info('return_container: Rescued from Docker::Error::NotFoundError: ' + error.to_s) Rails.logger.info('Nothing is done here additionally. The container will be exchanged upon its next retrieval.') end DockerContainerPool.return_container(container, execution_environment) - container.status = :returned + container.status = :available end # private :return_container diff --git a/lib/docker_container_mixin.rb b/lib/docker_container_mixin.rb index d5881b38..6055b85d 100644 --- a/lib/docker_container_mixin.rb +++ b/lib/docker_container_mixin.rb @@ -3,6 +3,8 @@ module DockerContainerMixin attr_accessor :start_time attr_accessor :status attr_accessor :re_use + attr_accessor :execution_environment + attr_accessor :docker_client def binds json['HostConfig']['Binds'] diff --git a/spec/lib/docker_client_spec.rb b/spec/lib/docker_client_spec.rb index 4cc8ce26..4884a802 100644 --- a/spec/lib/docker_client_spec.rb +++ b/spec/lib/docker_client_spec.rb @@ -1,9 +1,12 @@ +# frozen_string_literal: true require 'rails_helper' require 'seeds_helper' -describe DockerClient, docker: true do - WORKSPACE_PATH = '/tmp/code_ocean_test' +# rubocop:disable RSpec/MultipleMemoizedHelpers +WORKSPACE_PATH = Rails.root.join('tmp', 'files', Rails.env, 'code_ocean_test') + +describe DockerClient, docker: true do let(:command) { 'whoami' } let(:docker_client) { described_class.new(execution_environment: FactoryBot.build(:java), user: FactoryBot.build(:admin)) } let(:execution_environment) { FactoryBot.build(:java) } @@ -11,12 +14,12 @@ describe DockerClient, docker: true do let(:submission) { FactoryBot.create(:submission) } let(:workspace_path) { WORKSPACE_PATH } - before(:all) do - FileUtils.mkdir_p(WORKSPACE_PATH) - end - - after(:all) do - FileUtils.rm_rf(WORKSPACE_PATH) + before do + allow(described_class).to receive(:container_creation_options).and_wrap_original do |original_method, *args, &block| + result = original_method.call(*args, &block) + result['NanoCPUs'] = 2 * 1_000_000_000 # CPU quota in units of 10^-9 CPUs. + result + end end describe '.check_availability!' do @@ -36,7 +39,7 @@ describe DockerClient, docker: true do end describe '.container_creation_options' do - let(:container_creation_options) { described_class.container_creation_options(execution_environment) } + let(:container_creation_options) { described_class.container_creation_options(execution_environment, workspace_path) } it 'specifies the Docker image' do expect(container_creation_options).to include('Image' => described_class.find_image_by_tag(execution_environment.docker_image).info['RepoTags'].first) @@ -53,17 +56,13 @@ describe DockerClient, docker: true do it 'specifies to open the standard input stream once' do expect(container_creation_options).to include('OpenStdin' => true, 'StdinOnce' => true) end - end - - describe '.container_start_options' do - let(:container_start_options) { described_class.container_start_options(execution_environment, '') } it 'specifies mapped directories' do - expect(container_start_options).to include('Binds' => kind_of(Array)) + expect(container_creation_options).to include('Binds' => kind_of(Array)) end it 'specifies mapped ports' do - expect(container_start_options).to include('PortBindings' => kind_of(Hash)) + expect(container_creation_options).to include('PortBindings' => kind_of(Hash)) end end @@ -82,14 +81,21 @@ describe DockerClient, docker: true do end it 'creates a container' do - expect(described_class).to receive(:container_creation_options).with(execution_environment).and_call_original + local_workspace_path = File.join(workspace_path, 'example').to_s + FileUtils.mkdir_p(workspace_path) + allow(described_class).to receive(:generate_local_workspace_path).and_return(local_workspace_path) + expect(described_class).to receive(:container_creation_options).with(execution_environment, local_workspace_path) + .and_wrap_original do |original_method, *args, &block| + result = original_method.call(*args, &block) + result['NanoCPUs'] = 2 * 1_000_000_000 # CPU quota in units of 10^-9 CPUs. + result + end expect(Docker::Container).to receive(:create).with(kind_of(Hash)).and_call_original create_container end it 'starts the container' do - expect(described_class).to receive(:container_start_options).with(execution_environment, kind_of(String)).and_call_original - expect_any_instance_of(Docker::Container).to receive(:start).with(kind_of(Hash)).and_call_original + expect_any_instance_of(Docker::Container).to receive(:start).and_call_original create_container end @@ -162,6 +168,7 @@ describe DockerClient, docker: true do it 'creates a file' do expect(described_class).to receive(:local_workspace_path).at_least(:once).and_return(workspace_path) + FileUtils.mkdir_p(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) @@ -172,12 +179,8 @@ describe DockerClient, docker: true do let(:container) { described_class.create_container(execution_environment) } after(:each) { described_class.destroy_container(container) } - it 'stops the container' do - expect(container).to receive(:stop).and_return(container) - end - it 'kills running processes' do - expect(container).to receive(:kill) + expect(container).to receive(:kill).and_return(container) end it 'releases allocated ports' do @@ -213,7 +216,7 @@ describe DockerClient, docker: true do let(:error) { Excon::Errors::SocketError.new(SocketError.new) } context 'when retries are left' do - let(:result) { { status: "ok", stdout: 42 } } + let(:result) { {status: "ok", stdout: 42} } before(:each) do expect(docker_client).to receive(:send_command).and_raise(error).and_return(result) @@ -279,7 +282,7 @@ describe DockerClient, docker: true do describe '.generate_local_workspace_path' do it 'includes the correct workspace root' do - expect(described_class.generate_local_workspace_path).to start_with(DockerClient::LOCAL_WORKSPACE_ROOT.to_s) + expect(described_class.generate_local_workspace_path.to_s).to start_with(DockerClient::LOCAL_WORKSPACE_ROOT.to_s) end it 'includes a UUID' do @@ -321,7 +324,7 @@ describe DockerClient, docker: true do describe '.mapped_directories' do it 'returns a unique mapping' do mapping = described_class.mapped_directories(workspace_path).first - expect(mapping).to start_with(workspace_path) + expect(mapping).to start_with(workspace_path.to_s) expect(mapping).to end_with(DockerClient::CONTAINER_WORKSPACE_PATH) end end @@ -371,7 +374,7 @@ describe DockerClient, docker: true do context 'when a timeout occurs' do before(:each) do expect(container).to receive(:exec).once.and_raise(Timeout::Error) - expect(container).to receive(:exec).twice.and_return([ [], [] ]) + expect(container).to receive(:exec).twice.and_return([[], []]) end it 'destroys the container asynchronously' do @@ -401,3 +404,5 @@ describe DockerClient, docker: true do end end end + +# rubocop:enable RSpec/MultipleMemoizedHelpers