On March 13, 2016, 12:04 p.m., John Sirois wrote:
> > While your patch is rather easy, I am not sure it is the best way to move 
> > forward. It feels like it is crossing streams with 
> > https://issues.apache.org/jira/browse/AURORA-1288. Putting some thought 
> > into this might be helpful in the long run.
> 
> Bill Farner wrote:
>     FWIW i don't think it complicates or even diverges from that ticket.  In 
> my opinion it's yet to be seen whether it's feasible to use the same client 
> for a custom executor (at least, without a decent amount of modularization 
> work).  At the very least, that effort has lost momentum and we shouldn't 
> block progress for it.
> 
> Stephan Erb wrote:
>     I mostly brought it up because the ticket also repeatedly mentions the 
> default Mesos command executor. Supporting this one does not sound to 
> different from supporting Docker without Thermos. It would also need similar 
> logic at the UI layer to allow use the Mesos sanbox UI instead of the Thermos 
> one.
>     
>     I agree that we should not block progress here. I justed wanted to make 
> sure we are not rushing things (i.e., there isn't even a jira ticket right 
> now).
> 
> Maxim Khutornenko wrote:
>     +1 to Stephan's concerns. The schema changes in this patch don't 
> necessarily convey enough meaning to paint a clear picture of where this 
> effort leads us. FWICT, nothing in the Task aside from resources is 
> applicable to the Docker case and it feels quite hacky to onboard a new 
> executor case this way.
>     
>     > In my opinion it's yet to be seen whether it's feasible to use the same 
> client for a custom executor (at least, without a decent amount of 
> modularization work).
>     
>     Bill, would you mind clarifying what this means? Are you expecting this 
> to be a purely experimental (POC) effort or is there a solid production 
> quality future here? If it's the former, would it be more appropriate to have 
> this effort baked in a private branch to avoid possibly unnecessary code 
> churn and review cycles?
> 
> Bill Farner wrote:
>     | it feels quite hacky to onboard a new executor case this way
>     
>     Suggestions solicited!  Just please don't forget that the intent is to 
> offer real, immediate value - the docker support in Aurora today is quite 
> crippled, and this will address the biggest shortcomings (entrypoints, and 
> zero required deps in images).
>     
>     | FWICT, nothing in the Task aside from resources is applicable to the 
> Docker case
>     
>     This is a good point.  Perhaps we should create a separate struct and 
> field in `Job` for this case?
>     
>     | Bill, would you mind clarifying what this means?
>     
>     What i mean is that the client, DSL, and executor all have relatively 
> high coupling.  Adding custom executor support in the client will require 
> non-trivial effort to break that coupling.  I would like to avoid blocking 
> this feature on that goal.
>     
>     | Are you expecting this to be a purely experimental (POC) effort or is 
> there a solid production quality future here?
>     
>     That is very much dependent on the underlying support in mesos.  Today, i 
> see it as the best support for docker containers in mesos.  It's been 
> available for some time, and the work here is entirely plumbing to enable it 
> in Aurora.
>     
>     | would it be more appropriate to have this effort baked in a private 
> branch to avoid possibly unnecessary code churn and review cycles?
>     
>     I don't foresee enough churn to warrant that.
> 
> John Sirois wrote:
>     Noting that I'm backing off this change until sentiment settles one way 
> or the other.  If it settles in-favor I'll address both Stephan & Joshua's 
> feedback at that point.
> 
> Stephan Erb wrote:
>     I have to backoff out of the discussion here, as I don't have the 
> necessary cycles to participate. A couple of closing notes from my side:
>     
>     * I agree with Maxim that giving an empty process list a special meaning 
> feels kind of like a hack.
>     * I probably wouldn't have complained about this if it was that way from 
> the beginning...
>     * Docker support is still considered experimental, so no decision is cast 
> in stone. We can change stuff without to much hassle.
>     * It is great that you are improving the current docker support (even 
> though I am a fanboy of the upcoming unified container support :-)
> 
> Maxim Khutornenko wrote:
>     Thanks for explaning Bill. I am fine continuing this effort given the 
> above.
>     
>     > This is a good point.  Perhaps we should create a separate struct and 
> field in Job for this case?
>     
>     I don't have bandwidth to think about the alternatives at the moment but 
> your suggestion about plugging it higher in the chain (e.g. Job struct) 
> sounds logical.
> 
> Joshua Cohen wrote:
>     Could we just add name and resources to the Container struct (if we even 
> need name? I think it's only used by thermos today, but I haven't double 
> checked this)? I was going to suggest something similar, but was trying to 
> avoid exploding the change.
> 
> John Sirois wrote:
>     I've gone down this path trying to do things right and given pystachio is 
> FAPP frozen code atm:
>     ```python
>     class BaseJob(Struct):
>       name          = Required(String)
>       role          = Required(String)
>       contact       = String
>       cluster       = Required(String)
>       environment   = Required(String)
>       instances     = Default(Integer, 1)
>       announce      = Announcer
>       tier          = String
>     
>       cron_schedule = String
>       cron_collision_policy = Default(String, "KILL_EXISTING")
>     
>       update_config = Default(UpdateConfig, UpdateConfig())
>     
>       constraints                = Map(String, String)
>       service                    = Default(Boolean, False)
>       max_task_failures          = Default(Integer, 1)
>       production                 = Default(Boolean, False)
>       priority                   = Default(Integer, 0)
>     
>       enable_hooks = Default(Boolean, False)  # enable client API hooks; from 
> env python-list 'hooks'
>     
>     
>     def inherit(sup):
>       def assert_struct_type(item):
>         if not isinstance(item, TypeMetaclass) or item.type_factory() != 
> 'Struct':
>           raise TypeError('Cannot decorate {} with @inherit, it is not a 
> Struct subtype'.format(item))
>       assert_struct_type(sup)
>       def wrap(cls):
>         assert_struct_type(cls)
>         merged_typemap = sup.TYPEMAP.copy()
>         merged_typemap.update(cls.TYPEMAP)
>         cls.TYPEMAP = merged_typemap
>         return cls
>       return wrap
>     
>     
>     @inherit(BaseJob)
>     class DockerDaemonJob(Struct):
>       container = Required(Docker)
>       resources = Resources
>     
>     
>     @inherit(BaseJob)
>     class MesosJob(Struct):
>       name = Default(String, '{{task.name}}')
>       task = Required(Task)
>     
>       health_check_config        = Default(HealthCheckConfig, 
> HealthCheckConfig())
>       # TODO(wickman) Make Default(Any, LifecycleConfig()) once pystachio #17 
> is addressed.
>       lifecycle                  = Default(LifecycleConfig, 
> DefaultLifecycleConfig)
>       task_links                 = Map(String, String)  # Unsupported.  See 
> AURORA-739
>     
>       container = Container
>     ```
>     
>     Smirk, Smile, Gak or other?
> 
> Joshua Cohen wrote:
>     I’m neutral-ish on the inherits decorator (If we could get wickman to 
> merge it into pystachio itself rather than defining it in our schema, I’d be 
> more ok with it).
>     
>     I’m not sure how I feel about resources being defined on the Job rather 
> than the Container though, what's the reasoning behind that?
>     
>     Also, a little bit iffy on the fact that this would, in essence, require 
> users to do...
>     
>     
>         jobs = [
>           DockerDaemonJob(..., service=True)
>         ]
> 
> John Sirois wrote:
>     > ... If we could get wickman to merge it into pystachio itself rather 
> than defining it in our schema, I’d be more ok with it
>     
>     FWICT we can't.  I owe a dev@ thread today to propose adopting the code 
> like we did for java commons so we can get changes in.
>     
>     > I’m not sure how I feel about resources being defined on the Job rather 
> than the Container though, what's the reasoning behind that?
>     
>     I was trying to avoid stuffing yet more contextually relevant fields in 
> Structs.  If resources were moved to Container or Docker then we'd have the 
> possibility of conflicting resources for existing MesosJobs (ThermosJob is a 
> better name)that use a container (docker images with embedded thermos 
> executors).  This would require logic of the very sort folks were not happy 
> with above - ie using a subset of fields for certain cases.
>     
>     > Also, a little bit iffy on the fact that this would, in essence, 
> require users to do...
>     
>     I assume you're focused on the `service=True` inconvenience, in which 
> case a DockerDaemonService could be introduced that follows the pattern of 
> `Job`, `Service`.
>     If that's not what you were worried about, but instead the verbose name 
> (`DockerDaemonJob`), I'm open to suggestions, but thought it would be good to 
> clearly point out the job type will execute under docker and not thermos.

