Re: Review Request 44486: Exposing ports to shell health checkers

[Zameer Manji]

Currently the health checker is just launched by the executor blindly. This
proposal now plumbs information about the task to the subprocess. We
already have a mechanism to do this, it's the DSL provided to users when
they compose their processes. In that case we compose a command line with
variables like './foo --port={{thermos.ports[http]}}'. When this string is
passed to thermos (the parts of the executor that actually launch the
process), the variables are interpolated and the command string is passed
to $SHELL which then runs the process.

This proposal diverges from the existing setup because now ports are passed
to the subprocess via a completely different method. This proposal is also
limited because only ports are passed to the subprocess. Why not other
information available about the task such as the cluster, role, or
hostname? We can currently pass that information to processes via the DSL
as well by having cmdlines like './process --hostname={{mesos.hostname}}'.

I think removing this inconsistency is good, because the DSL becomes easier
to use and there is less of a support burden when educating users about the
DSL.

To implement this I see at least two (if not more) approaches:

   - The complex method involves using the code from thermos (the code that
   runs a process, interpolates variables, etc) into the shell health checker
   directly. It's a bit complex but it is doable and it has the most code
   reuse. It's different than using the subprocess32 module, because instead
   of spawning a subprocess, thermos will fork off a 'runner' process which
   then creates the desired process as a subprocess and the runner
   communicates state back to the caller via writing the process state to
   checkpoint files.
   - The alternative approach involves setting shell=True, and using
   the pystachio library (the implementation of the interpolation) to fill in
   the variables and then passing that string to subprocess32. This has a two
   benefits: it is much simpler and this is already done elsewhere in the
   code. The only drawback is that we have to do shell=True to prevent the
   argument issue you were describing, but it was objected upon for security
   reasons and it is a breaking change (although it seems Uber is the only
   user so we can skip over that).

I think the latter approach is preferable and I don't see it being a lot of
extra code. I think we can ignore the security implications from shell=True
for two reasons:

   - The documentation says "[...], unsanitized input from an untrusted
   source, [...]" which is not true here. Aurora assumes that the cmd strings
   provided in the DSL comes from the user which is authenticated by the
   scheduler when submitting the task.
   - The other process strings consumed by the DSL are passed to thermos
   which currently passes it to $SHELL, so we are already open to this kind of
   issue.

To implement the latter approach you can take a look at
`DistributedCommandRunner` in
`src/main/python/apache/aurora/client/api/command_runner.py` which supports
running a command in the context of an already running task and does
variable interpolation.

Specifically you can see this code:

  @classmethod
  def substitute(cls, command, task, cluster, **kw):
prefix_command = 'cd %s;' % cls.thermos_sandbox(cluster, **kw)
thermos_namespace = ThermosContext(
task_id=task.assignedTask.taskId,
ports=task.assignedTask.assignedPorts)
mesos_namespace = MesosContext(instance=task.assignedTask.instanceId)
command = String(prefix_command + command) % Environment(
thermos=thermos_namespace,
mesos=mesos_namespace)
return command.get()


This code takes a command string, the task object and other parameters and
uses the pystachio library to interpolate the values into the string.

I don't think constructing the ThermosContext/MesosContext objects is too
hard in the shell health checker and the interpolation is only two extra
lines.

I hope this clears up what I think should be done to this code.



On Tue, Mar 8, 2016 at 3:12 PM, Dmitriy Shirchenko 
wrote:

> https://docs.python.org/2/library/subprocess.html#frequently-used-arguments
>
> Ok, sorry, so what exactly are you proposing I do instead of simply
> passing in environment variables?
>
> What themos code? What do we replace subprocess with?
>
> I'm new to this code base so I'm still figuring things out so pardon my
> ignorance.
>
> Do you have strong concerns about this approach or just want it to be
> perfect?
>
> On Tue, Mar 8, 2016, 12:04 PM Zameer Manji  wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/44486/
>>
>> On March 8th, 2016, 11:25 a.m. PST, *Zameer Manji* wrote:
>>
>> The code for this approach looks fine to me, but I'm not sure if this 
>> approach is the way to go.
>>
>> Why can't the command for the health checker include 
>> '{{thermos.ports[http]}}' and we can resolve that valu

Re: Review Request 44486: Exposing ports to shell health checkers

[Bill Farner]

