Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============2572055002952149116=="
MIME-Version: 1.0
Subject: Re: Review Request 31338: Added a command line flag,
 -global_container_mounts,
 to allow mounting paths from the slaves into the (docker) containers they
 run.
From: "Bill Farner" <wfarner@apache.org>
To: "Bill Farner" <wfarner@apache.org>, "Jay Buffington" <me@jaybuff.com>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Steve Niemitz" <steve@tellapart.com>, "Stephan Erb" <apache@stephanerb.eu>,
 "Joshua Cohen" <jcohen@twopensource.com>
Date: Tue, 24 Feb 2015 20:45:45 -0000
Message-ID: <20150224204545.4175.59936@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Bill Farner" <noreply@reviews.apache.org>
References: <20150224060147.4175.88266@reviews.apache.org>
In-Reply-To: <20150224060147.4175.88266@reviews.apache.org>
Reply-To: "Bill Farner" <wfarner@apache.org>

--===============2572055002952149116==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote:
> >
> 
> Steve Niemitz wrote:
>     I'm not a big fan of how the parsing works here either.  I was thinking about this last night, I think I have a better plan here.  Lemme know what you think.
>     
>     I already want to add volume support per-job at some point, so I propose adding the needed thrift objects to api.thrift (Volume, Mode enum), and converting the command line into those objects in ExecutorSettings.  That would then let me reuse the volume-adding code in MesosTaskFactory for normal volumes in the future.
>     
>     Re: command line, I chose the format there because it's the same as the docker command line.  What about making a new parser to parse that into the above mentioned thrift objects?
> 
> Stephan Erb wrote:
>     How about using the same format and argument name as Mesos' `--default_container_info`:
>     
>         {
>           "type": "MESOS",
>           "volumes": [
>             {
>               "host_path": "./.private/tmp",
>               "container_path": "/tmp",
>               "mode": "RW"
>             }
>           ]
>         }
>         
>     See https://mesos.apache.org/documentation/latest/configuration/
> 
> Steve Niemitz wrote:
>     JSON on the command line is an escaping nightmare IMO, and very verbose to specify 2 (or 3) strings for this use case.
> 
> Bill Farner wrote:
>     JSON on the command line also makes me uneasy, but i don't think we have precedent for command line entities with this many knobs (3 are obvious so far).
>     
>     How about 2 approaches as straw men:
>     
>     `-volume_mounts_read_only` and `-volume_mounts_read_write`, both key-value mappings
>     
>     `-volume_mounts_config`, path to a JSON file with entities similar to what Stephan sketched above
>     
>     The first approach has the upside that it fits with the status quo, but it cannot be easily extended if we ever want more attributes.  The second would be the first config file we use, but is more future-proof.
> 
> Joshua Cohen wrote:
>     I think a single arg is easiest to reason about. I think I'm ok with Steve's follow up suggestion to create the thrift types that will be needed for per-job mounts now and use them to aid in arg parsing? That said, I'm not sure we should enforce the docker mount format since in theory these mounts can/will apply to other containers where they may not make as much sense.
> 
> Steve Niemitz wrote:
>     I think it's simple enough it'd make sense for any container system.  host:container:mode is pretty generic.  I'm also not as big of a fan about multiple args, and a default of RO if not specified seems reasonable to me.

Works for me, but -1 to defaulting.  I much prefer explicit over implicit, and this way it's easy to see how to turn a read-only mount into read-write.


- Bill


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


On Feb. 24, 2015, 2:07 a.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31338/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2015, 2:07 a.m.)
> 
> 
> Review request for Aurora, Jay Buffington and Bill Farner.
> 
> 
> Bugs: AURORA-1107
>     https://issues.apache.org/jira/browse/AURORA-1107
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.
> 
> This is the first portion of allowing per-job mounts, however, I wanted to get this out first since more people want it.  I'll implement per-job mounts in a future review.
> 
> 
> Diffs
> -----
> 
>   docs/deploying-aurora-scheduler.md d1123359961fd59ddb8c1a07f80f293bdd46019f 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java bacfbfeb237ecddf82f58679e05be012c5214e61 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 24b61c1e4f615295acf28d904588e1512972d3f4 
>   src/main/java/org/apache/aurora/scheduler/mesos/ExecutorSettings.java baacb71403d55c5b90fc11cb2a23f552a32e8ba5 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 5340d651b298ec8aa079e73d6d2f652fdf876293 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 6575b7d420f17ec68d6e2a83e7b380f684577d4f 
>   src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 444d6d3fdaf86eb84612f846eaa326eb75c49898 
>   src/test/java/org/apache/aurora/scheduler/mesos/TaskExecutors.java efe62ceb502ead88a2f0cd6d09a76664e465d9bc 
> 
> Diff: https://reviews.apache.org/r/31338/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>


--===============2572055002952149116==--