> I was trying to avoid stuffing yet more contextually relevant fields in 
> Structs.  If resources were moved to Container or Docker then we'd have the 
> possibility of conflicting resources for existing MesosJobs (ThermosJob is a 
> better name)that use a container (docker images with embedded thermos 
> executors).  This would require logic of the very sort folks were not happy 
> with above - ie using a subset of fields for certain cases.

Could we take a similar approach to what you're doing w/ MesosJob vs 
DockerDaemonJob? E.g.

    class BaseJob(Struct):
      # as per above
    
    @inherit(BaseJob):
    class MesosJob(Struct):
      task = Required(task)
      # etc.

    @inherit(Docker)
    class DockerDaemon(Struct):
      resources = Required(Resources)

Which would, in theory, allow job config like:

    jobs=[
      BaseJob(
        cluster = 'devcluster',
        role = getpass.getuser(),
        environment = 'test',
        contact = '{{role}}@localhost',
        instances = 1,
        container = Container(
          docker = DockerDaemon(
            resources = Resources(cpu=0.4, ram=32*MB, disk=64*MB),
            image = 'http_example',
            parameters = [
              Parameter(name = 'env', value = 'HTTP_PORT=8888'),
              Parameter(name = 'expose', value = '8888'),
              Parameter(name = 'publish', value = '8888:8888/tcp'),
            ],
          ),
        ),
      )
    ]