> On March 8, 2016, 11:25 a.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.
> 
> Joshua Cohen wrote:
> I think this is an excellent point, good catch Zameer!
> 
> Dmitriy, is there any reason why this approach won't work for you guys?
> 
> Dmitriy Shirchenko wrote:
> Yea, I tried using that approach at first. But we need to allocate 10 
> ports (mix of HTTP and another RPC protocol(s)) for some services, and our 
> existing health check scripts at the moment are just simple bash scripts. 
> Dealing with 10 arguments will become difficult (imagine writing one for that 
> case), especially since order will matter and the owner will need to keep 
> track of order in which they are passed in (eg is it an HTTP one, or some RPC 
> protocol.. oh wait, I thought that was passed in first... dammit I have to 
> read code in how our internal system massages them since we bypass aurora 
> client completely to see how it actually works). Environment variables are 
> just easier to deal with. 
> 
> Does this make sense? Perhaps I could have explained why this approach in 
> the Summary/Description.
> 
> Zameer Manji wrote:
> Using the DSL doesn't mean arguments, you can do something like this:
> 
> 'HTTP_PORT={{thermos.ports[http]}} RPC_PORT={{thermos.ports[rpc]}} 
> /usr/bin/health_checker'
> 
> Dmitriy Shirchenko wrote:
> Here's the code that runs the command:
> ```
> cmd = shlex.split(self.cmd)
> try:
>   subprocess.check_call(cmd, timeout=self.timeout_secs)
> ```
> 
> so if self.cmd is 'HTTP_PORT=123 RPC_PORT=234 /usr/bin/health_checker' 
> aws you are suggesting, how would this work? `check_call` doesn't pass 
> through environment variables w/out shell=True AFAIK. shell=True has security 
> concerns so it's disabled by default.
> 
> Zameer Manji wrote:
> Can you elaborate with with the security concerns?
> 
> Currently all of the other processes launched by thermos have the cmd 
> string passed to the shell which is very convinent. I think doing that here 
> would be useful as well. Infact, we could just replace the subprocess work 
> here by just re-using the existing thermos code we have which will pass the 
> cmd string to bash, interpolate variables and setuid, setguid so the health 
> check process doesn't run a root.

I'm glad you raised this, Zameer.  For some reason in my head i actually 
convinced myself this is how it worked.  I can see how both environment 
variables and mustache-style templating could be useful, and agree that we 
should offer templating for consistency.  I assume this is trivial to support 
and include in this patch.


- Bill


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


On March 8, 2016, 10:32 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 10:32 a.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

https://docs.python.org/2/library/subprocess.html#frequently-used-arguments

Ok, sorry, so what exactly are you proposing I do instead of simply passing
in environment variables?

What themos code? What do we replace subprocess with?

I'm new to this code base so I'm still figuring things out so pardon my
ignorance.

Do you have strong concerns about this approach or just want it to be
perfect?

On Tue, Mar 8, 2016, 12:04 PM Zameer Manji  wrote:

> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
>
> On March 8th, 2016, 11:25 a.m. PST, *Zameer Manji* wrote:
>
> The code for this approach looks fine to me, but I'm not sure if this 
> approach is the way to go.
>
> Why can't the command for the health checker include 
> '{{thermos.ports[http]}}' and we can resolve that value before launching the 
> subprocess? Thats more consistent with the rest of the DSL. Further, using 
> the mustache variables in the command variable would allow the health checker 
> process to have access to all of the same information that task processes 
> have like hostname.
>
> For example the command could be '/usr/bin/health_checker 
> --port-to-check={{thermos.ports[http]}}'.
>
> On March 8th, 2016, 11:29 a.m. PST, *Joshua Cohen* wrote:
>
> I think this is an excellent point, good catch Zameer!
>
> Dmitriy, is there any reason why this approach won't work for you guys?
>
> On March 8th, 2016, 11:39 a.m. PST, *Dmitriy Shirchenko* wrote:
>
> Yea, I tried using that approach at first. But we need to allocate 10 ports 
> (mix of HTTP and another RPC protocol(s)) for some services, and our existing 
> health check scripts at the moment are just simple bash scripts. Dealing with 
> 10 arguments will become difficult (imagine writing one for that case), 
> especially since order will matter and the owner will need to keep track of 
> order in which they are passed in (eg is it an HTTP one, or some RPC 
> protocol.. oh wait, I thought that was passed in first... dammit I have to 
> read code in how our internal system massages them since we bypass aurora 
> client completely to see how it actually works). Environment variables are 
> just easier to deal with.
>
> Does this make sense? Perhaps I could have explained why this approach in the 
> Summary/Description.
>
> On March 8th, 2016, 11:42 a.m. PST, *Zameer Manji* wrote:
>
> Using the DSL doesn't mean arguments, you can do something like this:
>
> 'HTTP_PORT={{thermos.ports[http]}} RPC_PORT={{thermos.ports[rpc]}} 
> /usr/bin/health_checker'
>
> On March 8th, 2016, 11:49 a.m. PST, *Dmitriy Shirchenko* wrote:
>
> Here's the code that runs the command:
>
> cmd = shlex.split(self.cmd)
> try:
>   subprocess.check_call(cmd, timeout=self.timeout_secs)
>
> so if self.cmd is 'HTTP_PORT=123 RPC_PORT=234 /usr/bin/health_checker' aws
> you are suggesting, how would this work? check_call doesn't pass through
> environment variables w/out shell=True AFAIK. shell=True has security
> concerns so it's disabled by default.
>
> Can you elaborate with with the security concerns?
>
> Currently all of the other processes launched by thermos have the cmd string 
> passed to the shell which is very convinent. I think doing that here would be 
> useful as well. Infact, we could just replace the subprocess work here by 
> just re-using the existing thermos code we have which will pass the cmd 
> string to bash, interpolate variables and setuid, setguid so the health check 
> process doesn't run a root.
>
>
> - Zameer
>
> On March 8th, 2016, 10:32 a.m. PST, Dmitriy Shirchenko wrote:
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> By Dmitriy Shirchenko.
>
> *Updated March 8, 2016, 10:32 a.m.*
> *Bugs: * AURORA-1622 
> *Repository: * aurora
> Description
>
> Exposing ports to shell health checkers
>
> Testing
>
> Unit and end to end test.
>
> Diffs
>
>- NEWS (b84a94550f93691eba0220afedb2bb4d5e00e6bd)
>- docs/configuration-reference.md
>(10702ff4e700b6da7bdd7fd036de442be1eba45c)
>- src/main/python/apache/aurora/common/health_check/shell.py
>(890bf0c5d50d0022c044a37191a2e3145cc6340f)
>- src/main/python/apache/aurora/executor/common/health_checker.py
>(303972778baa04e9d7dd47fb208fe1427e779976)
>- src/test/python/apache/aurora/common/health_check/test_shell.py
>(84f717fbf724c11863b4980fd2740dc23fe1404e)
>- src/test/python/apache/aurora/executor/common/test_health_checker.py
>(9bebce8f5a26662f58075d7ce881a8bdacb2fe46)
>
> View Diff 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Zameer Manji]

> On March 8, 2016, 11:25 a.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.
> 
> Joshua Cohen wrote:
> I think this is an excellent point, good catch Zameer!
> 
> Dmitriy, is there any reason why this approach won't work for you guys?
> 
> Dmitriy Shirchenko wrote:
> Yea, I tried using that approach at first. But we need to allocate 10 
> ports (mix of HTTP and another RPC protocol(s)) for some services, and our 
> existing health check scripts at the moment are just simple bash scripts. 
> Dealing with 10 arguments will become difficult (imagine writing one for that 
> case), especially since order will matter and the owner will need to keep 
> track of order in which they are passed in (eg is it an HTTP one, or some RPC 
> protocol.. oh wait, I thought that was passed in first... dammit I have to 
> read code in how our internal system massages them since we bypass aurora 
> client completely to see how it actually works). Environment variables are 
> just easier to deal with. 
> 
> Does this make sense? Perhaps I could have explained why this approach in 
> the Summary/Description.
> 
> Zameer Manji wrote:
> Using the DSL doesn't mean arguments, you can do something like this:
> 
> 'HTTP_PORT={{thermos.ports[http]}} RPC_PORT={{thermos.ports[rpc]}} 
> /usr/bin/health_checker'
> 
> Dmitriy Shirchenko wrote:
> Here's the code that runs the command:
> ```
> cmd = shlex.split(self.cmd)
> try:
>   subprocess.check_call(cmd, timeout=self.timeout_secs)
> ```
> 
> so if self.cmd is 'HTTP_PORT=123 RPC_PORT=234 /usr/bin/health_checker' 
> aws you are suggesting, how would this work? `check_call` doesn't pass 
> through environment variables w/out shell=True AFAIK. shell=True has security 
> concerns so it's disabled by default.

