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

Reply via email to