Sync DockerClient with DockerContainerPool

Fix failing specs after upgrade

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) %>

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) %>

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
+      rescue Errno::ENOENT, Errno::EACCES => error
-        Raven.capture_exception(e)
+        Raven.capture_exception(error)
-        Rails.logger.error("clean_container_workspace: Got #{e.class}: #{e}")
+        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
+      'Tty' => true,
-    }
-  end
-
-  def self.container_start_options(execution_environment, local_workspace_path)
-    {
       '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)
+        container.docker_client.kill_container(container, false)
-        Rails.logger.info('Killing container in status ' + container.status + ' after ' + (Time.now - container.start_time).to_s + ' seconds.')
+        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)
+          container.docker_client.kill_container(container, false)
-          Rails.logger.info('Force killing container in status ' + container.status + ' after ' + Time.now - container.start_time + ' seconds.')
+          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
+  rescue Docker::Error::NotFoundError => error
-    Rails.logger.error('create_container: Got Docker::Error::NotFoundError: ' + e.to_s)
+    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.kill
-      container.stop.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
+  rescue Docker::Error::NotFoundError => error
-    Rails.logger.error('destroy_container: Rescued from Docker::Error::NotFoundError: ' + e.to_s)
+    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
+  rescue Docker::Error::ConflictError => error
-    Rails.logger.error('destroy_container: Rescued from Docker::Error::ConflictError: ' + e.to_s)
+    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

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']

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
+# rubocop:disable RSpec/MultipleMemoizedHelpers
-  WORKSPACE_PATH = '/tmp/code_ocean_test'
 
+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
+  before do
-    FileUtils.mkdir_p(WORKSPACE_PATH)
+    allow(described_class).to receive(:container_creation_options).and_wrap_original do |original_method, *args, &block|
-  end
+      result = original_method.call(*args, &block)
-
+      result['NanoCPUs'] = 2 * 1_000_000_000 # CPU quota in units of 10^-9 CPUs.
-  after(:all) do
+      result
-    FileUtils.rm_rf(WORKSPACE_PATH)
+    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).and_call_original
-      expect_any_instance_of(Docker::Container).to receive(:start).with(kind_of(Hash)).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