Can you elaborate with with the security concerns?

Currently all of the other processes launched by thermos have the cmd string 
passed to the shell which is very convinent. I think doing that here would be 
useful as well. Infact, we could just replace the subprocess work here by just 
re-using the existing thermos code we have which will pass the cmd string to 
bash, interpolate variables and setuid, setguid so the health check process 
doesn't run a root.


- Zameer


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


On March 8, 2016, 10:32 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 10:32 a.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

> On March 8, 2016, 7:25 p.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.
> 
> Joshua Cohen wrote:
> I think this is an excellent point, good catch Zameer!
> 
> Dmitriy, is there any reason why this approach won't work for you guys?
> 
> Dmitriy Shirchenko wrote:
> Yea, I tried using that approach at first. But we need to allocate 10 
> ports (mix of HTTP and another RPC protocol(s)) for some services, and our 
> existing health check scripts at the moment are just simple bash scripts. 
> Dealing with 10 arguments will become difficult (imagine writing one for that 
> case), especially since order will matter and the owner will need to keep 
> track of order in which they are passed in (eg is it an HTTP one, or some RPC 
> protocol.. oh wait, I thought that was passed in first... dammit I have to 
> read code in how our internal system massages them since we bypass aurora 
> client completely to see how it actually works). Environment variables are 
> just easier to deal with. 
> 
> Does this make sense? Perhaps I could have explained why this approach in 
> the Summary/Description.
> 
> Zameer Manji wrote:
> Using the DSL doesn't mean arguments, you can do something like this:
> 
> 'HTTP_PORT={{thermos.ports[http]}} RPC_PORT={{thermos.ports[rpc]}} 
> /usr/bin/health_checker'

Here's the code that runs the command:
```
cmd = shlex.split(self.cmd)
try:
  subprocess.check_call(cmd, timeout=self.timeout_secs)
```

so if self.cmd is 'HTTP_PORT=123 RPC_PORT=234 /usr/bin/health_checker' aws you 
are suggesting, how would this work? `check_call` doesn't pass through 
environment variables w/out shell=True AFAIK. shell=True has security concerns 
so it's disabled by default.


- Dmitriy


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


On March 8, 2016, 6:32 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 6:32 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Zameer Manji]

> On March 8, 2016, 11:25 a.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.
> 
> Joshua Cohen wrote:
> I think this is an excellent point, good catch Zameer!
> 
> Dmitriy, is there any reason why this approach won't work for you guys?
> 
> Dmitriy Shirchenko wrote:
> Yea, I tried using that approach at first. But we need to allocate 10 
> ports (mix of HTTP and another RPC protocol(s)) for some services, and our 
> existing health check scripts at the moment are just simple bash scripts. 
> Dealing with 10 arguments will become difficult (imagine writing one for that 
> case), especially since order will matter and the owner will need to keep 
> track of order in which they are passed in (eg is it an HTTP one, or some RPC 
> protocol.. oh wait, I thought that was passed in first... dammit I have to 
> read code in how our internal system massages them since we bypass aurora 
> client completely to see how it actually works). Environment variables are 
> just easier to deal with. 
> 
> Does this make sense? Perhaps I could have explained why this approach in 
> the Summary/Description.

Using the DSL doesn't mean arguments, you can do something like this:

'HTTP_PORT={{thermos.ports[http]}} RPC_PORT={{thermos.ports[rpc]}} 
/usr/bin/health_checker'


- Zameer


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


On March 8, 2016, 10:32 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 10:32 a.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

> On March 8, 2016, 7:25 p.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.
> 
> Joshua Cohen wrote:
> I think this is an excellent point, good catch Zameer!
> 
> Dmitriy, is there any reason why this approach won't work for you guys?

