Skip to content

Commit

Permalink
Revert "Save docker image hash to consider when auto wiping the envir…
Browse files Browse the repository at this point in the history 
…onment (#3793)"

This reverts commit 7ac7dfc.
  • Loading branch information
@agjohnson
agjohnson committed Mar 26, 2018
1 parent 5a39385 commit d8f603c
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 119 deletions.
19 changes: 5 additions & 14 deletions readthedocs/doc_builder/environments.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,6 @@ 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:
Expand Down Expand Up @@ -788,13 +786,6 @@ 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."""
Expand Down Expand Up @@ -837,13 +828,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',
self.container_image,
)
log.info('Creating Docker container: image=%s', image)
self.container = client.create_container(
image=self.container_image,
image=image,
command=('/bin/sh -c "sleep {time}; exit {exit}"'
.format(time=self.container_time_limit,
exit=DOCKER_TIMEOUT_EXIT_CODE)),
Expand Down
11 changes: 6 additions & 5 deletions readthedocs/doc_builder/python_environments.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,32 +131,33 @@ def is_obsolete(self):
environment_conf = json.load(fpath)
env_python_version = environment_conf['python']['version']
env_build_image = environment_conf['build']['image']
env_build_hash = environment_conf['build']['hash']
except (IOError, TypeError, KeyError, ValueError):
log.error('Unable to read/parse readthedocs-environment.json file')
return False

build_image = self.config.build_image or DOCKER_IMAGE
# 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

# 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
# one coming from the project version config.
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."""
build_image = self.config.build_image or DOCKER_IMAGE
# 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

data = {
'python': {
'version': self.config.python_full_version,
},
'build': {
'image': build_image,
'hash': self.build_env.image_hash,
},
}

Expand Down
113 changes: 13 additions & 100 deletions readthedocs/rtd_tests/tests/test_doc_building.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
absolute_import, division, print_function, unicode_literals)

import os.path
import json
import re
import tempfile
import uuid
from builtins import str

Expand Down Expand Up @@ -839,54 +837,14 @@ def test_command_oom_kill(self):
u'Command killed due to excessive memory consumption\n')


class AutoWipeEnvironmentBase(object):


class TestAutoWipeEnvironment(TestCase):
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]
Expand All @@ -896,7 +854,7 @@ def test_is_obsolete_without_env_json_file(self):
exists.return_value = False
python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)

Expand All @@ -910,7 +868,7 @@ def test_is_obsolete_with_invalid_env_json_file(self):
exists.return_value = True
python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)

Expand All @@ -930,10 +888,10 @@ def test_is_obsolete_with_json_different_python_version(self):

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 3.5}}' # noqa
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}'
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)
Expand All @@ -952,10 +910,10 @@ def test_is_obsolete_with_json_different_build_image(self):

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 2.7}}' # noqa
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}'
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)
Expand All @@ -978,10 +936,10 @@ def test_is_obsolete_with_project_different_build_image(self):

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 2.7}}' # noqa
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 2.7}}'
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)
Expand All @@ -1000,55 +958,10 @@ def test_is_obsolete_with_json_same_data_as_version(self):

python_env = Virtualenv(
version=self.version,
build_env=self.build_env,
build_env=None,
config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0", "hash": "a1b2c3"}, "python": {"version": 3.5}}' # noqa
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}'
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)


@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

0 comments on commit d8f603c

Please sign in to comment.