Is that super gross? I think it's definitely making our complex configuration 
language even moreso, and things will only get more complicated when I shortly 
introduce `Image` to the `Job` struct as a peer of `Container`. Maybe it's 
solvable with documentation though?

(On a side note, I agree that as part of these changes, it would be nice to 
call it `ThermosJob` rather than `MesosJob`, but I think that's probably too 
much to ask of users who may already be using `MesosJob` for what's supposed to 
be a small tweak to allow an experimental feature.)

> I assume you're focused on the service=True inconvenience, in which case a 
> DockerDaemonService could be introduced that follows the pattern of Job, 
> Service.

Yes, that's what I mean, and adding `DockerDaemonService` would work. That 
said, this is starting to feel like it's snowballing. I don't want to send us 
down the path of refactoring our core configuration constructs for the sake of 
this change. At the same time, these are things that are easier to get right 
from the start than they are to change later, so I'm ok with exploring our 
options.


- Joshua


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


On March 13, 2016, 2:48 a.m., John Sirois wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44745/
> -----------------------------------------------------------
> 
> (Updated March 13, 2016, 2:48 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This allows for a job config with no processes defined IFF there is also
> a Docker container defined.  In this case it is assumed that the process
> to run is the Docker container's ENTRYPOINT via the Mesos Docker
> containerizer.
> 
>  src/main/python/apache/aurora/config/__init__.py    | 26 
> +++++++++++++++++++++++---
>  src/main/python/apache/aurora/config/thrift.py      |  9 +++++----
>  src/test/python/apache/aurora/client/test_config.py | 41 
> +++++++++++++++++++++++++++++++++++------
>  src/test/python/apache/aurora/config/test_thrift.py |  5 +++++
>  4 files changed, 68 insertions(+), 13 deletions(-)
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/config/__init__.py 
> 65923be1cb8b88139b8eab0ac5b75428972d3cb1 
>   src/main/python/apache/aurora/config/thrift.py 
> be0cd68674a71bd4baadf276f40a4bc0223ce4be 
>   src/main/python/apache/thermos/config/schema_base.py 
> a6768e67189b0560afef844d6b269bed8ada5f2f 
>   src/test/python/apache/aurora/client/test_config.py 
> b1a3c1865819899ef19173be0f861783a2631d0a 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 88292d3c4423c0555088a0adaee3c0e62ed0567e 
> 
> Diff: https://reviews.apache.org/r/44745/diff/
> 
> 
> Testing
> -------
> 
> Locally green `./build-support/jenkins/build.sh`
> 
> I also patched in https://reviews.apache.org/r/44685/ which this change
> depends on and was able to run scheduler with
> `-allow_docker_parameters -require_docker_use_executor` and successfully
> run this job:
> ```
> import getpass
> 
> jobs=[
>   Service(
>     cluster = 'devcluster',
>     role = getpass.getuser(),
>     environment = 'test',
>     name = 'http_example_docker_executor',
>     contact = '{{role}}@localhost',
>     instances = 1,
>     task = Task(
>       name = 'http_docker_example',
>       resources = Resources(cpu=0.4, ram=32*MB, disk=64*MB),
>       processes = []
>     ),
>     container = Container(
>       docker = Docker(
>         image = 'http_example',
>         parameters = [
>           Parameter(name = 'env', value = 'HTTP_PORT=8888'),
>           Parameter(name = 'expose', value = '8888'),
>           Parameter(name = 'publish', value = '8888:8888/tcp'),
>         ],
>       ),
>     ),
>   )
> ]
> ```
> 
> Using the image created with
> `docker build -t http_example src/test/sh/org/apache/aurora/e2e` from:
> ```
> FROM python:2.7
> 
> # mesos.native requires libcurl-nss to initialize MesosExecutorDriver
> RUN apt-get update && apt-get -y install libcurl4-nss-dev
> 
> COPY http_example.py /tmp/
> ENTRYPOINT python /tmp/http_example.py $HTTP_PORT
> ```
> 
> I could connect to http://aurora.local:8888 and get `Hello!` back.
> 
> 
> Thanks,
> 
> John Sirois
> 
>

Reply via email to