You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@aurora.apache.org by Martin Hrabovcin <mh...@gmail.com> on 2015/12/04 18:53:27 UTC

Re: Review Request 40922: Thermos: Add ability to forward process output to stdout

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/
-----------------------------------------------------------

(Updated Dec. 4, 2015, 9:53 a.m.)


Review request for Aurora.


Summary (updated)
-----------------

Thermos: Add ability to forward process output to stdout


Bugs: AURORA-1548
    https://issues.apache.org/jira/browse/AURORA-1548


Repository: aurora


Description
-------

(Thermos) Add ability to forward process output to stdout


Diffs
-----

  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0d02dc1 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 14e8b4b 
  src/main/python/apache/thermos/core/process.py fe95cb3 
  src/main/python/apache/thermos/core/runner.py f949f27 
  src/main/python/apache/thermos/runner/thermos_runner.py bd8cf7f 
  src/test/python/apache/thermos/core/test_process.py 5e6ad2f 

Diff: https://reviews.apache.org/r/40922/diff/


Testing
-------


Thanks,

Martin Hrabovcin

Re: Review Request 40922: Thermos: Add ability to forward process output to stdout

Posted by John Sirois <jo...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review110699
-----------------------------------------------------------

Ship it!


Master (fb8155d) is green with this patch.
  true

I will refresh this build result if you post a review containing "@ReviewBot retry"

- John Sirois


On Dec. 7, 2015, 1:17 p.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2015, 1:17 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Bugs: AURORA-1548
>     https://issues.apache.org/jira/browse/AURORA-1548
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch will provide way to **optionally** forward process output to stdout and stderr along with existing file 
> destinations. Its main use is to get logs to docker daemon when using Mesos Docker containerizer.
> 
> **What was changed:**
> 
> New command line option **log_to_std** is added to thermos_executor and thermos_runner. If thermos_executor gets this option it will pass it down to thermos_runner and runner will pass it down to **Process** class. New class **Tee** that mimicks behavior of unix tee utility that allows to split process output to file and stdout is core of this patch. subprocess.Popen in Process class used be opened with stdout/stderr file parameters. Now it will always get openend with subprocess.PIPE for stdout/stderr. Output of these pipes is constantly copied to Tee until process exits. If log_to_std is provided Tee will split output to expected files and stdout/stderr. Otherwise logs will be split to files and /dev/null.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0d02dc1 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 14e8b4b 
>   src/main/python/apache/thermos/core/process.py fe95cb3 
>   src/main/python/apache/thermos/core/runner.py f949f27 
>   src/main/python/apache/thermos/runner/thermos_runner.py bd8cf7f 
>   src/test/python/apache/thermos/core/test_process.py 5e6ad2f 
> 
> Diff: https://reviews.apache.org/r/40922/diff/
> 
> 
> Testing
> -------
> 
> Unit test coverage is provided for new functionality.
> 
> I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.
> 
> 
> Thanks,
> 
> Martin Hrabovcin
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Aurora ReviewBot <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review111765
-----------------------------------------------------------

Ship it!


Master (e75cdba) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Dec. 23, 2015, 9:33 a.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Dec. 23, 2015, 9:33 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Bugs: AURORA-1548
>     https://issues.apache.org/jira/browse/AURORA-1548
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch will provide way to **optionally** specify running process outputs destination. Implementation was built on top of https://reviews.apache.org/r/30695/
> 
> **What was changed:**
> 
> New `destination` parameter is available on global cluster level and also on each `Process` level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed` will split output to files and stream and finally `none` to discard any logs produced by running process.
> 
> 
> Diffs
> -----
> 
>   NEWS ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> Diff: https://reviews.apache.org/r/40922/diff/
> 
> 
> Testing
> -------
> 
> Unit test coverage is provided for new functionality.
> 
> I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.
> 
> 
> Thanks,
> 
> Martin Hrabovcin
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Martin Hrabovcin <mh...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/
-----------------------------------------------------------

(Updated Dec. 23, 2015, 9:33 a.m.)


Review request for Aurora.


Changes
-------

Fixed small bug after manual testing and minor docs changes. @ReviewBot retry


Bugs: AURORA-1548
    https://issues.apache.org/jira/browse/AURORA-1548


Repository: aurora


Description
-------

This patch will provide way to **optionally** specify running process outputs destination. Implementation was built on top of https://reviews.apache.org/r/30695/

**What was changed:**

New `destination` parameter is available on global cluster level and also on each `Process` level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed` will split output to files and stream and finally `none` to discard any logs produced by running process.


Diffs (updated)
-----

  NEWS ebfc3a6 
  docs/configuration-reference.md cf63cfa 
  docs/deploying-aurora-scheduler.md 73b2e04 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
  src/main/python/apache/thermos/config/schema_base.py 5552108 
  src/main/python/apache/thermos/core/process.py 8efdfdc 
  src/main/python/apache/thermos/core/runner.py 11c06a8 
  src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
  src/test/python/apache/thermos/core/test_process.py 261371d 

Diff: https://reviews.apache.org/r/40922/diff/


Testing
-------

Unit test coverage is provided for new functionality.

I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.


Thanks,

Martin Hrabovcin

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Aurora ReviewBot <wf...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review111620
-----------------------------------------------------------

Ship it!


