aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zameer Manji <zma...@apache.org>
Subject Re: Review Request 44486: Exposing ports to shell health checkers
Date Tue, 08 Mar 2016 20:04:36 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message