diff --git a/readthedocs/doc_builder/environments.py b/readthedocs/doc_builder/environments.py index f4539fc1e7b..7123cf983cf 100644 --- a/readthedocs/doc_builder/environments.py +++ b/readthedocs/doc_builder/environments.py @@ -625,6 +625,8 @@ def __init__(self, *args, **kwargs): ) if self.config and self.config.build_image: self.container_image = self.config.build_image + if self.project.container_image: + self.container_image = self.project.container_image if self.project.container_mem_limit: self.container_mem_limit = self.project.container_mem_limit if self.project.container_time_limit: @@ -786,6 +788,13 @@ def get_container_host_config(self): mem_limit=self.container_mem_limit, ) + @property + def image_hash(self): + """Return the hash of the Docker image.""" + client = self.get_client() + image_metadata = client.inspect_image(self.container_image) + return image_metadata.get('Id') + @property def container_id(self): """Return id of container if it is valid.""" @@ -828,13 +837,13 @@ def update_build_from_container_state(self): def create_container(self): """Create docker container.""" client = self.get_client() - image = self.container_image - if self.project.container_image: - image = self.project.container_image try: - log.info('Creating Docker container: image=%s', image) + log.info( + 'Creating Docker container: image=%s', + self.container_image, + ) self.container = client.create_container( - image=image, + image=self.container_image, command=('/bin/sh -c "sleep {time}; exit {exit}"' .format(time=self.container_time_limit, exit=DOCKER_TIMEOUT_EXIT_CODE)), diff --git a/readthedocs/doc_builder/python_environments.py b/readthedocs/doc_builder/python_environments.py index 0d58551a4d6..5313c921697 100644 --- a/readthedocs/doc_builder/python_environments.py +++ b/readthedocs/doc_builder/python_environments.py @@ -109,12 +109,14 @@ def environment_json_path(self): @property def is_obsolete(self): """ - Determine if the Python version of the venv obsolete. + Determine if the environment is obsolete for different reasons. It checks the the data stored at ``readthedocs-environment.json`` and - compares it against the Python version in the project version to be - built and the Docker image used to create the venv against the one in - the project version config. + compares it with the one to be used. In particular: + + * the Python version (e.g. 2.7, 3, 3.6, etc) + * the Docker image name + * the Docker image hash :returns: ``True`` when it's obsolete and ``False`` otherwise @@ -129,15 +131,23 @@ def is_obsolete(self): try: with open(self.environment_json_path(), 'r') as fpath: environment_conf = json.load(fpath) - env_python_version = environment_conf['python']['version'] - env_build_image = environment_conf['build']['image'] except (IOError, TypeError, KeyError, ValueError): - log.error('Unable to read/parse readthedocs-environment.json file') - return False - - # TODO: remove getattr when https://github.com/rtfd/readthedocs.org/pull/3339 got merged - build_image = getattr(self.config, 'build_image', self.version.project.container_image) or DOCKER_IMAGE # noqa - + log.warning('Unable to read/parse readthedocs-environment.json file') + # We remove the JSON file here to avoid cycling over time with a + # corrupted file. + os.remove(self.environment_json_path()) + return True + + env_python = environment_conf.get('python', {}) + env_build = environment_conf.get('build', {}) + + # By defaulting non-existent options to ``None`` we force a wipe since + # we don't know how the environment was created + env_python_version = env_python.get('version', None) + env_build_image = env_build.get('image', None) + env_build_hash = env_build.get('hash', None) + + build_image = self.config.build_image or DOCKER_IMAGE # If the user define the Python version just as a major version # (e.g. ``2`` or ``3``) we won't know exactly which exact version was # used to create the venv but we can still compare it against the new @@ -145,19 +155,19 @@ def is_obsolete(self): return any([ env_python_version != self.config.python_full_version, env_build_image != build_image, + env_build_hash != self.build_env.image_hash, ]) def save_environment_json(self): """Save on disk Python and build image versions used to create the venv.""" - # TODO: remove getattr when https://github.com/rtfd/readthedocs.org/pull/3339 got merged - build_image = getattr(self.config, 'build_image', self.version.project.container_image) or DOCKER_IMAGE # noqa - + build_image = self.config.build_image or DOCKER_IMAGE data = { 'python': { 'version': self.config.python_full_version, }, 'build': { 'image': build_image, + 'hash': self.build_env.image_hash, }, } diff --git a/readthedocs/rtd_tests/tests/test_doc_building.py b/readthedocs/rtd_tests/tests/test_doc_building.py index b5229b07409..9b0c82253ef 100644 --- a/readthedocs/rtd_tests/tests/test_doc_building.py +++ b/readthedocs/rtd_tests/tests/test_doc_building.py @@ -9,7 +9,9 @@ absolute_import, division, print_function, unicode_literals) import os.path +import json import re +import tempfile import uuid from builtins import str @@ -837,14 +839,54 @@ def test_command_oom_kill(self): u'Command killed due to excessive memory consumption\n') - - -class TestAutoWipeEnvironment(TestCase): +class AutoWipeEnvironmentBase(object): fixtures = ['test_data'] + build_env_class = None def setUp(self): self.pip = Project.objects.get(slug='pip') self.version = self.pip.versions.get(slug='0.8') + self.build_env = self.build_env_class( + project=self.pip, + version=self.version, + build={'id': DUMMY_BUILD_ID}, + ) + + def test_save_environment_json(self): + config_data = { + 'build': { + 'image': '2.0', + }, + 'python': { + 'version': 2.7, + }, + } + yaml_config = create_load(config_data)()[0] + config = ConfigWrapper(version=self.version, yaml_config=yaml_config) + + python_env = Virtualenv( + version=self.version, + build_env=self.build_env, + config=config, + ) + + with patch( + 'readthedocs.doc_builder.python_environments.PythonEnvironment.environment_json_path', + return_value=tempfile.mktemp(suffix='envjson'), + ): + python_env.save_environment_json() + json_data = json.load(open(python_env.environment_json_path())) + + expected_data = { + 'build': { + 'image': 'readthedocs/build:2.0', + 'hash': 'a1b2c3', + }, + 'python': { + 'version': 2.7, + }, + } + self.assertDictEqual(json_data, expected_data) def test_is_obsolete_without_env_json_file(self): yaml_config = create_load()()[0] @@ -854,7 +896,7 @@ def test_is_obsolete_without_env_json_file(self): exists.return_value = False python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) @@ -868,7 +910,7 @@ def test_is_obsolete_with_invalid_env_json_file(self): exists.return_value = True python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) @@ -888,10 +930,10 @@ def test_is_obsolete_with_json_different_python_version(self): python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) - env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}' + env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 3.5}}' # noqa with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa exists.return_value = True self.assertTrue(python_env.is_obsolete) @@ -910,10 +952,10 @@ def test_is_obsolete_with_json_different_build_image(self): python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) - env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}' + env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 2.7}}' # noqa with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa exists.return_value = True self.assertTrue(python_env.is_obsolete) @@ -936,10 +978,10 @@ def test_is_obsolete_with_project_different_build_image(self): python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) - env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}' + env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 2.7}}' # noqa with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa exists.return_value = True self.assertTrue(python_env.is_obsolete) @@ -958,10 +1000,82 @@ def test_is_obsolete_with_json_same_data_as_version(self): python_env = Virtualenv( version=self.version, - build_env=None, + build_env=self.build_env, config=config, ) - env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}' + env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 3.5}}' # noqa with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa exists.return_value = True self.assertFalse(python_env.is_obsolete) + + def test_is_obsolete_with_json_different_build_hash(self): + config_data = { + 'build': { + 'image': '2.0', + }, + 'python': { + 'version': 2.7, + }, + } + yaml_config = create_load(config_data)()[0] + config = ConfigWrapper(version=self.version, yaml_config=yaml_config) + + # Set container_image manually + self.pip.container_image = 'readthedocs/build:2.0' + self.pip.save() + + python_env = Virtualenv( + version=self.version, + build_env=self.build_env, + config=config, + ) + env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "foo"}, "python": {"version": 2.7}}' # noqa + with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa + exists.return_value = True + self.assertTrue(python_env.is_obsolete) + + def test_is_obsolete_with_json_missing_build_hash(self): + config_data = { + 'build': { + 'image': '2.0', + 'hash': 'a1b2c3', + }, + 'python': { + 'version': 2.7, + }, + } + yaml_config = create_load(config_data)()[0] + config = ConfigWrapper(version=self.version, yaml_config=yaml_config) + + # Set container_image manually + self.pip.container_image = 'readthedocs/build:2.0' + self.pip.save() + + python_env = Virtualenv( + version=self.version, + build_env=self.build_env, + config=config, + ) + env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}' # noqa + with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa + exists.return_value = True + self.assertTrue(python_env.is_obsolete) + + +@patch( + 'readthedocs.doc_builder.environments.DockerBuildEnvironment.image_hash', + PropertyMock(return_value='a1b2c3'), +) +class AutoWipeDockerBuildEnvironmentTest(AutoWipeEnvironmentBase, TestCase): + build_env_class = DockerBuildEnvironment + + +@pytest.mark.xfail( + reason='PythonEnvironment needs to be refactored to do not rely on DockerBuildEnvironment', +) +@patch( + 'readthedocs.doc_builder.environments.DockerBuildEnvironment.image_hash', + PropertyMock(return_value='a1b2c3'), +) +class AutoWipeLocalBuildEnvironmentTest(AutoWipeEnvironmentBase, TestCase): + build_env_class = LocalBuildEnvironment