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