Master (0813e3a) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Dec. 22, 2015, 4:52 p.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Bugs: AURORA-1548
>     https://issues.apache.org/jira/browse/AURORA-1548
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch will provide way to **optionally** specify running process outputs destination. Implementation was built on top of https://reviews.apache.org/r/30695/
> 
> **What was changed:**
> 
> New `destination` parameter is available on global cluster level and also on each `Process` level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed` will split output to files and stream and finally `none` to discard any logs produced by running process.
> 
> 
> Diffs
> -----
> 
>   NEWS ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> Diff: https://reviews.apache.org/r/40922/diff/
> 
> 
> Testing
> -------
> 
> Unit test coverage is provided for new functionality.
> 
> I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.
> 
> 
> Thanks,
> 
> Martin Hrabovcin
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Martin Hrabovcin <mh...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/#review111619
-----------------------------------------------------------


This is reworked patch that is more flexible, allows global cluster level configuration as well as process level configuration. I was following patterns in https://reviews.apache.org/r/30695/ as that patch is related.

@ReviewBot retry

- Martin Hrabovcin


On Dec. 22, 2015, 4:52 p.m., Martin Hrabovcin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40922/
> -----------------------------------------------------------
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Bugs: AURORA-1548
>     https://issues.apache.org/jira/browse/AURORA-1548
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch will provide way to **optionally** specify running process outputs destination. Implementation was built on top of https://reviews.apache.org/r/30695/
> 
> **What was changed:**
> 
> New `destination` parameter is available on global cluster level and also on each `Process` level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed` will split output to files and stream and finally `none` to discard any logs produced by running process.
> 
> 
> Diffs
> -----
> 
>   NEWS ebfc3a6 
>   docs/configuration-reference.md cf63cfa 
>   docs/deploying-aurora-scheduler.md 73b2e04 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
>   src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
>   src/main/python/apache/thermos/config/schema_base.py 5552108 
>   src/main/python/apache/thermos/core/process.py 8efdfdc 
>   src/main/python/apache/thermos/core/runner.py 11c06a8 
>   src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
>   src/test/python/apache/thermos/core/test_process.py 261371d 
> 
> Diff: https://reviews.apache.org/r/40922/diff/
> 
> 
> Testing
> -------
> 
> Unit test coverage is provided for new functionality.
> 
> I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.
> 
> 
> Thanks,
> 
> Martin Hrabovcin
> 
>

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Martin Hrabovcin <mh...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/
-----------------------------------------------------------

(Updated Dec. 22, 2015, 4:52 p.m.)


Review request for Aurora.


Changes
-------

Updated description to reflect new patch.


Bugs: AURORA-1548
    https://issues.apache.org/jira/browse/AURORA-1548


Repository: aurora


Description (updated)
-------

This patch will provide way to **optionally** specify running process outputs destination. Implementation was built on top of https://reviews.apache.org/r/30695/

**What was changed:**

New `destination` parameter is available on global cluster level and also on each `Process` level. Possible options are `file` (default), `stream` to parent process stdout/stderr, `mixed` will split output to files and stream and finally `none` to discard any logs produced by running process.


Diffs
-----

  NEWS ebfc3a6 
  docs/configuration-reference.md cf63cfa 
  docs/deploying-aurora-scheduler.md 73b2e04 
  src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b 
  src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2 
  src/main/python/apache/thermos/config/schema_base.py 5552108 
  src/main/python/apache/thermos/core/process.py 8efdfdc 
  src/main/python/apache/thermos/core/runner.py 11c06a8 
  src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a 
  src/test/python/apache/thermos/core/test_process.py 261371d 

Diff: https://reviews.apache.org/r/40922/diff/


Testing
-------

Unit test coverage is provided for new functionality.

I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.


Thanks,

Martin Hrabovcin

Re: Review Request 40922: Thermos: Add ability to specify process outputs destination

Posted by Martin Hrabovcin <mh...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40922/
-----------------------------------------------------------

(Updated Dec. 22, 2015, 4:47 p.m.)

Review request for Aurora.

Summary (updated)
-----------------

Thermos: Add ability to specify process outputs destination

Bugs: AURORA-1548
https://issues.apache.org/jira/browse/AURORA-1548

Repository: aurora

Description
-------

This patch will provide way to **optionally** forward process output to stdout and stderr along with existing file
destinations. Its main use is to get logs to docker daemon when using Mesos Docker containerizer.

**What was changed:**

New command line option **log_to_std** is added to thermos_executor and thermos_runner. If thermos_executor gets this option it will pass it down to thermos_runner and runner will pass it down to **Process** class. New class **Tee** that mimicks behavior of unix tee utility that allows to split process output to file and stdout is core of this patch. subprocess.Popen in Process class used be opened with stdout/stderr file parameters. Now it will always get openend with subprocess.PIPE for stdout/stderr. Output of these pipes is constantly copied to Tee until process exits. If log_to_std is provided Tee will split output to expected files and stdout/stderr. Otherwise logs will be split to files and /dev/null.

Diffs
-----

NEWS ebfc3a6
docs/configuration-reference.md cf63cfa
docs/deploying-aurora-scheduler.md 73b2e04
src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 7b7ef4b
src/main/python/apache/aurora/executor/thermos_task_runner.py 25fcca2
src/main/python/apache/thermos/config/schema_base.py 5552108
src/main/python/apache/thermos/core/process.py 8efdfdc
src/main/python/apache/thermos/core/runner.py 11c06a8
src/main/python/apache/thermos/runner/thermos_runner.py a36bd2a
src/test/python/apache/thermos/core/test_process.py 261371d

Diff: https://reviews.apache.org/r/40922/diff/

Testing
-------

Unit test coverage is provided for new functionality.

I did also manual testing with mesos/docker and I made sure that logs are being written to expected files and also same output gets to docker daemon.

Thanks,

Martin Hrabovcin