Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the latest django-docker #2172

Merged
merged 7 commits into from
Sep 21, 2017
Merged

Use the latest django-docker #2172

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8960555
Use the latest django-docker
mccalluc Sep 19, 2017
33c1e19
Constructor now requires an argument
mccalluc Sep 20, 2017
a077480
I get a different error locally... but it looks like vagrant provisio…
mccalluc Sep 20, 2017
57b7fda
Need another special case to create dir on travis
mccalluc Sep 20, 2017
f9b9bee
sudo mkdir and chown?
mccalluc Sep 20, 2017
363b515
comment because "vagrant" user could be confusing
mccalluc Sep 20, 2017
d46a91b
Increment version # to match latest
mccalluc Sep 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ env:
- DJANGO_SETTINGS_MODULE=config.settings.prod

before_script:
- mkdir /data # In vagrant, this is created by puppet; In production, this is an EBS mount.
- createuser --createdb --no-superuser --no-createrole vagrant
- createdb -O vagrant refinery
- cd refinery
Expand Down
8 changes: 8 additions & 0 deletions deployment/manifests/default.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@
require => Exec['create_guest'],
}

# Django-docker-engine needs a place for ephemeral data.
# In production, this is a separate EBS mount, so we don't need to create it locally.
file { '/data':
ensure => 'directory',
owner => $app_user,
group => $app_group
}

Copy link
Member

@hackdna hackdna Sep 20, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be combined into a separate module along with storage related items in aws.pp.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hackdna Are you saying create a storage.pp?
Would this be better suited for an issue outside of this pr?

Just seeing this comment: #2172 (comment) if you want to checkout this branch and make the storage.pp file that would be really helpful.

# See code in refinery-modules/refinery/...
include refinery
include refinery::apache2
Expand Down
3 changes: 2 additions & 1 deletion refinery/config/settings/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,8 @@ def get_setting(name, settings=local_settings, default=None):
# Time in seconds to wait before killing unused visualization
DJANGO_DOCKER_ENGINE_SECONDS_INACTIVE = 60 * 60
# Location of DjangoDockerEngine proxy logging
PROXY_LOG = '/tmp/django_docker_engine.log'
DJANGO_DOCKER_ENGINE_DATA_DIR = '/data/django-docker-engine-data'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/data doesn't exist in Vagrant VM and will go away on AWS once Refinery switches to storing data files in S3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where docker instances will have mounted volumes for their inputs and ephemeral storage needs. The path is accessed by docker-engine: If docker-engine is running on a separate instance, it will be a directory on that instance. If we can use ECS, then this may be moot. But it needs to be somewhere for now, and this is better than having it hard-coded to a path on the root volume.

Using /data also seemed better than mounting an entirely new EBS volume.

On vagrant, do you have a strong preference about whether we create a /data directory as part of setup, or have this setting depend on environment, or something else entirely?

Copy link
Member

@hackdna hackdna Sep 20, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then it's fine to use /data for Docker files for now on condition that only django_docker_engine would have access to them to avoid tight coupling with the rest of the application. Also, django_docker_engine should consider this storage ephemeral because this is going to be the case when it runs on a separate instance.

It's probably a good idea to add /data to Vagrant for consistency and move all dirs from $project_root (media, import, isa-tab and Solr index dirs) there.

Finally, it'd be great to consolidate all storage related Puppet resources into one module (similar to python.pp for example).

PROXY_LOG = '/data/django-docker-engine.log'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably best to follow standard Django logging practices instead of restricting logging to just a file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point: I think there was some reason initially why it didn't fit, but I can't recall it now, and I'm not sure the reason would be valid even if I could. refinery-platform/django_docker_engine#80


REFINERY_DEPLOYMENT_PLATFORM = "vagrant"

Expand Down
6 changes: 4 additions & 2 deletions refinery/tool_manager/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ def launch(self):
launched container's url
- <HttpResponseBadRequest>, <HttpServerError>
"""
client = DockerClientWrapper()
client = DockerClientWrapper(settings.DJANGO_DOCKER_ENGINE_DATA_DIR)
max_containers = settings.DJANGO_DOCKER_ENGINE_MAX_CONTAINERS
if len(client.list()) >= max_containers:
raise VisualizationToolError('Max containers')
Expand Down Expand Up @@ -1193,7 +1193,9 @@ def remove_tool_container(sender, instance, *args, **kwargs):
VisualizationTool's launch.
"""
try:
DockerClientWrapper().purge_by_label(instance.uuid)
DockerClientWrapper(
settings.DJANGO_DOCKER_ENGINE_DATA_DIR
).purge_by_label(instance.uuid)
except APIError as e:
logger.error("Couldn't purge container for Tool with UUID: %s %s",
instance.uuid, e)
2 changes: 1 addition & 1 deletion refinery/tool_manager/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
@task()
def django_docker_cleanup():
# TODO: Specify manager, if not default
client = DockerClientWrapper()
client = DockerClientWrapper(settings.DJANGO_DOCKER_ENGINE_DATA_DIR)
client.purge_inactive(settings.DJANGO_DOCKER_ENGINE_SECONDS_INACTIVE)
7 changes: 5 additions & 2 deletions refinery/tool_manager/tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2785,14 +2785,17 @@ def test_docker_cleanup(self):
settings.DJANGO_DOCKER_ENGINE_SECONDS_INACTIVE = wait_time

def assertions(tool):
DockerClientWrapper().lookup_container_url(tool.container_name)
client = DockerClientWrapper(
settings.DJANGO_DOCKER_ENGINE_DATA_DIR
)
client.lookup_container_url(tool.container_name)

time.sleep(wait_time * 2)
django_docker_cleanup()
time.sleep(wait_time * 2)

with self.assertRaises(NotFound):
DockerClientWrapper().lookup_container_url(tool.container_name)
client.lookup_container_url(tool.container_name)

self._start_visualization(
'hello_world.json',
Expand Down
1 change: 1 addition & 0 deletions refinery/tool_manager/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
please_wait_content = f.read()

url_patterns = Proxy(
settings.DJANGO_DOCKER_ENGINE_DATA_DIR,
logger=FileLogger(settings.PROXY_LOG),
please_wait_content=please_wait_content
).url_patterns()
Expand Down
4 changes: 3 additions & 1 deletion refinery/tool_manager/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,9 @@ def create_tool_definition(annotation_data):
logger.debug(
"Pulling Docker image: %s", tool_definition.image_name
)
DockerClientWrapper().pull(image_name, version=version)
DockerClientWrapper(
settings.DJANGO_DOCKER_ENGINE_DATA_DIR
).pull(image_name, version=version)

tool_definition.annotation = json.dumps(annotation)
tool_definition.save()
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,4 @@ watchdog==0.6.0
xmltodict==0.9.2
yolk==0.4.1

git+https://github.com/refinery-platform/django_docker_engine.git@v0.0.20
git+https://github.com/refinery-platform/django_docker_engine.git@v0.0.21