From 5adc4b257e05ae79cdb6643a0ad3217e4aefaf6e Mon Sep 17 00:00:00 2001 From: Tim Head Date: Tue, 7 May 2019 19:12:34 +0200 Subject: [PATCH 1/6] Update base image used for memory limit checks --- tests/memlimit/dockerfile/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/memlimit/dockerfile/Dockerfile b/tests/memlimit/dockerfile/Dockerfile index 0cb02777..9513e5e4 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 From ef2860371a82b804dba9f5b878437ba71689717d Mon Sep 17 00:00:00 2001 From: Tim Head Date: Wed, 8 May 2019 08:11:38 +0200 Subject: [PATCH 2/6] Fix memory limits set for container image builds --- repo2docker/buildpacks/base.py | 16 ++++++++++------ repo2docker/buildpacks/docker.py | 16 ++++++++++------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/repo2docker/buildpacks/base.py b/repo2docker/buildpacks/base.py index 8616dbd0..dedd915f 100644 --- a/repo2docker/buildpacks/base.py +++ b/repo2docker/buildpacks/base.py @@ -523,13 +523,17 @@ class BuildPack: 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 + limits = {} if memory_limit: - limits['memory'] = memory_limit + # We'd like to always disable swap but all we can do is set the + # total amount. This means we only limit it when the caller set + # a memory limit + limits = { + 'memory': memory_limit, + 'memswap': memory_limit + 1 + } build_kwargs = dict( fileobj=tarf, diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index 91b58403..a9da371d 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -21,13 +21,17 @@ 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 = { - # 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 + limits = {} if memory_limit: - limits['memory'] = memory_limit + # We'd like to always disable swap but all we can do is set the + # total amount. This means we onnly limit it when the caller set + # a memory limit + limits = { + 'memory': memory_limit, + 'memswap': memory_limit + 1 + } build_kwargs = dict( path=os.getcwd(), From 9f44075839c175d71213c717e6427c70241132f5 Mon Sep 17 00:00:00 2001 From: Tim Head Date: Wed, 8 May 2019 13:20:10 +0200 Subject: [PATCH 3/6] Fix cache-from tests to pass memory limit as integer --- tests/unit/test_cache_from.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_cache_from.py b/tests/unit/test_cache_from.py index be102a18..270debec 100644 --- a/tests/unit/test_cache_from.py +++ b/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 - From 032baf6d0452733fd6b09acb2e73adb07abd9f7f Mon Sep 17 00:00:00 2001 From: Tim Head Date: Wed, 8 May 2019 13:20:31 +0200 Subject: [PATCH 4/6] Add check to `build()` for memory limit type --- repo2docker/buildpacks/base.py | 3 +++ repo2docker/buildpacks/docker.py | 3 +++ tests/unit/test_memlimit.py | 19 +++++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/repo2docker/buildpacks/base.py b/repo2docker/buildpacks/base.py index dedd915f..ba50356b 100644 --- a/repo2docker/buildpacks/base.py +++ b/repo2docker/buildpacks/base.py @@ -525,6 +525,9 @@ class BuildPack: # 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: # We'd like to always disable swap but all we can do is set the diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index a9da371d..208807ed 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -23,6 +23,9 @@ class DockerBuildPack(BuildPack): """Build a Docker image based on the Dockerfile in the source repo.""" # 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: # We'd like to always disable swap but all we can do is set the diff --git a/tests/unit/test_memlimit.py b/tests/unit/test_memlimit.py index f636da7c..1d3f88ba 100644 --- a/tests/unit/test_memlimit.py +++ b/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) From 2eb4781c2e954f0e19ee3c2ae2c6e170b4c919b2 Mon Sep 17 00:00:00 2001 From: Tim Head Date: Wed, 8 May 2019 18:22:11 +0200 Subject: [PATCH 5/6] Swap limit doesn't have to be bigger than memory limit --- repo2docker/buildpacks/base.py | 2 +- repo2docker/buildpacks/docker.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/repo2docker/buildpacks/base.py b/repo2docker/buildpacks/base.py index ba50356b..e6cb3916 100644 --- a/repo2docker/buildpacks/base.py +++ b/repo2docker/buildpacks/base.py @@ -535,7 +535,7 @@ class BuildPack: # a memory limit limits = { 'memory': memory_limit, - 'memswap': memory_limit + 1 + 'memswap': memory_limit } build_kwargs = dict( diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index 208807ed..3f3c09a4 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -33,7 +33,7 @@ class DockerBuildPack(BuildPack): # a memory limit limits = { 'memory': memory_limit, - 'memswap': memory_limit + 1 + 'memswap': memory_limit, } build_kwargs = dict( From 74fc378230f8bc138bf0f01661914d2951008093 Mon Sep 17 00:00:00 2001 From: yuvipanda Date: Wed, 8 May 2019 09:44:58 -0700 Subject: [PATCH 6/6] Clarify comment around memswap & memory --- repo2docker/buildpacks/base.py | 7 ++++--- repo2docker/buildpacks/docker.py | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/repo2docker/buildpacks/base.py b/repo2docker/buildpacks/base.py index e6cb3916..a11dd1ed 100644 --- a/repo2docker/buildpacks/base.py +++ b/repo2docker/buildpacks/base.py @@ -530,9 +530,10 @@ class BuildPack: "integer but is '{}'".format(type(memory_limit))) limits = {} if memory_limit: - # We'd like to always disable swap but all we can do is set the - # total amount. This means we only limit it when the caller set - # a 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 diff --git a/repo2docker/buildpacks/docker.py b/repo2docker/buildpacks/docker.py index 3f3c09a4..5f437b36 100644 --- a/repo2docker/buildpacks/docker.py +++ b/repo2docker/buildpacks/docker.py @@ -28,9 +28,10 @@ class DockerBuildPack(BuildPack): "integer but is '{}'".format(type(memory_limit))) limits = {} if memory_limit: - # We'd like to always disable swap but all we can do is set the - # total amount. This means we onnly limit it when the caller set - # a 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,