Use the latest django-docker #2172

mccalluc · 2017-09-19T23:57:41Z

Use refinery-platform/django_docker_engine#79: django-docker temp dirs should be cleaned up, and we can configure them to be stored under /data.

Fix #2166, Fix #2160.

hackdna · 2017-09-20T01:20:28Z

refinery/config/settings/base.py

@@ -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'


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

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?

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).

hackdna · 2017-09-20T01:23:25Z

refinery/config/settings/base.py

@@ -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'
+PROXY_LOG = '/data/django-docker-engine.log'


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

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

…n did not pull the latest django-docker? Try travis

mccalluc · 2017-09-20T15:26:42Z

At least locally, /data does seem to be created, and the first test that fails on travis because of the missing directory passes:

(refinery-platform)vagrant@refinery:/vagrant/refinery$ ./manage.py test tool_manager.tests.ToolAPITests.test_bad_extra_directories_path_with_rollback
2017-09-20 11:20:05 DEBUG    pyvirtualdisplay:10 <module>() - version=0.2.1
Creating test database for alias 'default'...
2017-09-20 11:20:58 DEBUG    tool_manager.utils:158 create_tool_definition() - Pulling Docker image: nginx:1.10.3-alpine
2017-09-20 11:21:00 DEBUG    tool_manager.views:73 create() - Successfully created Tool: Test LIST Visualization bad extra_directories-launch
2017-09-20 11:21:00 ERROR    tool_manager.views:76 create() - Specified path: `not_an_absolute_path` is not absolute
.
----------------------------------------------------------------------
Ran 1 test in 3.110s

OK

vs travis:

FAIL: test_bad_extra_directories_path_with_rollback (tool_manager.tests.ToolAPITests)

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/refinery-platform/refinery-platform/refinery/tool_manager/tests.py", line 2182, in test_bad_extra_directories_path_with_rollback

    'Specified path: `not_an_absolute_path` is not absolute'

AssertionError: "could not create '/data': Permission denied" != 'Specified path: `not_an_absolute_path` is not absolute'

Check if something in the larger test is causing problems?
Check whether provisioning on Travis creates /data or not

hackdna · 2017-09-20T15:27:59Z

deployment/manifests/default.pp

+  owner  => $app_user,
+  group  => $app_group
+}
+


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

@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.

hackdna · 2017-09-20T15:36:42Z

I don't think Travis allows creating directories in /, so perhaps Docker ephemeral files should use system temp storage instead for simplicity. This means increasing the root volume size on AWS temporarily (to be reduced when Docker runs on a separate instance).

scottx611x

Looks good to me if Ilya's comments are taken into account, and we see tests pass.

Although, I'm not sure if all of these comment should be addressed immediately considering how long we've pushed back the release already, but they are definitely important.

codecov-io · 2017-09-20T16:20:54Z

Codecov Report

Merging #2172 into release-1.6.0 will increase coverage by <.01%.
The diff coverage is 100%.

@@                Coverage Diff                @@
##           release-1.6.0    #2172      +/-   ##
=================================================
+ Coverage          46.35%   46.36%   +<.01%     
=================================================
  Files                412      412              
  Lines              27977    27978       +1     
  Branches            1312     1312              
=================================================
+ Hits               12970    12971       +1     
  Misses             15007    15007

Impacted Files	Coverage Δ
refinery/tool_manager/urls.py	`100% <ø> (ø)`	⬆️
refinery/tool_manager/utils.py	`84.32% <100%> (ø)`	⬆️
refinery/tool_manager/tasks.py	`100% <100%> (ø)`	⬆️
refinery/tool_manager/tests.py	`99.84% <100%> (ø)`	⬆️
refinery/tool_manager/models.py	`95.74% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68e8d11...d46a91b. Read the comment docs.

hackdna · 2017-09-20T16:42:25Z

Actually, my suggestions should be quick to implement. I could help with the Puppet code if needed.

scottx611x · 2017-09-20T16:43:47Z

deployment/manifests/default.pp

+  owner  => $app_user,
+  group  => $app_group
+}
+


@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.

mccalluc · 2017-09-21T13:30:24Z

The proposed refactoring in #2175 isn't working right now. Talked with Scott and decided the best thing for where we are now is to merge this as is.

mccalluc requested a review from scottx611x September 19, 2017 23:57

Use the latest django-docker

8960555

hackdna reviewed Sep 20, 2017

View reviewed changes

mccalluc added 2 commits September 19, 2017 21:50

Constructor now requires an argument

33c1e19

I get a different error locally... but it looks like vagrant provisio…

a077480

…n did not pull the latest django-docker? Try travis

hackdna reviewed Sep 20, 2017

View reviewed changes

Need another special case to create dir on travis

57b7fda

scottx611x approved these changes Sep 20, 2017

View reviewed changes

mccalluc added 2 commits September 20, 2017 12:06

sudo mkdir and chown?

f9b9bee

comment because "vagrant" user could be confusing

363b515

scottx611x approved these changes Sep 20, 2017

View reviewed changes

Increment version # to match latest

d46a91b

mccalluc merged commit aef7ee9 into release-1.6.0 Sep 21, 2017

mccalluc deleted the mccalluc/django-docker-data-dir branch October 24, 2017 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the latest django-docker #2172

Use the latest django-docker #2172

mccalluc commented Sep 19, 2017 •

edited

hackdna Sep 20, 2017

mccalluc Sep 20, 2017

hackdna Sep 20, 2017 •

edited

hackdna Sep 20, 2017

mccalluc Sep 20, 2017

mccalluc commented Sep 20, 2017

hackdna Sep 20, 2017 •

edited

scottx611x Sep 20, 2017 •

edited

hackdna commented Sep 20, 2017 •

edited

scottx611x left a comment •

edited

codecov-io commented Sep 20, 2017 •

edited

hackdna commented Sep 20, 2017

scottx611x Sep 20, 2017 •

edited

mccalluc commented Sep 21, 2017

Use the latest django-docker #2172

Use the latest django-docker #2172

Conversation

mccalluc commented Sep 19, 2017 • edited

hackdna Sep 20, 2017

Choose a reason for hiding this comment

mccalluc Sep 20, 2017

Choose a reason for hiding this comment

hackdna Sep 20, 2017 • edited

Choose a reason for hiding this comment

hackdna Sep 20, 2017

Choose a reason for hiding this comment

mccalluc Sep 20, 2017

Choose a reason for hiding this comment

mccalluc commented Sep 20, 2017

hackdna Sep 20, 2017 • edited

Choose a reason for hiding this comment

scottx611x Sep 20, 2017 • edited

Choose a reason for hiding this comment

hackdna commented Sep 20, 2017 • edited

scottx611x left a comment • edited

Choose a reason for hiding this comment

codecov-io commented Sep 20, 2017 • edited

Codecov Report

hackdna commented Sep 20, 2017

scottx611x Sep 20, 2017 • edited

Choose a reason for hiding this comment

mccalluc commented Sep 21, 2017

mccalluc commented Sep 19, 2017 •

edited

hackdna Sep 20, 2017 •

edited

hackdna Sep 20, 2017 •

edited

scottx611x Sep 20, 2017 •

edited

hackdna commented Sep 20, 2017 •

edited

scottx611x left a comment •

edited

codecov-io commented Sep 20, 2017 •

edited

scottx611x Sep 20, 2017 •

edited