> On Dec. 13, 2014, 1:59 a.m., Jay Buffington wrote:
> > I haven't had time to complete this review, but I wanted to give you what I 
> > have so far.  This is all fantastic and I really appreciate you doing this! 
> >  I'm excited to start using this implementation.
> > 
> > You should update configuration-reference.md and 
> > deploying-aurora-scheduler.md in the docs dir to explain these changes.  
> > You should state minimum mesos slave version as well explain minimum docker 
> > version required on the slaves.
> > 
> > You've made a bunch of changes to the executor and the runner to make them 
> > docker aware. I'd like those two components to not know about docker and I 
> > think we could really simplify this patch if we remove all that.  I think 
> > these changes are wholly unnecessary.  We should chat on IRC or Facetime 
> > about this.
> > 
> > I don't understand how users are managed in this world.  Docker images have 
> > their own /etc/passwd, so it seems to me that the unix user the mesos task 
> > runs as (same as aurora role) needs to exist inside that /etc/passwd.  If 
> > that's not the case I'm confused and want to understand what user (username 
> > and id) the task does run as inside and outside the container.
> > 
> > Allowing the aurora user to set volumes is a security nightmare.  before 
> > docker we're using unix permissions to control security.  We basically 
> > said: your aurora role is your unix user and that's all the permissions you 
> > get on a host.  The docker daemon runs as a priviledged user, so now you 
> > can tell docker to bind mount in files from all over the system that your 
> > unix user didn't previously have permissions to read.   So at a minimum, we 
> > should have a flag to disable the volumes feature with a big red warning 
> > flag in docs telling people what security issues they're signing up for 
> > whenever they enable it.
> 
> Jay Buffington wrote:
>     Oh, I forgot to mention the docs need to explain what requirements are on 
> the docker image to run the docker executor in it.  Perhaps that's just 
> python 2.7.  If that's the case, we should consider bind mounting (aka docker 
> volumes) in (statically compiled?) py2.7 executable.

Great feedback all around here, thanks for spending the time to go over this.  
Im on a plane into SF right now, I'll follow up with you this afternoon.


> On Dec. 13, 2014, 1:59 a.m., Jay Buffington wrote:
> > examples/jobs/docker/thermos_executor.sh, line 5
> > <https://reviews.apache.org/r/28920/diff/2/?file=789359#file789359line5>
> >
> >     I don't think setting LIBPROCESS_IP is necessary here and I don't think 
> > you set the second arg to this anywhere in the script.  Perhaps this 
> > unintentionally snuck into this patch?

Good catch, that wasn't supposed to be in there.  I think we can get rid of 
this whole example script eventually.


> On Dec. 13, 2014, 1:59 a.m., Jay Buffington wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 280
> > <https://reviews.apache.org/r/28920/diff/2/?file=789365#file789365line280>
> >
> >     info is too verbose here.

I'm just going to delete these instead, they don't add much value.


> On Dec. 13, 2014, 1:59 a.m., Jay Buffington wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 308
> > <https://reviews.apache.org/r/28920/diff/2/?file=789365#file789365line308>
> >
> >     To make this log message useful to an operations person reading it, it 
> > should explain which job this message pertains to.

As above, just removing these.


> On Dec. 13, 2014, 1:59 a.m., Jay Buffington wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 285
> > <https://reviews.apache.org/r/28920/diff/2/?file=789365#file789365line285>
> >
> >     This is confusing to the user and I think unnecessary.  It seems like 
> > the user should be able to use whatever wrapper script they want (why does 
> > it have to end in .sh?)  
> >     
> >     The requirements of what the wrapper script must do should be explained 
> > in the deploying-aurora doc

I actually changed this a few days ago based on internal feedback such that you 
pass both an executor (and or ) wrapper into the scheduler on the command line. 
 It'll be included in this patch set.


- Steve


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28920/#review65000
-----------------------------------------------------------


On Dec. 11, 2014, 6:16 a.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2014, 6:16 a.m.)
> 
> 
> Review request for Aurora, Jay Buffington, Kevin Sweeney, and Bill Farner.
> 
> 
> Bugs: AURORA-633
>     https://issues.apache.org/jira/browse/AURORA-633
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This change adds support for launching docker containers through aurora.  
> These changes are based off of the discussion in 
> https://issues.apache.org/jira/browse/AURORA-633
> 
> As of now, a special thermos_executor.sh script is needed to launch the 
> executor inside docker containers.  A sample script is in 
> examples/jobs/docker, as well as an example aurora file.
> 
> In addition, mesos-slave must be run with `--containerizers=docker,mesos`, 
> the example upstart config in examples/vagrant/upstart has been updated to 
> reflect this.
> 
> The thermos root path defaults to /var/run/thermos, however if a different 
> path is used, it must be passed to the scheduler via 
> `--thermos_observer_root=<some path>`
> 
> 
> Diffs
> -----
> 
>   Vagrantfile f8b7db8eebdc6a10989de3bc9a2c3e89ce17f5fc 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 5665c69cd7b49c3fd7345074c9f16a3b224496ab 
>   api/src/main/thrift/org/apache/thermos/thermos_internal.thrift 
> 2c449a491bc5a8ac858ea6487e4cef0591f36f66 
>   examples/jobs/docker/Dockerfile PRE-CREATION 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/jobs/docker/hello_docker.py PRE-CREATION 
>   examples/jobs/docker/thermos_executor.sh PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 69983d0140b76c6869cd04e55d760f3e3a1e4262 
>   examples/vagrant/upstart/mesos-slave.conf 
> 512ce7ecf34042ed68dda55efb2dd0415f8469db 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 72c7545e7f16549f6a9ccb5fb74a06f154a7ea94 
>   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
> ead9d28100673440168a32d114ecaa15874978a6 
>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java 
> d885b224ec5a1d529347d84e03ba98ab6734a126 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 5bf283062c9d119ff91ed45da8b236e36d0fc9aa 
>   src/main/python/apache/aurora/config/thrift.py 
> ba94ac3c0cbaf3c91eb1a1d86a244ed6fa3b649c 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> 636b23d30a897b557eb8c3f8733c90b23cb807ef 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 9df9b4b79c0c7d29c5088409bf15c0d32a621df0 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> f47a32b3fefb4a89940b1ddc473b8316ac00df12 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> 5e4bd65537d186459003c0b9434f1b769e04f448 
>   src/main/python/apache/thermos/bin/thermos_runner.py 
> 647de2771f301b17de33d8b45198c211d2e84367 
>   src/main/python/apache/thermos/config/schema_base.py 
> f9143cc1b83143d6147f59d90c79435d055d0518 
>   src/main/python/apache/thermos/core/runner.py 
> 8aac6b50c66080abbb5308b367e9f74c487f42e3 
>   src/main/python/apache/thermos/observer/task_observer.py 
> cd528dcca3f5a330359cf38005f3a1a0329a4886 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 5e54364a49a208bd5f19b9649633dc8feca591e9 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> ddcb511d108220ab5e4efcf3496458f7ab4a20c2 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 503e62f4cac872b14f6985b5bccc3e4dfcf81789 
> 
> Diff: https://reviews.apache.org/r/28920/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Reply via email to