Yea, I tried using that approach at first. But we need to allocate 10 ports 
(mix of HTTP and another RPC protocol(s)) for some services, and our existing 
health check scripts at the moment are just simple bash scripts. Dealing with 
10 arguments will become difficult (imagine writing one for that case), 
especially since order will matter and the owner will need to keep track of 
order in which they are passed in (eg is it an HTTP one, or some RPC protocol.. 
oh wait, I thought that was passed in first... dammit I have to read code in 
how our internal system massages them since we bypass aurora client completely 
to see how it actually works). Environment variables are just easier to deal 
with. 

Does this make sense? Perhaps I could have explained why this approach in the 
Summary/Description.


- Dmitriy


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


On March 8, 2016, 6:32 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 6:32 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Joshua Cohen]

> On March 8, 2016, 7:25 p.m., Zameer Manji wrote:
> > The code for this approach looks fine to me, but I'm not sure if this 
> > approach is the way to go.
> > 
> > Why can't the command for the health checker include 
> > '{{thermos.ports[http]}}' and we can resolve that value before launching 
> > the subprocess? Thats more consistent with the rest of the DSL. Further, 
> > using the mustache variables in the command variable would allow the health 
> > checker process to have access to all of the same information that task 
> > processes have like hostname.
> > 
> > For example the command could be '/usr/bin/health_checker 
> > --port-to-check={{thermos.ports[http]}}'.

I think this is an excellent point, good catch Zameer!

Dmitriy, is there any reason why this approach won't work for you guys?


- Joshua


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


On March 8, 2016, 6:32 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 6:32 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Zameer Manji]

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



The code for this approach looks fine to me, but I'm not sure if this approach 
is the way to go.

Why can't the command for the health checker include '{{thermos.ports[http]}}' 
and we can resolve that value before launching the subprocess? Thats more 
consistent with the rest of the DSL. Further, using the mustache variables in 
the command variable would allow the health checker process to have access to 
all of the same information that task processes have like hostname.

For example the command could be '/usr/bin/health_checker 
--port-to-check={{thermos.ports[http]}}'.

- Zameer Manji


On March 8, 2016, 10:32 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 10:32 a.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Aurora ReviewBot]

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



Master (26efe55) is red with this patch.
  ./build-support/jenkins/build.sh

   proxy_driver = ProxyDriver()
   with temporary_dir() as checkpoint_root:
 te = AuroraExecutor(
 >   
runner_provider=make_provider(checkpoint_root),
 
sandbox_provider=DefaultTestSandboxProvider())
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:580: 
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:193: in 
make_provider
 pex_location=thermos_runner_path(),
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 build = True
 
 def thermos_runner_path(build=True):
   if not build:
 return getattr(thermos_runner_path, 'value', 
None)
 
   if not hasattr(thermos_runner_path, 'value'):
 pex_dir = safe_mkdtemp()
 >   assert subprocess.call(["./pants", 
"--pants-distdir=%s" % pex_dir, "binary",
   
"src/main/python/apache/thermos/runner:thermos_runner"]) == 0
 E   assert 1 == 0
 E+  where 1 = (['./pants', '--pants-distdir=/tmp/user/2/tmpZBVjJy', 
'binary', 'src/main/python/apache/thermos/runner:thermos_runner'])
 E+where  = subprocess.call
 
 
src/test/python/apache/aurora/executor/test_thermos_executor.py:185: 
AssertionError
 -- Captured stderr call --
 Traceback (most recent call last):
   File 
"/home/jenkins/.cache/pants/setup/bootstrap-Linux-x86_64/0.0.71/bin/pants", 
line 7, in 
 from pants.bin.pants_exe import main
 ImportError: No module named pants.bin.pants_exe
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  16 failed, 637 passed, 5 skipped, 1 warnings, 8 
error in 273.68 seconds 
 
FAILURE


19:23:56 06:00   [complete]
   FAILURE


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

- Aurora ReviewBot


On March 8, 2016, 6:32 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 6:32 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

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

(Updated March 8, 2016, 6:32 p.m.)


Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.


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


Repository: aurora


Description
---

Exposing ports to shell health checkers


Diffs (updated)
-

  NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
  docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
  src/main/python/apache/aurora/common/health_check/shell.py 
890bf0c5d50d0022c044a37191a2e3145cc6340f 
  src/main/python/apache/aurora/executor/common/health_checker.py 
303972778baa04e9d7dd47fb208fe1427e779976 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
84f717fbf724c11863b4980fd2740dc23fe1404e 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
9bebce8f5a26662f58075d7ce881a8bdacb2fe46 

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


