aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Shirchenko <cald...@gmail.com>
Subject Re: Review Request 44486: Exposing ports to shell health checkers
Date Tue, 08 Mar 2016 20:12:14 GMT
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 <zmanji@apache.org> 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 <https://issues.apache.org/jira/browse/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 <https://reviews.apache.org/r/44486/diff/>
>

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