You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2015/12/12 01:39:35 UTC
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 11, 2015, 4:39 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Rename from ExecutorLogger to ContainerLogger. Removed `getLogURL` for now, because the existing code does not exercise it.
Summary (updated)
-----------------
Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description (updated)
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 21, 2015, 4:49 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Change the restriction on `IO::PIPE` into a compile-time restriction.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 20, 2015, 6:21 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 85-93
> > <https://reviews.apache.org/r/41002/diff/10/?file=1173564#file1173564line85>
> >
> > I'm a little confused. Who executes this? The comment makes it read like there is some particular code that executes this, which makes me wonder why it needs to be a static function on `ContainerLogger`?
> >
> > And why don't we want `SubprocessInfo::out/err` to use pipes? I thought that was the whole point?
> >
> > Finally, killing the agent seems a little bit harsh. Couldn't the container just get killed?
This is meant to enforce the `NOTE` in `SubprocessInfo` about how the module may not specify `Subprocess::IO::PIPE()` as part of the return value.
I talked with Joris about either making this a compile-time or run-time check, and we decided that a compile-time check would be somewhat distracting compared to a `validate` function like this. (We would need to add a new class that provides just `Subprocess::FD` and `Subprocess::PATH`.)
This function will exit on failure because it indicates that the chosen `ContainerLogger` breaks that assumption.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review111436
-----------------------------------------------------------
On Dec. 20, 2015, 4:39 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2015, 4:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 20, 2015, 6:21 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 85-93
> > <https://reviews.apache.org/r/41002/diff/10/?file=1173564#file1173564line85>
> >
> > I'm a little confused. Who executes this? The comment makes it read like there is some particular code that executes this, which makes me wonder why it needs to be a static function on `ContainerLogger`?
> >
> > And why don't we want `SubprocessInfo::out/err` to use pipes? I thought that was the whole point?
> >
> > Finally, killing the agent seems a little bit harsh. Couldn't the container just get killed?
>
> Joseph Wu wrote:
> This is meant to enforce the `NOTE` in `SubprocessInfo` about how the module may not specify `Subprocess::IO::PIPE()` as part of the return value.
>
> I talked with Joris about either making this a compile-time or run-time check, and we decided that a compile-time check would be somewhat distracting compared to a `validate` function like this. (We would need to add a new class that provides just `Subprocess::FD` and `Subprocess::PATH`.)
>
> This function will exit on failure because it indicates that the chosen `ContainerLogger` breaks that assumption.
Changed to a compile-time restriction, as discussed.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review111436
-----------------------------------------------------------
On Dec. 21, 2015, 4:49 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 4:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review111436
-----------------------------------------------------------
include/mesos/slave/container_logger.hpp (lines 85 - 93)
<https://reviews.apache.org/r/41002/#comment171609>
I'm a little confused. Who executes this? The comment makes it read like there is some particular code that executes this, which makes me wonder why it needs to be a static function on `ContainerLogger`?
And why don't we want `SubprocessInfo::out/err` to use pipes? I thought that was the whole point?
Finally, killing the agent seems a little bit harsh. Couldn't the container just get killed?
- Benjamin Hindman
On Dec. 21, 2015, 12:39 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 21, 2015, 12:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 20, 2015, 4:39 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Remove `subprocess` arguments except for pipes. Added `validate` function.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 17, 2015, 3:46 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Comment changes, per BenH's comments.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 73-75
> > <https://reviews.apache.org/r/41002/diff/8/?file=1165100#file1165100line73>
> >
> > Can we document what the failure case of using an IO::PIPE means? As in, what if someone specifies an IO::PIPE? What breaks down?
>
> Joseph Wu wrote:
> Would it be reasonable to `ABORT` the agent if the module specifies a `PIPE`? Or perhaps make this a compile-time restriction (i.e. by introducing a subclass of `Subprocess::IO`).
>
> (Note: The check for `PIPE` isn't implemented yet.)
Added `ContainerLogger::validate`.
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, line 128
> > <https://reviews.apache.org/r/41002/diff/8/?file=1165100#file1165100line128>
> >
> > What's the long term plan for this? Will we have multiple container loggers running simultaneously at some point?
> >
> > I'd like you to capture a TODO here to follow up with the semantics on executors that can't be recovered. And let's roll this into the next phase so that we have consistent semantics here.
>
> Joseph Wu wrote:
> For Docker command executors, there will be a container logger running alongside the executor. But it will only ever spawn one task (because the executor will refuse >1 task).
>
> In general, this means we could:
>
> - Launch an executor that logs to the sandbox,
> - Restart the agent with a special container logger (say, to syslog),
> - Have the recovered executor continue logging to the sandbox, while new executors log to syslog.
Note: We decided to remove the ContainerLogger from the executor (so now it only lives in the Containerizer, and only one module instance will exist per containerizer.)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110826
-----------------------------------------------------------
On Dec. 20, 2015, 4:39 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2015, 4:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > Only high-level comment is that the _container_ logger talks a lot about executor's and tasks, I need to look into some of the upcoming reviews, but why isn't it sufficient to only talk about containers?
I wanted to be explicit about the scope of the module. i.e. All executors/tasks run in containers, but not all containers are executors/tasks.
Changed to use "container" in the comments.
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 43-44
> > <https://reviews.apache.org/r/41002/diff/8/?file=1165100#file1165100line43>
> >
> > Make this part a TODO?
I'll add a TODO to expose some sort of message or URL in the Mesos web UI.
The module still needs to provide a log-reading interface (because write-only logs aren't very useful). But Mesos won't know how to read said logs.
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 73-75
> > <https://reviews.apache.org/r/41002/diff/8/?file=1165100#file1165100line73>
> >
> > Can we document what the failure case of using an IO::PIPE means? As in, what if someone specifies an IO::PIPE? What breaks down?
Would it be reasonable to `ABORT` the agent if the module specifies a `PIPE`? Or perhaps make this a compile-time restriction (i.e. by introducing a subclass of `Subprocess::IO`).
(Note: The check for `PIPE` isn't implemented yet.)
> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, line 128
> > <https://reviews.apache.org/r/41002/diff/8/?file=1165100#file1165100line128>
> >
> > What's the long term plan for this? Will we have multiple container loggers running simultaneously at some point?
> >
> > I'd like you to capture a TODO here to follow up with the semantics on executors that can't be recovered. And let's roll this into the next phase so that we have consistent semantics here.
For Docker command executors, there will be a container logger running alongside the executor. But it will only ever spawn one task (because the executor will refuse >1 task).
In general, this means we could:
- Launch an executor that logs to the sandbox,
- Restart the agent with a special container logger (say, to syslog),
- Have the recovered executor continue logging to the sandbox, while new executors log to syslog.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110826
-----------------------------------------------------------
On Dec. 17, 2015, 3:46 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2015, 3:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110826
-----------------------------------------------------------
Ship it!
Only high-level comment is that the _container_ logger talks a lot about executor's and tasks, I need to look into some of the upcoming reviews, but why isn't it sufficient to only talk about containers?
include/mesos/slave/container_logger.hpp (line 40)
<https://reviews.apache.org/r/41002/#comment171027>
s/executors/containers/
s/of tasks launched by default executors/of tasks launched without an executor (that implicitly use the command executor)/
include/mesos/slave/container_logger.hpp (lines 43 - 44)
<https://reviews.apache.org/r/41002/#comment171028>
Make this part a TODO?
include/mesos/slave/container_logger.hpp (lines 73 - 75)
<https://reviews.apache.org/r/41002/#comment171029>
Can we document what the failure case of using an IO::PIPE means? As in, what if someone specifies an IO::PIPE? What breaks down?
include/mesos/slave/container_logger.hpp (line 125)
<https://reviews.apache.org/r/41002/#comment171030>
s/Error/Failure/
s/error/failure/
include/mesos/slave/container_logger.hpp (line 128)
<https://reviews.apache.org/r/41002/#comment171031>
What's the long term plan for this? Will we have multiple container loggers running simultaneously at some point?
I'd like you to capture a TODO here to follow up with the semantics on executors that can't be recovered. And let's roll this into the next phase so that we have consistent semantics here.
include/mesos/slave/container_logger.hpp (line 135)
<https://reviews.apache.org/r/41002/#comment171032>
s/launches an executor or a default executor launches a task/creates a container/
include/mesos/slave/container_logger.hpp (line 139)
<https://reviews.apache.org/r/41002/#comment171033>
s/an executor or task/container/
include/mesos/slave/container_logger.hpp (line 155)
<https://reviews.apache.org/r/41002/#comment170823>
Kill newline.
- Benjamin Hindman
On Dec. 15, 2015, 8:38 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 8:38 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 12:38 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Remove `options` in favor of module parameters.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 11:24 a.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Comment tweaks. Changed `options` to an `Option<string>`. Moved call to `initialize` into `create`.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 14, 2015, 8:02 p.m., Timothy Chen wrote:
> > include/mesos/slave/container_logger.hpp, line 120
> > <https://reviews.apache.org/r/41002/diff/6/?file=1163811#file1163811line120>
> >
> > What `captureOutput` are you referring to here?
Oops :)
This was renamed (twice) to `prepare`.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110394
-----------------------------------------------------------
On Dec. 14, 2015, 3:46 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 3:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110394
-----------------------------------------------------------
include/mesos/slave/container_logger.hpp (line 120)
<https://reviews.apache.org/r/41002/#comment170223>
What `captureOutput` are you referring to here?
- Timothy Chen
On Dec. 14, 2015, 11:46 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110393
-----------------------------------------------------------
include/mesos/slave/container_logger.hpp (line 111)
<https://reviews.apache.org/r/41002/#comment170221>
Extra space in the comments
- Timothy Chen
On Dec. 14, 2015, 11:46 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 11:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/
-----------------------------------------------------------
(Updated Dec. 14, 2015, 3:46 p.m.)
Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
Changes
-------
Tweaked some comments. Change `ExecutorID` to `ExecutorInfo`, so as to pass more information to the module.
Bugs: MESOS-4087
https://issues.apache.org/jira/browse/MESOS-4087
Repository: mesos
Description
-------
The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
Diffs (updated)
-----
include/mesos/slave/container_logger.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/41002/diff/
Testing
-------
This is added to the Makefile later in the review chain.
Thanks,
Joseph Wu
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 133
> > <https://reviews.apache.org/r/41002/diff/5/?file=1160533#file1160533line133>
> >
> > I'm probably missing something here but why all the "executor or task" and "executor/task" references? Mesos doesn't spawn processes for tasks, it spawns processes for executors and then executors execute tasks (using threads, child procs, containers, whatever). Which means that this module is invoked for executors only -- not tasks, correct? The current docs read in a way that's misleading.
This logging module would would be executor-only for custom executors (the original name of the class was "ExecutorLogger"). However, two of the executors (and their tasks) provided by Mesos will be affected by the logging module:
* The command executor => spawns as an executor (process), but acts like a task.
* `mesos-docker-executor` => spawns as a process (or in a Docker container). Mesos launches command tasks (Docker containers) using this, and the resulting stdout/stderr goes into the sandbox currently.
The only logging Mesos does not necessarily control is tasks launched by custom executors.
> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 157
> > <https://reviews.apache.org/r/41002/diff/5/?file=1160533#file1160533line157>
> >
> > It may be useful here to include the executor labels here so that, for example, frameworks could advertise additional information to logging modules. for example, things like additional log files (LOGFILEx=...), or a logs dir (LOGSDIRx=xyz) to monitor.
> >
> > It costs very little to include the executor labels here in the API and opens the door to more powerful modules down the road w/o needing to change the API to pass in additional context.
(Followed up in the design doc:)
https://docs.google.com/document/d/1Y8Sh_1eya0xI-djPmBZvqB-IVp1aaYSl-HCI6-2u5w4/
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110113
-----------------------------------------------------------
On Dec. 11, 2015, 4:39 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 4:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 157
> > <https://reviews.apache.org/r/41002/diff/5/?file=1160533#file1160533line157>
> >
> > It may be useful here to include the executor labels here so that, for example, frameworks could advertise additional information to logging modules. for example, things like additional log files (LOGFILEx=...), or a logs dir (LOGSDIRx=xyz) to monitor.
> >
> > It costs very little to include the executor labels here in the API and opens the door to more powerful modules down the road w/o needing to change the API to pass in additional context.
>
> Joseph Wu wrote:
> (Followed up in the design doc:)
> https://docs.google.com/document/d/1Y8Sh_1eya0xI-djPmBZvqB-IVp1aaYSl-HCI6-2u5w4/
I'll change `ExecutorID` to `ExecutorInfo`.
> On Dec. 12, 2015, 10:49 p.m., James DeFelice wrote:
> > include/mesos/slave/container_logger.hpp, line 133
> > <https://reviews.apache.org/r/41002/diff/5/?file=1160533#file1160533line133>
> >
> > I'm probably missing something here but why all the "executor or task" and "executor/task" references? Mesos doesn't spawn processes for tasks, it spawns processes for executors and then executors execute tasks (using threads, child procs, containers, whatever). Which means that this module is invoked for executors only -- not tasks, correct? The current docs read in a way that's misleading.
>
> Joseph Wu wrote:
> This logging module would would be executor-only for custom executors (the original name of the class was "ExecutorLogger"). However, two of the executors (and their tasks) provided by Mesos will be affected by the logging module:
>
> * The command executor => spawns as an executor (process), but acts like a task.
> * `mesos-docker-executor` => spawns as a process (or in a Docker container). Mesos launches command tasks (Docker containers) using this, and the resulting stdout/stderr goes into the sandbox currently.
>
> The only logging Mesos does not necessarily control is tasks launched by custom executors.
Tweaked some comments to make the distinction. i.e. Some tasks are involved, but only the ones that Mesos (not custom executor) launches.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110113
-----------------------------------------------------------
On Dec. 14, 2015, 3:46 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 3:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 41002: Logger Module: Introduce the ContainerLogger
interface for logging the stdout/stderr of executors and tasks.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41002/#review110113
-----------------------------------------------------------
include/mesos/slave/container_logger.hpp (line 133)
<https://reviews.apache.org/r/41002/#comment169895>
I'm probably missing something here but why all the "executor or task" and "executor/task" references? Mesos doesn't spawn processes for tasks, it spawns processes for executors and then executors execute tasks (using threads, child procs, containers, whatever). Which means that this module is invoked for executors only -- not tasks, correct? The current docs read in a way that's misleading.
include/mesos/slave/container_logger.hpp (line 157)
<https://reviews.apache.org/r/41002/#comment169894>
It may be useful here to include the executor labels here so that, for example, frameworks could advertise additional information to logging modules. for example, things like additional log files (LOGFILEx=...), or a logs dir (LOGSDIRx=xyz) to monitor.
It costs very little to include the executor labels here in the API and opens the door to more powerful modules down the road w/o needing to change the API to pass in additional context.
- James DeFelice
On Dec. 12, 2015, 12:39 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2015, 12:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, and Thomas Rampelberg.
>
>
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The container logger is an agent module whose job is to take an executor or task's stdout/stderr, and pipe it to some sink.
>
> Existing executor and task stdout/stderr are piped into files ("stdout" and "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more sophisticated logging solutions.
>
>
> Diffs
> -----
>
> include/mesos/slave/container_logger.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/41002/diff/
>
>
> Testing
> -------
>
> This is added to the Makefile later in the review chain.
>
>
> Thanks,
>
> Joseph Wu
>
>