You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2016/03/30 23:58:52 UTC
Review Request 45506: Execute shell-based health checks as the task
user.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
Diff: https://reviews.apache.org/r/45506/diff/
Testing
-------
I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
Thanks,
Bill Farner
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126207
-----------------------------------------------------------
Master (55a2422) is red with this patch.
./build-support/jenkins/build.sh
virtualenv-14.0.6/virtualenv_embedded/activate.sh
virtualenv-14.0.6/virtualenv_embedded/activate_this.py
virtualenv-14.0.6/virtualenv_embedded/deactivate.bat
virtualenv-14.0.6/virtualenv_embedded/distutils-init.py
virtualenv-14.0.6/virtualenv_embedded/distutils.cfg
virtualenv-14.0.6/virtualenv_embedded/python-config
virtualenv-14.0.6/virtualenv_embedded/site.py
virtualenv-14.0.6/virtualenv_support/
virtualenv-14.0.6/virtualenv_support/__init__.py
virtualenv-14.0.6/virtualenv_support/argparse-1.4.0-py2.py3-none-any.whl
virtualenv-14.0.6/virtualenv_support/pip-8.0.2-py2.py3-none-any.whl
virtualenv-14.0.6/virtualenv_support/setuptools-20.0-py2.py3-none-any.whl
virtualenv-14.0.6/virtualenv_support/wheel-0.29.0-py2.py3-none-any.whl
+ touch virtualenv-14.0.6/BOOTSTRAPPED
+ popd
~/jenkins-slave/workspace/AuroraBot
+ exec /usr/bin/python2.7 /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/virtualenv-14.0.6/virtualenv.py /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:315: 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. 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:120: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. For more information, see https://urllib3.readthedocs.org/en/latest/security.html#insecureplatformwarning.
InsecurePlatformWarning
Using cached isort-4.0.0-py2.py3-none-any.whl
Installing collected packages: isort
Successfully installed isort-4.0.0
ERROR: /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_health_checker.py Imports are incorrectly sorted.
--- /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_health_checker.py:before 2016-03-30 22:01:12.247552
+++ /home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/executor/common/test_health_checker.py:after 2016-03-30 22:13:40.741774
@@ -39,7 +39,7 @@
from .fixtures import HELLO_WORLD, MESOS_JOB
-from gen.apache.aurora.api.ttypes import AssignedTask, ExecutorConfig, TaskConfig, JobKey
+from gen.apache.aurora.api.ttypes import AssignedTask, ExecutorConfig, JobKey, TaskConfig
class TestHealthChecker(unittest.TestCase):
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On March 30, 2016, 9:58 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 9:58 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
> On March 30, 2016, 3:07 p.m., John Sirois wrote:
> > There is code to do this in apache.thermos.core.process.Process and its tested here: https://github.com/apache/aurora/blob/master/src/test/python/apache/thermos/core/test_process.py#L103
> > Process (ProcessBase) does look a bit fat for this, but it would be nice if there was eventually 1 tested piece of code for executing an un-priviledged process in the task sandbox.
I agree that it would be preferable. I admit i only did a quick pass through `Process`, but backed away because it does not support a timeout, and wants to write several files to disk (including the checkpoint stream). I could certainly retrofit it to do that, but it was hard to justify.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126206
-----------------------------------------------------------
On March 30, 2016, 2:58 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 2:58 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by John Sirois <js...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126206
-----------------------------------------------------------
There is code to do this in apache.thermos.core.process.Process and its tested here: https://github.com/apache/aurora/blob/master/src/test/python/apache/thermos/core/test_process.py#L103
Process (ProcessBase) does look a bit fat for this, but it would be nice if there was eventually 1 tested piece of code for executing an un-priviledged process in the task sandbox.
- John Sirois
On March 30, 2016, 3:58 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 3:58 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
>
> Bill Farner wrote:
> I'm generally a fan of scoping exceptions to catch only where they are known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same exception type, i would probably put them in the same `try`, but in this case they are distinct.
>
> However, i'm not sure what you mean by 'Popen already supports it', so it's possible i'm missing something.
>
> Dmitriy Shirchenko wrote:
> If you look at subprocess32, timeout is an option into Popen. What's the logic behind changing how timeout is done via process.wait instead of relying on Popen()?
>
> Bill Farner wrote:
> I don't see a `timeout` kwarg on any `Popen` docs or sources.
>
> https://docs.python.org/3.3/library/subprocess.html#popen-constructor
> https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L716-L722
`check_call` has it. I was wrong: made a wrong assumption that Popen has it.
https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L587
I guess question should be rephrased: why switch to Popen when `check_call` does what we need and it just a wrapper around Popen with timeout.
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
On March 31, 2016, 6:57 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:57 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
> On March 31, 2016, 9:20 a.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
>
> Bill Farner wrote:
> I'm generally a fan of scoping exceptions to catch only where they are known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same exception type, i would probably put them in the same `try`, but in this case they are distinct.
>
> However, i'm not sure what you mean by 'Popen already supports it', so it's possible i'm missing something.
>
> Dmitriy Shirchenko wrote:
> If you look at subprocess32, timeout is an option into Popen. What's the logic behind changing how timeout is done via process.wait instead of relying on Popen()?
I don't see a `timeout` kwarg on any `Popen` docs or sources.
https://docs.python.org/3.3/library/subprocess.html#popen-constructor
https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L716-L722
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
On March 31, 2016, 11:38 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 11:38 a.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
> On March 31, 2016, 9:20 a.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
>
> Bill Farner wrote:
> I'm generally a fan of scoping exceptions to catch only where they are known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same exception type, i would probably put them in the same `try`, but in this case they are distinct.
>
> However, i'm not sure what you mean by 'Popen already supports it', so it's possible i'm missing something.
>
> Dmitriy Shirchenko wrote:
> If you look at subprocess32, timeout is an option into Popen. What's the logic behind changing how timeout is done via process.wait instead of relying on Popen()?
>
> Bill Farner wrote:
> I don't see a `timeout` kwarg on any `Popen` docs or sources.
>
> https://docs.python.org/3.3/library/subprocess.html#popen-constructor
> https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L716-L722
>
> Dmitriy Shirchenko wrote:
> `check_call` has it. I was wrong: made a wrong assumption that Popen has it.
> https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L587
>
> I guess question should be rephrased: why switch to Popen when `check_call` does what we need and it just a wrapper around Popen with timeout.
It was originally because i did not notice that `check_call` accepts the `preexec_fn`, but then because i did not realize `check_call` would kill upon a timeout (and we had no access to the PID to kill on our own). However, this doc section clears it up:
> The timeout argument is passed to Popen.wait(). If the timeout expires, the child process will be killed and then waited for again. The TimeoutExpired exception will be re-raised after the child process has terminated.
Will switch back to `check_call`, thanks!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
On March 31, 2016, 11:57 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 11:57 a.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
> On March 31, 2016, 9:20 a.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 60
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line60>
> >
> > why did you get rid of .format? i personally find it much clearer to understand so would you please justify this decision.
> >
> > some refs:
> > http://stackoverflow.com/a/5082482
> > talk of deprecating % altogether in python3 and explicit mention for using .format.
> > https://docs.python.org/3/whatsnew/2.6.html#pep-3101
That may be true, but `%` is still the predominant pattern in this codebase for string formatting.
```console
$ grep -R '.format(' src/{main,test}/python | wc -l
3
$ grep -R ' % ' src/{main,test}/python | wc -l
785
```
I tend to follow the convention until a mass switch takes place. Patches welcome :-)
> On March 31, 2016, 9:20 a.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 74
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line74>
> >
> > :nit .format instead of interpolation.
Ditto per above.
> On March 31, 2016, 9:20 a.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
I'm generally a fan of scoping exceptions to catch only where they are known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same exception type, i would probably put them in the same `try`, but in this case they are distinct.
However, i'm not sure what you mean by 'Popen already supports it', so it's possible i'm missing something.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
On March 30, 2016, 4:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 4:41 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 60
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line60>
> >
> > why did you get rid of .format? i personally find it much clearer to understand so would you please justify this decision.
> >
> > some refs:
> > http://stackoverflow.com/a/5082482
> > talk of deprecating % altogether in python3 and explicit mention for using .format.
> > https://docs.python.org/3/whatsnew/2.6.html#pep-3101
>
> Bill Farner wrote:
> That may be true, but `%` is still the predominant pattern in this codebase for string formatting.
> ```console
> $ grep -R '.format(' src/{main,test}/python | wc -l
> 3
> $ grep -R ' % ' src/{main,test}/python | wc -l
> 785
> ```
> I tend to follow the convention until a mass switch takes place. Patches welcome :-)
I see your point. My point is that objectively .format is the correct way (given refs ^). I don't feel strongly about it, but I will say that I don't agree with logic of going down the incorrect path as it will make it difficult (or impossible) to correct the behavior down the line. In all of my patches, I use .format but it looks like I am getting overruled and using .format is discouraged :). It's not a big deal!
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
>
> Bill Farner wrote:
> I'm generally a fan of scoping exceptions to catch only where they are known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same exception type, i would probably put them in the same `try`, but in this case they are distinct.
>
> However, i'm not sure what you mean by 'Popen already supports it', so it's possible i'm missing something.
If you look at subprocess32, timeout is an option into Popen. What's the logic behind changing how timeout is done via process.wait instead of relying on Popen()?
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
On March 31, 2016, 6:38 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:38 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
-----------------------------------------------------------
src/main/python/apache/aurora/common/health_check/shell.py (line 34)
<https://reviews.apache.org/r/45506/#comment189279>
document inside docstring, plz.
src/main/python/apache/aurora/common/health_check/shell.py (line 56)
<https://reviews.apache.org/r/45506/#comment189291>
why did you get rid of .format? i personally find it much clearer to understand so would you please justify this decision.
some refs:
http://stackoverflow.com/a/5082482
talk of deprecating % altogether in python3 and explicit mention for using .format.
https://docs.python.org/3/whatsnew/2.6.html#pep-3101
src/main/python/apache/aurora/common/health_check/shell.py (line 61)
<https://reviews.apache.org/r/45506/#comment189293>
why are you doing a separate try/except with timeout when subprocess.Popen already supports it.
you can still do a kill if that's your intention in one swoop.
src/main/python/apache/aurora/common/health_check/shell.py (line 69)
<https://reviews.apache.org/r/45506/#comment189295>
:nit .format instead of interpolation.
src/test/python/apache/aurora/common/health_check/test_shell.py (line 24)
<https://reviews.apache.org/r/45506/#comment189277>
damn, my bad. thanks for catching this!
- Dmitriy Shirchenko
On March 30, 2016, 11:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 11:41 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> On March 31, 2016, 6:11 p.m., Zameer Manji wrote:
> > The change and the tests LGTM.
> >
> > I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
>
> Joshua Cohen wrote:
> If we want something that would give us more certainty that the e2e test behaved as expected, we could touch a file in /tmp as root (from the test runner) and configure a shell health checker that tries to remove it. Then we can assert that the health check failed and that the file still exists (thus giving us confidence that the reason for the failure was permission-based and not due to some other factor).
>
> Bill Farner wrote:
> I was thinking something along the lines of access as well. How about a check that tries to do something pseudo-malicious like delete `/etc/passwd`?
>
> Zameer Manji wrote:
> +1 deleting /etc/passwd or similar it a good test.
Yea, e2e has a test which makes sure a failed health check rolls back the update: https://github.com/apache/aurora/blob/master/src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh#L206
Should be super easy to modify it to roll back on a file you aren't supposed to delete.
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
On March 31, 2016, 6:38 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:38 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Joshua Cohen <jc...@apache.org>.
> On March 31, 2016, 6:11 p.m., Zameer Manji wrote:
> > The change and the tests LGTM.
> >
> > I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
If we want something that would give us more certainty that the e2e test behaved as expected, we could touch a file in /tmp as root (from the test runner) and configure a shell health checker that tries to remove it. Then we can assert that the health check failed and that the file still exists (thus giving us confidence that the reason for the failure was permission-based and not due to some other factor).
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
On March 31, 2016, 6:01 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:01 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Joshua Cohen <jc...@apache.org>.
> On March 31, 2016, 6:11 p.m., Zameer Manji wrote:
> > The change and the tests LGTM.
> >
> > I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
>
> Joshua Cohen wrote:
> If we want something that would give us more certainty that the e2e test behaved as expected, we could touch a file in /tmp as root (from the test runner) and configure a shell health checker that tries to remove it. Then we can assert that the health check failed and that the file still exists (thus giving us confidence that the reason for the failure was permission-based and not due to some other factor).
>
> Bill Farner wrote:
> I was thinking something along the lines of access as well. How about a check that tries to do something pseudo-malicious like delete `/etc/passwd`?
>
> Zameer Manji wrote:
> +1 deleting /etc/passwd or similar it a good test.
>
> Dmitriy Shirchenko wrote:
> Yea, e2e has a test which makes sure a failed health check rolls back the update: https://github.com/apache/aurora/blob/master/src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh#L206
>
> Should be super easy to modify it to roll back on a file you aren't supposed to delete.
Sounds good to me!
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
On March 31, 2016, 6:38 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:38 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Zameer Manji <zm...@apache.org>.
> On March 31, 2016, 11:11 a.m., Zameer Manji wrote:
> > The change and the tests LGTM.
> >
> > I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
>
> Joshua Cohen wrote:
> If we want something that would give us more certainty that the e2e test behaved as expected, we could touch a file in /tmp as root (from the test runner) and configure a shell health checker that tries to remove it. Then we can assert that the health check failed and that the file still exists (thus giving us confidence that the reason for the failure was permission-based and not due to some other factor).
>
> Bill Farner wrote:
> I was thinking something along the lines of access as well. How about a check that tries to do something pseudo-malicious like delete `/etc/passwd`?
+1 deleting /etc/passwd or similar it a good test.
- Zameer
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
On March 31, 2016, 11:38 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 11:38 a.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
> On March 31, 2016, 11:11 a.m., Zameer Manji wrote:
> > The change and the tests LGTM.
> >
> > I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
>
> Joshua Cohen wrote:
> If we want something that would give us more certainty that the e2e test behaved as expected, we could touch a file in /tmp as root (from the test runner) and configure a shell health checker that tries to remove it. Then we can assert that the health check failed and that the file still exists (thus giving us confidence that the reason for the failure was permission-based and not due to some other factor).
I was thinking something along the lines of access as well. How about a check that tries to do something pseudo-malicious like delete `/etc/passwd`?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
On March 31, 2016, 11:01 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 11:01 a.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126378
-----------------------------------------------------------
The change and the tests LGTM.
I currently have great ideas on how to ensure end to end validation. The best idea that I can provide is make use of the shell checker in the e2e tests. The program executed by the shell checker should just return 1 if it is executed as root and return 0 if it isn't. The e2e test can check for task failure and infer that the command was run as root if the task fails.
src/main/python/apache/aurora/common/health_check/shell.py (line 43)
<https://reviews.apache.org/r/45506/#comment189370>
There is no need to set `preexec_fn` to `_noop` the default value for the `Popen` method is `None` so it should be fine to plumb the None into the method.
- Zameer Manji
On March 31, 2016, 11:01 a.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 11:01 a.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126387
-----------------------------------------------------------
Ship it!
Master (193f17e) 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 31, 2016, 6:01 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:01 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126408
-----------------------------------------------------------
Ship it!
Master (193f17e) 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 31, 2016, 6:57 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 6:57 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126447
-----------------------------------------------------------
Ship it!
Ship It!
- Zameer Manji
On March 31, 2016, 1:18 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 1:18 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126443
-----------------------------------------------------------
Ship it!
Ship It!
- Dmitriy Shirchenko
On March 31, 2016, 8:18 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 8:18 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126434
-----------------------------------------------------------
Ship it!
Master (193f17e) 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 31, 2016, 8:18 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 31, 2016, 8:18 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
> src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
(Updated March 31, 2016, 1:18 p.m.)
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs (updated)
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
Diff: https://reviews.apache.org/r/45506/diff/
Testing
-------
Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
Thanks,
Bill Farner
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
(Updated March 31, 2016, 11:57 a.m.)
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs (updated)
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c709b035a54822ff700136963e2f942f30f38898
src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh e1c12bbd4382c31e576439f6693d82d5661029b9
Diff: https://reviews.apache.org/r/45506/diff/
Testing (updated)
-------
Altered an end-to-end test case to verify that shell checks are not run as a privileged user.
Thanks,
Bill Farner
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
(Updated March 31, 2016, 11:38 a.m.)
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs (updated)
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
Diff: https://reviews.apache.org/r/45506/diff/
Testing
-------
I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
Thanks,
Bill Farner
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
(Updated March 31, 2016, 11:01 a.m.)
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs (updated)
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
Diff: https://reviews.apache.org/r/45506/diff/
Testing
-------
I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
Thanks,
Bill Farner
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126226
-----------------------------------------------------------
Ship it!
Master (193f17e) 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 30, 2016, 11:41 p.m., Bill Farner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
>
> (Updated March 30, 2016, 11:41 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> -------
>
> I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the
task user.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/
-----------------------------------------------------------
(Updated March 30, 2016, 4:41 p.m.)
Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
Changes
-------
green build, moved uid/gid awareness up a level from `ShellHealthCheck`.
Bugs: AURORA-1641
https://issues.apache.org/jira/browse/AURORA-1641
Repository: aurora
Description
-------
Here's a stab at this using `os` and `pwd` modules directly to demote health checks to the target user.
Diffs (updated)
-----
src/main/python/apache/aurora/common/health_check/shell.py 6cb7dfc164f4e16143fc974d50c19a5887d32015
src/main/python/apache/aurora/executor/common/health_checker.py 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
src/test/python/apache/aurora/common/health_check/test_shell.py 7026af8c4671a40f4b517ecf12149eac34a552c8
src/test/python/apache/aurora/executor/common/test_health_checker.py 19c4f76347e34374c29974c182d1f4c118bcb18d
Diff: https://reviews.apache.org/r/45506/diff/
Testing
-------
I haven't spent any time thinking of a test strategy for this, but i don't think we should proceed without end-to-end validation. I'm open to ideas here.
Thanks,
Bill Farner