Testing
---

Unit and end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 44486: Exposing ports to shell health checkers

[John Sirois]

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


Fix it, then Ship it!




LGTM - thanks for the doc additions.

Zameer will take this review home.


NEWS (line 17)


This belongs in the `New/updated` section above and it's probably worth 
linking the new docs on github.  The 0.12.0 entry below starting with `- Added 
support for jobs to specify arbitrary ZooKeeper paths...` is a good example.



docs/configuration-reference.md (line 453)


s/look/looks/ and perhaps language more closely aligned with environment 
variables - perhaps something like "... will have the `HTTP_PORT` variable 
exported in its environment."


- John Sirois


On March 7, 2016, 7:10 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 7, 2016, 7:10 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Aurora ReviewBot]

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


Ship it!




Master (a91a759) 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 8, 2016, 2:10 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 8, 2016, 2:10 a.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
>   docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

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

(Updated March 8, 2016, 2:10 a.m.)


Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.


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


Repository: aurora


Description
---

Exposing ports to shell health checkers


Diffs (updated)
-

  NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
  docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
  src/main/python/apache/aurora/common/health_check/shell.py 
890bf0c5d50d0022c044a37191a2e3145cc6340f 
  src/main/python/apache/aurora/executor/common/health_checker.py 
303972778baa04e9d7dd47fb208fe1427e779976 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
84f717fbf724c11863b4980fd2740dc23fe1404e 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
9bebce8f5a26662f58075d7ce881a8bdacb2fe46 

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


Testing
---

Unit and end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

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

(Updated March 8, 2016, 2:08 a.m.)


Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.


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


Repository: aurora


Description
---

Exposing ports to shell health checkers


Diffs (updated)
-

  NEWS b84a94550f93691eba0220afedb2bb4d5e00e6bd 
  docs/configuration-reference.md 10702ff4e700b6da7bdd7fd036de442be1eba45c 
  src/main/python/apache/aurora/common/health_check/shell.py 
890bf0c5d50d0022c044a37191a2e3145cc6340f 
  src/main/python/apache/aurora/executor/common/health_checker.py 
303972778baa04e9d7dd47fb208fe1427e779976 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
84f717fbf724c11863b4980fd2740dc23fe1404e 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
9bebce8f5a26662f58075d7ce881a8bdacb2fe46 

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


Testing
---

Unit and end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 44486: Exposing ports to shell health checkers

[John Sirois]

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



This code lgtm, but adding some docs that define what a shell health checker 
can expect in its environment from thermos would be good.  Maybe 
[here](https://github.com/apache/aurora/blob/master/docs/configuration-reference.md#shellhealthchecker-objects).
  This also seems [NEWS](https://github.com/apache/aurora/blob/master/NEWS) 
worthy.

- John Sirois


On March 7, 2016, 6:42 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44486/
> ---
> 
> (Updated March 7, 2016, 6:42 p.m.)
> 
> 
> Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1622
> https://issues.apache.org/jira/browse/AURORA-1622
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Exposing ports to shell health checkers
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 890bf0c5d50d0022c044a37191a2e3145cc6340f 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 303972778baa04e9d7dd47fb208fe1427e779976 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 84f717fbf724c11863b4980fd2740dc23fe1404e 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 
> 
> Diff: https://reviews.apache.org/r/44486/diff/
> 
> 
> Testing
> ---
> 
> Unit and end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 44486: Exposing ports to shell health checkers

[Dmitriy Shirchenko]

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

(Updated March 8, 2016, 1:42 a.m.)


Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji.


Summary (updated)
-

Exposing ports to shell health checkers


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


Repository: aurora


Description (updated)
---

Exposing ports to shell health checkers


Diffs
-

  src/main/python/apache/aurora/common/health_check/shell.py 
890bf0c5d50d0022c044a37191a2e3145cc6340f 
  src/main/python/apache/aurora/executor/common/health_checker.py 
303972778baa04e9d7dd47fb208fe1427e779976 
  src/test/python/apache/aurora/common/health_check/test_shell.py 
84f717fbf724c11863b4980fd2740dc23fe1404e 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
9bebce8f5a26662f58075d7ce881a8bdacb2fe46 

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


Testing
---

Unit and end to end test.


Thanks,

Dmitriy Shirchenko