Re: Review Request 44871: Speed up Vagrant provisioning by using a custom base box.

[John Sirois]

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


Ship it!




2:44 for a destroy/up after the initial image pull from Montana.  Thats 
qualitatively much better than before!


The e2e ran green for me, albeit on my 2nd attempt after a trip home.  I don't 
have details for you on the initial failure except for the inability of the 
client to find the scheduler.
The connection difficulty somehow went away on the second run.

I'm away until Sunday so this LGTM as it stands, but you may want to replace me 
with another reviewer if there are substantial changes.


.gitignore (line 1)


Not related, but noticing this is a lie iiuc.  Should be 'Use /dist/ to 
ignore dist in project root.' (reference 'PATTERN FORMAT' section here: 
https://www.kernel.org/pub/software/scm/git/docs/gitignore.html).



examples/vagrant/provision-dev-cluster.sh (line 87)


Is this `/home/vagrant/aurora`?  As things stand almost all paths - save 
for `pushd aurora` in the `prepare_extras` function - are absolute; so this 
relative path reads risky / uncertain.


- John Sirois


On March 15, 2016, 5:59 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44871/
> ---
> 
> (Updated March 15, 2016, 5:59 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Relevant docs on packer can be found here: https://www.packer.io/docs/
> 
> To support this, i have created the [`apache-aurora` 
> org](https://atlas.hashicorp.com/apache-aurora).  If we move forward with 
> this patch, i will be looking to add other committers to the org and improve 
> the bus factor.
> 
> 
> Diffs
> -
> 
>   .gitignore 1af09a251b3f76c13813033d32aa7efba9aef304 
>   Vagrantfile 2d6c2ae598e80035840f7e517e161be266b581dd 
>   build-support/packer/README.md PRE-CREATION 
>   build-support/packer/aurora.json PRE-CREATION 
>   build-support/packer/build.sh PRE-CREATION 
>   examples/vagrant/provision-dev-cluster.sh 
> df33a30c1dcc0ea832899c849be05fe9deb47433 
> 
> Diff: https://reviews.apache.org/r/44871/diff/
> 
> 
> Testing
> ---
> 
> I can run `vagrant up`.  Currently looking into a possible configuration 
> issue in end-to-end tests, to be corrected before submitting.
> 
> Reviewers - you can do this as well by applying the patch.  You will observe 
> a long fetch on the first run (much like you would with the current repo if 
> you wiped your local vagrant boxes).  However, subsequent `vagrant destroy`, 
> `vagrant up` cycles should be relatively fast, about 1m30s for me.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 44871: Speed up Vagrant provisioning by using a custom base box.

[Aurora ReviewBot]

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


Ship it!




Master (6537581) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On March 15, 2016, 11:59 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44871/
> ---
> 
> (Updated March 15, 2016, 11:59 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Relevant docs on packer can be found here: https://www.packer.io/docs/
> 
> To support this, i have created the [`apache-aurora` 
> org](https://atlas.hashicorp.com/apache-aurora).  If we move forward with 
> this patch, i will be looking to add other committers to the org and improve 
> the bus factor.
> 
> 
> Diffs
> -
> 
>   .gitignore 1af09a251b3f76c13813033d32aa7efba9aef304 
>   Vagrantfile 2d6c2ae598e80035840f7e517e161be266b581dd 
>   build-support/packer/README.md PRE-CREATION 
>   build-support/packer/aurora.json PRE-CREATION 
>   build-support/packer/build.sh PRE-CREATION 
>   examples/vagrant/provision-dev-cluster.sh 
> df33a30c1dcc0ea832899c849be05fe9deb47433 
> 
> Diff: https://reviews.apache.org/r/44871/diff/
> 
> 
> Testing
> ---
> 
> I can run `vagrant up`.  Currently looking into a possible configuration 
> issue in end-to-end tests, to be corrected before submitting.
> 
> Reviewers - you can do this as well by applying the patch.  You will observe 
> a long fetch on the first run (much like you would with the current repo if 
> you wiped your local vagrant boxes).  However, subsequent `vagrant destroy`, 
> `vagrant up` cycles should be relatively fast, about 1m30s for me.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 44532: Allow overriding hostname before announcing

[Stephan Erb]

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


Fix it, then Ship it!




Patch looks good, thanks for your patience! Minor nitpick below.


src/test/python/apache/aurora/executor/common/test_announcer.py (line 341)


Those asserts a risky. If you have a typo like `assert_any_callz()` your 
test will pass. 

Could you try to write that one as an explicit assertion using `assert`? 
For example, this could be done via 
https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.mock_calls


- Stephan Erb


On March 15, 2016, 7:31 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44532/
> ---
> 
> (Updated March 15, 2016, 7:31 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Stephan Erb, and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow overriding hostname before announcing 
> (https://issues.apache.org/jira/browse/AURORA-1611)
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> f82858c528808d2a9e77bb56f16e897cfb5bbe73 
>   src/main/python/apache/aurora/executor/common/announcer.py 
> 34e36e0a59093468a8934f58bacb68512949347c 
>   src/test/python/apache/aurora/executor/common/test_announcer.py 
> f4032c7302f4733ab5670322b1905102c200f1c9 
> 
> Diff: https://reviews.apache.org/r/44532/diff/
> 
> 
> Testing
> ---
> 
> Tested on vagrant with a wrapper for the executor
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Re: Review Request 44745: Allow for a pure docker executor.

[John Sirois]

On March 13, 2016, 6:04 a.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 

Re: Review Request 44745: Allow for a pure docker executor.

[John Sirois]

On March 13, 2016, 6:04 a.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 

Re: Review Request 44745: Allow for a pure docker executor.

[Joshua Cohen]

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 

Re: Review Request 44745: Allow for a pure docker executor.

[John Sirois]

On March 13, 2016, 6:04 a.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 

Re: Review Request 44745: Allow for a pure docker executor.

[Joshua Cohen]

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 

Re: Review Request 44745: Allow for a pure docker executor.

[John Sirois]

On March 13, 2016, 6:04 a.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 

Re: Review Request 44827: Do not split the shell command string passed into shell health check script

[John Sirois]

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


Ship it!




Ship It!

- John Sirois


On March 14, 2016, 9:49 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44827/
> ---
> 
> (Updated March 14, 2016, 9:49 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1633
> https://issues.apache.org/jira/browse/AURORA-1633
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fixing bug where you could not pass in shell command into health checker with 
> environment variables. When environment variable assignement was passed in, 
> only that part would get executed and 0 would always get returned.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> bf63d936b3044dfa97a787938b643a52497f2d79 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 8d3a3e4e259e1ff699854aeb2434ac21f38e49ea 
> 
> Diff: https://reviews.apache.org/r/44827/diff/
> 
> 
> Testing
> ---
> 
> Unit tests + end_to_end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44745: Allow for a pure docker executor.

[Joshua Cohen]

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.

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, 

Re: Review Request 44532: Allow overriding hostname before announcing

[Kunal Thakar]

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

(Updated March 15, 2016, 6:31 p.m.)


Review request for Aurora, John Sirois, Stephan Erb, and Bill Farner.


Changes
---

Adding reviewers


Repository: aurora


Description (updated)
---

Allow overriding hostname before announcing 
(https://issues.apache.org/jira/browse/AURORA-1611)


Diffs
-

  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
f82858c528808d2a9e77bb56f16e897cfb5bbe73 
  src/main/python/apache/aurora/executor/common/announcer.py 
34e36e0a59093468a8934f58bacb68512949347c 
  src/test/python/apache/aurora/executor/common/test_announcer.py 
f4032c7302f4733ab5670322b1905102c200f1c9 

Diff: https://reviews.apache.org/r/44532/diff/


Testing (updated)
---

Tested on vagrant with a wrapper for the executor


Thanks,

Kunal Thakar

Re: Review Request 44532: Allow overriding hostname before announcing

[Kunal Thakar]

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

(Updated March 15, 2016, 6:29 p.m.)


Review request for Aurora.


Changes
---

Updated as per recommendations to allow overriding hostname without shelling 
out in the executor


Repository: aurora


Description
---

Allow overriding hostname before announcing


Diffs (updated)
-

  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
f82858c528808d2a9e77bb56f16e897cfb5bbe73 
  src/main/python/apache/aurora/executor/common/announcer.py 
34e36e0a59093468a8934f58bacb68512949347c 
  src/test/python/apache/aurora/executor/common/test_announcer.py 
f4032c7302f4733ab5670322b1905102c200f1c9 

Diff: https://reviews.apache.org/r/44532/diff/


Testing
---


Thanks,

Kunal Thakar

Re: Review Request 44745: Allow for a pure docker executor.

[Maxim Khutornenko]

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

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.


- Maxim


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