Re: Review Request 52804: Adding an error message when the mesos_containerizer_path is not set correctly.

2016-10-18 Thread Zameer Manji 

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


Ship it!




Ship It!

- Zameer Manji


On Oct. 18, 2016, 11:22 a.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52804/
> ---
> 
> (Updated Oct. 18, 2016, 11:22 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1789
> https://issues.apache.org/jira/browse/AURORA-1789
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding an error message when the mesos_containerizer_path is not set 
> correctly.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> efa51e162de28c9d96fc898b87a1f63843928829 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 0bfe9e931f873c9f804f2ba4012e050e1f9fd24e 
> 
> Diff: https://reviews.apache.org/r/52804/diff/
> 
> 
> Testing
> ---
> 
> I verified the new error makes its way to the UI when 
> mesos_containerizer_path is set to a file that does not exist and also 
> verified the executors complates succesfully when mesos_containerizer_path is 
> set to the correct location.
> 
> Unit tests:
> ```
> ./pants test src/test/python/apache/thermos::
> ./pants test src/test/python/apache/aurora/executor::
> ```
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 52804: Adding an error message when the mesos_containerizer_path is not set correctly.

2016-10-18 Thread Aurora ReviewBot 

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


Ship it!




Master (ad77de1) 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 Oct. 18, 2016, 6:22 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52804/
> ---
> 
> (Updated Oct. 18, 2016, 6:22 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1789
> https://issues.apache.org/jira/browse/AURORA-1789
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding an error message when the mesos_containerizer_path is not set 
> correctly.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 
> efa51e162de28c9d96fc898b87a1f63843928829 
>   src/test/python/apache/aurora/executor/test_thermos_executor.py 
> 0bfe9e931f873c9f804f2ba4012e050e1f9fd24e 
> 
> Diff: https://reviews.apache.org/r/52804/diff/
> 
> 
> Testing
> ---
> 
> I verified the new error makes its way to the UI when 
> mesos_containerizer_path is set to a file that does not exist and also 
> verified the executors complates succesfully when mesos_containerizer_path is 
> set to the correct location.
> 
> Unit tests:
> ```
> ./pants test src/test/python/apache/thermos::
> ./pants test src/test/python/apache/aurora/executor::
> ```
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 52957: Handle the case where content type header is null.

2016-10-18 Thread Stephan Erb 

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


Ship it!




Thanks for picking this up!

- Stephan Erb


On Oct. 18, 2016, 12:38 a.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52957/
> ---
> 
> (Updated Oct. 18, 2016, 12:38 a.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Bugs: AURORA-1795
> https://issues.apache.org/jira/browse/AURORA-1795
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Per the 
> [documentation](http://docs.oracle.com/javaee/7/api/javax/servlet/ServletRequestWrapper.html#getContentType--)
>  `getContentType` can return `null`. This now handles that case gracefully.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java 
> 1634cb88ac09c778c5bb277ca902f4ca35dd6c9d 
> 
> Diff: https://reviews.apache.org/r/52957/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Justin Pinkul 

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

Review request for Aurora, Joshua Cohen and Zameer Manji.


Bugs: AURORA-1798
https://issues.apache.org/jira/browse/AURORA-1798


Repository: aurora


Description
---

The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
copied into the taskfs when using the Mesos containierizer with a Docker image.


Diffs
-

  src/main/python/apache/aurora/executor/common/sandbox.py 
4a0f3b5094940cc3dad34689a0b004fb33b348a0 
  src/test/python/apache/aurora/executor/common/test_sandbox.py 
41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 

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


Testing
---

Ran unit tests and launched a simple ping Aurora job with and without the 
change.


Thanks,

Justin Pinkul

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Justin Pinkul 


> On Oct. 18, 2016, 11:59 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 308-313
> > 
> >
> > Is this always necessary, or only necessary when filesystem isolation 
> > is used in conjunction with the network/cni isolator? If the latter, does 
> > it make more sense to just configure these as global mounts via the 
> > scheduler's `-global_container_mounts` command line flag, rather than doing 
> > this for everyone where it may not be necessary/desirable?
> > 
> > Alternately, I'm not super familiar w/ CNI, but is it possible to infer 
> > from the TaskInfo whether CNI is enabled (e.g. is NetworkInfo set 
> > somewhere)?

This is always nessisary when using a Docker image with the Mesos 
containierizer. The reason I brought up the network/cni isolator is that when 
you are running with a Docker image set as the rootfs this isolator will copy 
these files in, even if no CNI networks are defined. Since the current Thermos 
executor is using a volume instead of a rootfs this logic is completely 
bypassed. It makes sense for this change to be in the executor since it is 
required for DNS to function properly.

Pod support can be used as a longer term fix. This will allow us to set the 
rootfs for processes and the ownership of this logic can return to Mesos.


- Justin


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


On Oct. 18, 2016, 11:41 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 11:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 
> 
> Diff: https://reviews.apache.org/r/53003/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests and launched a simple ping Aurora job with and without the 
> change.
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Zameer Manji 

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


Ship it!




Ship It!

- Zameer Manji


On Oct. 18, 2016, 3:13 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 3:13 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 
> 
> Diff: https://reviews.apache.org/r/53003/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests and launched a simple ping Aurora job with and without the 
> change.
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Aurora ReviewBot 

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



Master (485504a) is red with this patch.
  ./build-support/jenkins/build.sh

virtualenv-15.0.2/virtualenv_embedded/activate_this.py
virtualenv-15.0.2/virtualenv_embedded/deactivate.bat
virtualenv-15.0.2/virtualenv_embedded/distutils-init.py
virtualenv-15.0.2/virtualenv_embedded/distutils.cfg
virtualenv-15.0.2/virtualenv_embedded/python-config
virtualenv-15.0.2/virtualenv_embedded/site.py
virtualenv-15.0.2/virtualenv_support/
virtualenv-15.0.2/virtualenv_support/__init__.py
virtualenv-15.0.2/virtualenv_support/argparse-1.4.0-py2.py3-none-any.whl
virtualenv-15.0.2/virtualenv_support/pip-8.1.2-py2.py3-none-any.whl
virtualenv-15.0.2/virtualenv_support/setuptools-21.2.1-py2.py3-none-any.whl
virtualenv-15.0.2/virtualenv_support/wheel-0.29.0-py2.py3-none-any.whl
+ touch virtualenv-15.0.2/BOOTSTRAPPED
+ popd
/home/jenkins/jenkins-slave/workspace/AuroraBot
+ exec /usr/bin/python2.7 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-15.0.2/virtualenv.py
 --no-download 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv
New python executable in 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv/bin/python2.7
Also creating executable in 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv/bin/python
Installing setuptools, pip, wheel...done.
Collecting isort==4.0.0
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv/local/lib/python2.7/site-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:318:
 SNIMissingWarning: An HTTPS request has been made, but the SNI (Subject Name 
Indication) extension to TLS is not available on this platform. This may cause 
the server to present an incorrect TLS certificate, which can cause validation 
failures. You can upgrade to a newer version of Python to solve this. For more 
information, see 
https://urllib3.readthedocs.org/en/latest/security.html#snimissingwarning.
  SNIMissingWarning
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv/local/lib/python2.7/site-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122:
 InsecurePlatformWarning: A true SSLContext object is not available. This 
prevents urllib3 from configuring SSL appropriately and may cause certain SSL 
connections to fail. You can upgrade to a newer version of Python to solve 
this. For more information, see 
https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading isort-4.0.0-py2.py3-none-any.whl
Installing collected packages: isort
Successfully installed isort-4.0.0
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/isort.venv/local/lib/python2.7/site-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122:
 InsecurePlatformWarning: A true SSLContext object is not available. This 
prevents urllib3 from configuring SSL appropriately and may cause certain SSL 
connections to fail. You can upgrade to a newer version of Python to solve 
this. For more information, see 
https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
ERROR: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_sandbox.py
 Imports are incorrectly sorted.
--- 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_sandbox.py:before
2016-10-18 22:20:22.082246
+++ 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_sandbox.py:after
 2016-10-18 22:29:54.250653
@@ -29,6 +29,7 @@
 )
 
 from gen.apache.aurora.api.ttypes import AssignedTask, Container, 
DockerContainer, TaskConfig
+
 
 def test_directory_sandbox():
   with temporary_dir() as d:


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

- Aurora ReviewBot


On Oct. 18, 2016, 10:13 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 10:13 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Joshua Cohen 

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




src/main/python/apache/aurora/executor/common/sandbox.py (lines 308 - 313)


Is this always necessary, or only necessary when filesystem isolation is 
used in conjunction with the network/cni isolator? If the latter, does it make 
more sense to just configure these as global mounts via the scheduler's 
`-global_container_mounts` command line flag, rather than doing this for 
everyone where it may not be necessary/desirable?

Alternately, I'm not super familiar w/ CNI, but is it possible to infer 
from the TaskInfo whether CNI is enabled (e.g. is NetworkInfo set somewhere)?


- Joshua Cohen


On Oct. 18, 2016, 11:41 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 11:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 
> 
> Diff: https://reviews.apache.org/r/53003/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests and launched a simple ping Aurora job with and without the 
> change.
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Joshua Cohen 


> On Oct. 18, 2016, 11:59 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/common/sandbox.py, lines 308-313
> > 
> >
> > Is this always necessary, or only necessary when filesystem isolation 
> > is used in conjunction with the network/cni isolator? If the latter, does 
> > it make more sense to just configure these as global mounts via the 
> > scheduler's `-global_container_mounts` command line flag, rather than doing 
> > this for everyone where it may not be necessary/desirable?
> > 
> > Alternately, I'm not super familiar w/ CNI, but is it possible to infer 
> > from the TaskInfo whether CNI is enabled (e.g. is NetworkInfo set 
> > somewhere)?
> 
> Justin Pinkul wrote:
> This is always nessisary when using a Docker image with the Mesos 
> containierizer. The reason I brought up the network/cni isolator is that when 
> you are running with a Docker image set as the rootfs this isolator will copy 
> these files in, even if no CNI networks are defined. Since the current 
> Thermos executor is using a volume instead of a rootfs this logic is 
> completely bypassed. It makes sense for this change to be in the executor 
> since it is required for DNS to function properly.
> 
> Pod support can be used as a longer term fix. This will allow us to set 
> the rootfs for processes and the ownership of this logic can return to Mesos.

Gotcha, thanks for clarifying. Given the above, does it make sense to only do 
this when the container is being launched with a Docker image?


- Joshua


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


On Oct. 18, 2016, 11:41 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 11:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 
> 
> Diff: https://reviews.apache.org/r/53003/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests and launched a simple ping Aurora job with and without the 
> change.
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Aurora ReviewBot 

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


Ship it!




Master (485504a) 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 Oct. 18, 2016, 11:41 p.m., Justin Pinkul wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53003/
> ---
> 
> (Updated Oct. 18, 2016, 11:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1798
> https://issues.apache.org/jira/browse/AURORA-1798
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
> copied into the taskfs when using the Mesos containierizer with a Docker 
> image.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/sandbox.py 
> 4a0f3b5094940cc3dad34689a0b004fb33b348a0 
>   src/test/python/apache/aurora/executor/common/test_sandbox.py 
> 41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 
> 
> Diff: https://reviews.apache.org/r/53003/diff/
> 
> 
> Testing
> ---
> 
> Ran unit tests and launched a simple ping Aurora job with and without the 
> change.
> 
> 
> Thanks,
> 
> Justin Pinkul
> 
>

Re: Review Request 53003: Adding logic to copy network files when using the Mesos containierizer with a Docker image.

2016-10-18 Thread Justin Pinkul 

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

(Updated Oct. 18, 2016, 11:41 p.m.)


Review request for Aurora, Joshua Cohen and Zameer Manji.


Bugs: AURORA-1798
https://issues.apache.org/jira/browse/AURORA-1798


Repository: aurora


Description
---

The networking files /etc/resolv.conf, /etc/hosts and /etc/hostname are now 
copied into the taskfs when using the Mesos containierizer with a Docker image.


Diffs (updated)
-

  src/main/python/apache/aurora/executor/common/sandbox.py 
4a0f3b5094940cc3dad34689a0b004fb33b348a0 
  src/test/python/apache/aurora/executor/common/test_sandbox.py 
41ee884a309e8cc8fedecf19cab2fbc397fcf1dc 

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


Testing
---

Ran unit tests and launched a simple ping Aurora job with and without the 
change.


Thanks,

Justin Pinkul