diff --git a/repo2docker/buildpacks/base.py b/repo2docker/buildpacks/base.py index 8616dbd08..a11dd1edd 100644 --- a/repo2docker/buildpacks/base.py +++ b/repo2docker/buildpacks/base.py @@ -523,13 +523,21 @@ def _filter_tar(tar): tar.close() tarf.seek(0) - limits = { - # Always disable memory swap for building, since mostly - # nothing good can come of that. - 'memswap': -1 - } + # If you work on this bit of code check the corresponding code in + # buildpacks/docker.py where it is duplicated + if not isinstance(memory_limit, int): + 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, diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index 91b584038..5f437b369 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -21,13 +21,21 @@ def render(self): 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 = { - # Always disable memory swap for building, since mostly - # nothing good can come of that. - 'memswap': -1 - } + # If you work on this bit of code check the corresponding code in + # buildpacks/base.py where it is duplicated + if not isinstance(memory_limit, int): + 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(), diff --git a/tests/memlimit/dockerfile/Dockerfile b/tests/memlimit/dockerfile/Dockerfile index 0cb027778..9513e5e44 100644 --- a/tests/memlimit/dockerfile/Dockerfile +++ b/tests/memlimit/dockerfile/Dockerfile @@ -1,4 +1,4 @@ -FROM ubuntu:artful +FROM ubuntu:bionic RUN apt-get update && apt-get install --yes python3 diff --git a/tests/unit/test_cache_from.py b/tests/unit/test_cache_from.py index be102a18e..270debec0 100644 --- a/tests/unit/test_cache_from.py +++ b/tests/unit/test_cache_from.py @@ -10,7 +10,6 @@ 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 - diff --git a/tests/unit/test_memlimit.py b/tests/unit/test_memlimit.py index f636da7ce..1d3f88ba1 100644 --- a/tests/unit/test_memlimit.py +++ b/tests/unit/test_memlimit.py @@ -10,9 +10,14 @@ 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)