You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/08/16 20:11:45 UTC
Review Request 61697: Included nested command checks output in the
executor logs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/
-----------------------------------------------------------
Review request for mesos, Alexander Rukletsov and Vinod Kone.
Bugs: MESOS-7861
https://issues.apache.org/jira/browse/MESOS-7861
Repository: mesos
Description
-------
This patch makes the default executor include the output of the COMMAND
(health) checks in its logs.
Diffs
-----
src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
Diff: https://reviews.apache.org/r/61697/diff/1/
Testing
-------
Manual tests.
Thanks,
Gastón Kleiman
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Aug. 16, 2017, 11:03 p.m., Greg Mann wrote:
> > src/checks/checker_process.cpp
> > Lines 143-144 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line143>
> >
> > "returns the merged stdout and stderr": is this true? Looks like you only use STDOUT type events here?
I'm also not sure we want to merge stdout and stderr from a check here. Maybe return a tuple here so that we can print stdout and stderr separately?
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183071
-----------------------------------------------------------
On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 8:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/2/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 16, 2017, 11:03 p.m., Greg Mann wrote:
> > src/checks/checker_process.cpp
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line145>
> >
> > s/It/This function/
Fixed, I guess that we'll wnat to change https://github.com/apache/mesos/blob/4d0e692e72e56e827d2136b78311db0b72ada6a2/src/tests/api_tests.cpp#L3036-L3038 as well?
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183071
-----------------------------------------------------------
On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 11:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/3/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183071
-----------------------------------------------------------
src/checks/checker_process.cpp
Lines 143-144 (patched)
<https://reviews.apache.org/r/61697/#comment259076>
"returns the merged stdout and stderr": is this true? Looks like you only use STDOUT type events here?
src/checks/checker_process.cpp
Lines 145 (patched)
<https://reviews.apache.org/r/61697/#comment259074>
s/It/This function/
src/checks/checker_process.cpp
Lines 153 (patched)
<https://reviews.apache.org/r/61697/#comment259079>
So this fuction expects a stream with ContentType of protobuf; could you call this out in the comment above the function declaration?
src/checks/checker_process.cpp
Lines 543-545 (original), 581-583 (patched)
<https://reviews.apache.org/r/61697/#comment259080>
Could you extend this comment with TODO explaining why we want to switch to a streaming response (for logging of hung checks), with a reference to a relevant JIRA?
- Greg Mann
On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 8:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/2/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 17, 2017, 2:22 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 630-631 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line630>
> >
> > I assume line feed and carriage return characters are included in the output. In this case, how a reader can distinguish between executor output and check output? Is it because check output lines will not be prepended by a timestamp? Have you tested it and checked that the output is sane?
Exaclty, the check output lines will not be prepended with anything, here's an example output with the health check command `ls >&2 && echo 'foo bar baz' && true`:
```
I0818 16:24:04.182832 36687 checker_process.cpp:639] Output of the COMMAND health check for task 'test-task-id' (stdout):
foo bar baz
I0818 16:24:04.182896 36687 checker_process.cpp:643] Output of the COMMAND health check for task 'test-task-id' (stderr):
containers
stderr
stdout
```
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183121
-----------------------------------------------------------
On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 11:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/3/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 17, 2017, 2:22 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 144-145 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line144>
> >
> > Insert a blank line here.
Ditto https://github.com/apache/mesos/blob/4d0e692e72e56e827d2136b78311db0b72ada6a2/src/tests/api_tests.cpp#L3036-L3038.
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183121
-----------------------------------------------------------
On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 11:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/3/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Aug. 17, 2017, 2:22 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 855-858 (original), 903-906 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line903>
> >
> > While we're on it, let's consistently print out HTTP and TCP check output. Instead of including it into the failure. As a separate patch, please.
Do you mean that we should always print the output of the HTTP/TCP commands?
We're including it in the Failure here, because in this case curl returned something unexpected, otherwise I don't think it makes much sense to print the exit status and output of curl.
Anyway, I made the output more consistent here: https://reviews.apache.org/r/61766/. Let's continue this conversation there.
> On Aug. 17, 2017, 2:22 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 999-1009 (original), 1047-1057 (patched)
> > <https://reviews.apache.org/r/61697/diff/2/?file=1798839#file1798839line1047>
> >
> > Ditto. Let's make the output consistent.
Ditto https://reviews.apache.org/r/61766/.
- Gastón
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183121
-----------------------------------------------------------
On Aug. 18, 2017, 11:20 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 11:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/3/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183121
-----------------------------------------------------------
src/checks/checker_process.cpp
Lines 144-145 (patched)
<https://reviews.apache.org/r/61697/#comment259125>
Insert a blank line here.
src/checks/checker_process.cpp
Lines 629 (patched)
<https://reviews.apache.org/r/61697/#comment259137>
Why not always printing the output? So that a reader clearly sees that there were no output and no errors.
src/checks/checker_process.cpp
Lines 630 (patched)
<https://reviews.apache.org/r/61697/#comment259127>
Maybe `std::endl` instead of `\n`? `\n` is not Windows-friendly.
src/checks/checker_process.cpp
Lines 630-631 (patched)
<https://reviews.apache.org/r/61697/#comment259129>
I assume line feed and carriage return characters are included in the output. In this case, how a reader can distinguish between executor output and check output? Is it because check output lines will not be prepended by a timestamp? Have you tested it and checked that the output is sane?
src/checks/checker_process.cpp
Lines 855-858 (original), 903-906 (patched)
<https://reviews.apache.org/r/61697/#comment259144>
While we're on it, let's consistently print out HTTP and TCP check output. Instead of including it into the failure. As a separate patch, please.
src/checks/checker_process.cpp
Lines 999-1009 (original), 1047-1057 (patched)
<https://reviews.apache.org/r/61697/#comment259146>
Ditto. Let's make the output consistent.
- Alexander Rukletsov
On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 8:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/2/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183064
-----------------------------------------------------------
Bad patch!
Reviews applied: [61697]
Failed command: python support/apply-reviews.py -n -r 61697
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 417, in <module>
main()
File "support/apply-reviews.py", line 412, in main
reviewboard(options)
File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 25: ordinal not in range(128)
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/239/console
- Mesos Reviewbot Windows
On Aug. 16, 2017, 8:17 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 8:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/2/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/
-----------------------------------------------------------
(Updated Aug. 16, 2017, 8:17 p.m.)
Review request for mesos, Alexander Rukletsov and Vinod Kone.
Changes
-------
Style changes.
Bugs: MESOS-7861
https://issues.apache.org/jira/browse/MESOS-7861
Repository: mesos
Description
-------
This patch makes the default executor include the output of the COMMAND
(health) checks in its logs.
Diffs (updated)
-----
src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
Diff: https://reviews.apache.org/r/61697/diff/2/
Changes: https://reviews.apache.org/r/61697/diff/1-2/
Testing
-------
Manual tests.
Thanks,
Gastón Kleiman
Re: Review Request 61697: Included nested command checks output in the
executor logs.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61697/#review183061
-----------------------------------------------------------
Bad patch!
Reviews applied: [61697]
Failed command: python support/apply-reviews.py -n -r 61697
Error:
Traceback (most recent call last):
File "support/apply-reviews.py", line 417, in <module>
main()
File "support/apply-reviews.py", line 412, in main
reviewboard(options)
File "support/apply-reviews.py", line 402, in reviewboard
apply_review(options)
File "support/apply-reviews.py", line 160, in apply_review
commit_patch(options)
File "support/apply-reviews.py", line 268, in commit_patch
shell(cmd, options['dry_run'])
File "support/apply-reviews.py", line 144, in shell
error_code = subprocess.call(command, stderr=subprocess.STDOUT, shell=True)
File "C:\Python27\lib\subprocess.py", line 168, in call
return Popen(*popenargs, **kwargs).wait()
File "C:\Python27\lib\subprocess.py", line 390, in __init__
errread, errwrite)
File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
args = '{} /c "{}"'.format (comspec, args)
UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in position 25: ordinal not in range(128)
Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/238/console
- Mesos Reviewbot Windows
On Aug. 16, 2017, 8:11 p.m., Gastón Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61697/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 8:11 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
>
>
> Bugs: MESOS-7861
> https://issues.apache.org/jira/browse/MESOS-7861
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch makes the default executor include the output of the COMMAND
> (health) checks in its logs.
>
>
> Diffs
> -----
>
> src/checks/checker_process.cpp 30dda0e6efca31aa6b9cd4f753f96b979717ab2e
>
>
> Diff: https://reviews.apache.org/r/61697/diff/1/
>
>
> Testing
> -------
>
> Manual tests.
>
>
> Thanks,
>
> Gastón Kleiman
>
>