aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 28920: Add support for docker containers to aurora
Date Wed, 14 Jan 2015 01:03:26 GMT

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


Mostly style and configuration ergonomics remaining.  Once these are addressed my final pass
before giving a ship will be assessing for complete test coverage.


docs/configuration-reference.md
<https://reviews.apache.org/r/28920/#comment112074>

    Flag as experimental somehow, here and in other new doc portions.



examples/vagrant/upstart/aurora-scheduler.conf
<https://reviews.apache.org/r/28920/#comment112075>

    remove extra newline



examples/vagrant/upstart/mesos-slave.conf
<https://reviews.apache.org/r/28920/#comment112076>

    Is this afflicted by MESOS-2215?  I'm worried (read: certain) people will copy/paste the
example here, and likely be set up for failure.  I don't have an answer, curious what your
thoughts are.



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
<https://reviews.apache.org/r/28920/#comment112077>

    The Args system handles multi-valued args, comma-separated.  Change to `Arg<List<String>>`
and this will Just Work.



src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
<https://reviews.apache.org/r/28920/#comment112107>

    Should this instead be a list of allowed `ContainerType`s?  If we want this to support
arbitrary container types, seems like a good litmus test for success is absense of the string
'docker' from code and comments.  Of course, there may be special cases that dictate otherwise.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112088>

    Please grep for 'wrapper' to make sure that code, comments, and docs refer to this as
executor resources.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112084>

    This one is not optional.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112083>

    Feel free to omit "A[n] optional" from all of these, as the type self-documents.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112078>

    blank line between wrapped method signature and body.



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112079>

    space-pad the colon:
    
        for (String resource : splitResources) {



src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java
<https://reviews.apache.org/r/28920/#comment112089>

    shell=true is default, is there a reason you want to explicitly set it?  I'm not against
it, just want to be on the same page.



src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
<https://reviews.apache.org/r/28920/#comment112115>

    I would love to see another check for whether the scheduler is configured to allow docker
mounts.  In fact, i'm not sure if `allow_docker_mounts` (or its replacement, based on another
comment) should be plumbed anywhere else from here.
    
    Reasoning here is that only the cluster administrator should need to know what's on the
slave command line.



src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
<https://reviews.apache.org/r/28920/#comment112090>

    please break these out and give a separate error message



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112092>

    kick the first arg onto the next line, and use +4 indent for all (so a method rename doesn't
move these out of alignment).



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112091>

    blank line above



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112095>

    Can you include a comment explaining why this is necessary?  Will be good for posterity.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112100>

    Prefer inserting line breaks to keep the longest continuous statement unbroken.  In this
case, i would break after the `=` and put the rest on one line.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112096>

    blank line before



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112097>

    blank line before



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112098>

    blank line before



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
<https://reviews.apache.org/r/28920/#comment112101>

    line break after the paren, +4 indent for each arg after


- Bill Farner


On Jan. 13, 2015, 1:11 a.m., Steve Niemitz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28920/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2015, 1:11 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 aurora file is in examples/jobs/docker.
> 
> 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.
> 
> More information is in subsequent review request comments.
> 
> 
> Diffs
> -----
> 
>   Vagrantfile f8b7db8eebdc6a10989de3bc9a2c3e89ce17f5fc 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 08ba1cdf88b712de22c26c04443079282db59ef9

>   config/legacy_untested_classes.txt 33c1d6eb4ea02e01b7002c2c2bae5a3858c8b0e5 
>   docs/configuration-reference.md f3cb257206a194b82fd2045dc20456ee832dbcea 
>   docs/deploying-aurora-scheduler.md 711ae7eda07c2c1735601c265c06a88c1862cce7 
>   examples/jobs/docker/hello_docker.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 1e31f21998d02fd69ce0db88e6adb3d32cff67fd 
>   examples/vagrant/provision-dev-cluster.sh 7af4b52a6876268a97630279221bb98d9b04efad

>   examples/vagrant/upstart/aurora-scheduler.conf 788ec254270bca074dae91829c7f4fccdc8f8bb0

>   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/GcExecutorLauncher.java 5226e3d1b303b1773a057078f2911c5ec2aa97f5

>   src/main/java/org/apache/aurora/scheduler/base/CommandUtil.java d885b224ec5a1d529347d84e03ba98ab6734a126

>   src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 01b03508afac37b5a8f0ec5c3da1460695e1ef59

>   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 f7d8977e42aa56188799400bf8e12a6886fb4a8f

>   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/config/schema_base.py f9143cc1b83143d6147f59d90c79435d055d0518

>   src/main/python/apache/thermos/core/runner.py 8aac6b50c66080abbb5308b367e9f74c487f42e3

>   src/main/resources/scheduler/assets/configSummary.html 28878908b0c9381e366b71a3135dfc28c542a890

>   src/main/resources/scheduler/assets/js/services.js b744f375411e09b7f776e4a05ee5961227143439

>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 5e54364a49a208bd5f19b9649633dc8feca591e9

>   src/test/java/org/apache/aurora/scheduler/base/CommandUtilTest.java 876e173ccbac04e4a06a245648c7c6af15eaaa92

>   src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
dc2cb37adf32df0a6e4c7ee2ba776ba9f1f3c2f8 
>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message