[MRG] Update base image used for memory limit checks (#677)

[MRG] Update base image used for memory limit checks

repo2docker/buildpacks/base.py

@@ -523,13 +523,21 @@ class BuildPack:
         tar.close()
         tarf.seek(0)
 
-        limits = {
+        # If you work on this bit of code check the corresponding code in
-            # Always disable memory swap for building, since mostly
+        # buildpacks/docker.py where it is duplicated
-            # nothing good can come of that.
+        if not isinstance(memory_limit, int):
-            'memswap': -1
+            raise ValueError("The memory limit has to be specified as an"
-        }
+                             "integer but is '{}'".format(type(memory_limit)))
+        limits = {}
         if memory_limit:
-            limits['memory'] = memory_limit
+            # We want to always disable swap. Docker expects `memswap` to
+            # be total allowable memory, *including* swap - while `memory`
+            # points to non-swap memory. We set both values to the same so
+            # we use no swap.
+            limits = {
+                'memory': memory_limit,
+                'memswap': memory_limit
+            }
 
         build_kwargs = dict(
             fileobj=tarf,

repo2docker/buildpacks/docker.py

@@ -21,13 +21,21 @@ class DockerBuildPack(BuildPack):
 
     def build(self, client, image_spec, memory_limit, build_args, cache_from, extra_build_kwargs):
         """Build a Docker image based on the Dockerfile in the source repo."""
-        limits = {
+        # If you work on this bit of code check the corresponding code in
-            # Always disable memory swap for building, since mostly
+        # buildpacks/base.py where it is duplicated
-            # nothing good can come of that.
+        if not isinstance(memory_limit, int):
-            'memswap': -1
+            raise ValueError("The memory limit has to be specified as an"
-        }
+                             "integer but is '{}'".format(type(memory_limit)))
+        limits = {}
         if memory_limit:
-            limits['memory'] = memory_limit
+            # We want to always disable swap. Docker expects `memswap` to
+            # be total allowable memory, *including* swap - while `memory`
+            # points to non-swap memory. We set both values to the same so
+            # we use no swap.
+            limits = {
+                'memory': memory_limit,
+                'memswap': memory_limit,
+            }
 
         build_kwargs = dict(
             path=os.getcwd(),

tests/memlimit/dockerfile/Dockerfile

@@ -1,4 +1,4 @@
-FROM ubuntu:artful
+FROM ubuntu:bionic
 
 RUN apt-get update && apt-get install --yes python3
 

tests/unit/test_cache_from.py

@@ -10,7 +10,6 @@ from repo2docker.buildpacks import BaseImage, DockerBuildPack, LegacyBinderDocke
 
 
 def test_cache_from_base(tmpdir):
-    FakeDockerClient = MagicMock()
     cache_from = [
         'image-1:latest'
     ]
@@ -21,16 +20,14 @@ def test_cache_from_base(tmpdir):
 
     # Test base image build pack
     tmpdir.chdir()
-    for line in BaseImage().build(fake_client, 'image-2', '1Gi', {}, cache_from, extra_build_kwargs):
+    for line in BaseImage().build(fake_client, 'image-2', 100, {}, cache_from, extra_build_kwargs):
         assert line == fake_log_value
     called_args, called_kwargs = fake_client.build.call_args
     assert 'cache_from' in called_kwargs
     assert called_kwargs['cache_from'] == cache_from
 
 
-
 def test_cache_from_docker(tmpdir):
-    FakeDockerClient = MagicMock()
     cache_from = [
         'image-1:latest'
     ]
@@ -44,7 +41,7 @@ def test_cache_from_docker(tmpdir):
     with tmpdir.join("Dockerfile").open('w') as f:
         f.write('FROM scratch\n')
 
-    for line in DockerBuildPack().build(fake_client, 'image-2', '1Gi', {}, cache_from, extra_build_kwargs):
+    for line in DockerBuildPack().build(fake_client, 'image-2', 100, {}, cache_from, extra_build_kwargs):
         assert line == fake_log_value
     called_args, called_kwargs = fake_client.build.call_args
     assert 'cache_from' in called_kwargs
@@ -52,7 +49,6 @@ def test_cache_from_docker(tmpdir):
 
 
 def test_cache_from_legacy(tmpdir):
-    FakeDockerClient = MagicMock()
     cache_from = [
         'image-1:latest'
     ]
@@ -65,9 +61,8 @@ def test_cache_from_legacy(tmpdir):
     with tmpdir.join("Dockerfile").open('w') as f:
         f.write('FROM andrewosh/binder-base\n')
 
-    for line in LegacyBinderDockerBuildPack().build(fake_client, 'image-2', '1Gi', {}, cache_from, extra_build_kwargs):
+    for line in LegacyBinderDockerBuildPack().build(fake_client, 'image-2', 100, {}, cache_from, extra_build_kwargs):
         assert line == fake_log_value
     called_args, called_kwargs = fake_client.build.call_args
     assert 'cache_from' in called_kwargs
     assert called_kwargs['cache_from'] == cache_from
-

tests/unit/test_memlimit.py

@@ -10,9 +10,14 @@ import os
 import shutil
 import time
 
+from unittest.mock import MagicMock
+
+import docker
+
 import pytest
 
 from repo2docker.app import Repo2Docker
+from repo2docker.buildpacks import BaseImage, DockerBuildPack
 
 
 basedir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
@@ -82,3 +87,17 @@ def test_memlimit_same_postbuild():
             file_contents.append(f.read())
     # Make sure they're all the same
     assert len(set(file_contents)) == 1
+
+
+@pytest.mark.parametrize('BuildPack', [BaseImage, DockerBuildPack])
+def test_memlimit_argument_type(BuildPack):
+    # check that an exception is raised when the memory limit isn't an int
+    fake_log_value = {'stream': 'fake'}
+    fake_client = MagicMock(spec=docker.APIClient)
+    fake_client.build.return_value = iter([fake_log_value])
+
+    with pytest.raises(ValueError) as exc_info:
+        for line in BuildPack().build(fake_client, 'image-2', "10Gi", {}, [], {}):
+            pass
+
+        assert "The memory limit has to be specified as an" in str(exc_info.value)