You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/06/21 06:57:57 UTC
Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
Bugs: MESOS-1404
https://issues.apache.org/jira/browse/MESOS-1404
Repository: mesos-git
Description
-------
See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
Also, we leverage the new Subprocess interface to handle redirections.
(BTW: I fixed a few 70 char width comments issue when I doing the refactor).
Diffs
-----
src/Makefile.am fd2c80f
src/slave/containerizer/launcher.hpp 63c0cb6
src/slave/containerizer/launcher.cpp c43396a
src/slave/containerizer/linux_launcher.hpp 60b401f
src/slave/containerizer/linux_launcher.cpp 85c74f0
src/slave/containerizer/mesos_containerizer.hpp 1f5908a
src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
src/tests/isolator_tests.cpp 0bbec09
Diff: https://reviews.apache.org/r/22852/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 840
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line840>
> >
> > Could we also push checkpointing the pid file into the subcommand as an option too?
>
> Jie Yu wrote:
> Added a TODO. Again, let's do the refactor in a new patch since this patch is already quite complicated.
>
> Jie Yu wrote:
> Sorry, for this, I don't understand why the checkpointing should be done by the subprocess, not the slave?
>
> Ian Downes wrote:
> We need to correctly handle the slave dying soon after forking the child, i.e., if the child process is running we must have the pid checkpointed so we know about it when we restart.
>
> The current code uses a pipe to synchronize. The child only execs after checkpointing is successful; if the slave exits the pipe is closed and the child exits.
>
> If we have the child checkpoint itself then we eliminate all of this.
Aha, that makes sense! Will add a TODO.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Ian Downes <ia...@gmail.com>.
> On June 23, 2014, 11:02 a.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 840
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line840>
> >
> > Could we also push checkpointing the pid file into the subcommand as an option too?
>
> Jie Yu wrote:
> Added a TODO. Again, let's do the refactor in a new patch since this patch is already quite complicated.
>
> Jie Yu wrote:
> Sorry, for this, I don't understand why the checkpointing should be done by the subprocess, not the slave?
We need to correctly handle the slave dying soon after forking the child, i.e., if the child process is running we must have the pid checkpointed so we know about it when we restart.
The current code uses a pipe to synchronize. The child only execs after checkpointing is successful; if the slave exits the pipe is closed and the child exits.
If we have the child checkpoint itself then we eliminate all of this.
> On June 23, 2014, 11:02 a.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 907
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line907>
> >
> > Is it cleaner to pass in the environment variables as well since we're specifying the command, working directory, and user.
> >
> > In the future we should use a clean environment, can we add this as a TODO?
>
> Jie Yu wrote:
> I am not able to get your first part? Can you elaborate?
We're specifying command/cwd/user but inheriting the environment. I think making the environment explicit is preferred.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 20, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 20, 2014, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 840
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line840>
> >
> > Could we also push checkpointing the pid file into the subcommand as an option too?
>
> Jie Yu wrote:
> Added a TODO. Again, let's do the refactor in a new patch since this patch is already quite complicated.
Sorry, for this, I don't understand why the checkpointing should be done by the subprocess, not the slave?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/linux_launcher.cpp, line 291
> > <https://reviews.apache.org/r/22852/diff/2/?file=614738#file614738line291>
> >
> > Suggestion: move the cgroups::assign as an optional, linux specific flag to the subcommand and have the child do the assign. This would obviate the sync in the linux launcher and all of the childSetup if the setsid is moved too.
That's a pretty big change, can we do that in a new review as this patch is already quite big. I'll add a TODO here.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/launcher.cpp, line 95
> > <https://reviews.apache.org/r/22852/diff/2/?file=614736#file614736line95>
> >
> > Since this is common between the launchers, what about moving this into the subcommand?
See my comments below. I'll add a TODO.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 840
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line840>
> >
> > Could we also push checkpointing the pid file into the subcommand as an option too?
Added a TODO. Again, let's do the refactor in a new patch since this patch is already quite complicated.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 907
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line907>
> >
> > Is it cleaner to pass in the environment variables as well since we're specifying the command, working directory, and user.
> >
> > In the future we should use a clean environment, can we add this as a TODO?
I am not able to get your first part? Can you elaborate?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 907
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line907>
> >
> > Is it cleaner to pass in the environment variables as well since we're specifying the command, working directory, and user.
> >
> > In the future we should use a clean environment, can we add this as a TODO?
>
> Jie Yu wrote:
> I am not able to get your first part? Can you elaborate?
>
> Ian Downes wrote:
> We're specifying command/cwd/user but inheriting the environment. I think making the environment explicit is preferred.
Added a TODO.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 23, 2014, 6:02 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 951
> > <https://reviews.apache.org/r/22852/diff/2/?file=614740#file614740line951>
> >
> > This formatting seems strange?
80 char limit:) No better way:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46414
-----------------------------------------------------------
Now that we've got the Subcommand to execute the executor I think we can move code out and simplify the Launchers and MesosContainerizer.
- setsid
- cgroups::assign for freezer
- checkpointing the pid
Pushing these down would, I think, let us remove the two cases of using pipes to synchronize with the slave. Details as issues.
src/slave/containerizer/launcher.cpp
<https://reviews.apache.org/r/22852/#comment81798>
Since this is common between the launchers, what about moving this into the subcommand?
src/slave/containerizer/linux_launcher.cpp
<https://reviews.apache.org/r/22852/#comment81799>
Suggestion: move the cgroups::assign as an optional, linux specific flag to the subcommand and have the child do the assign. This would obviate the sync in the linux launcher and all of the childSetup if the setsid is moved too.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment81801>
Could we also push checkpointing the pid file into the subcommand as an option too?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment81802>
Is it cleaner to pass in the environment variables as well since we're specifying the command, working directory, and user.
In the future we should use a clean environment, can we add this as a TODO?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment81803>
This formatting seems strange?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment81804>
Kill extra newline.
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment81805>
Kill extra whitespace: s/s ./s./
- Ian Downes
On June 20, 2014, 9:57 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 20, 2014, 9:57 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 24, 2014, 7:25 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos_containerizer.hpp, lines 248-249
> > <https://reviews.apache.org/r/22852/diff/2/?file=614739#file614739line248>
> >
> > why snakecase?
Because we use strings::lower for all flags. So it's either piperead or pipe_read.
Also, this is consistent with all our flags (e.g. in src/slave/flags.hpp).
> On June 24, 2014, 7:25 p.m., Vinod Kone wrote:
> > src/slave/containerizer/launcher.hpp, lines 49-63
> > <https://reviews.apache.org/r/22852/diff/2/?file=614735#file614735line49>
> >
> > From here, it's not clear to me when 'setup' will be called if you are forking the binary at 'path'.
Added a comment.
> On June 24, 2014, 7:25 p.m., Vinod Kone wrote:
> > src/slave/containerizer/launcher.cpp, lines 95-98
> > <https://reviews.apache.org/r/22852/diff/2/?file=614736#file614736line95>
> >
> > Can setsid() be part of the 'setup'?
We will eventually move this to the subcommand. I've already added a TODO here.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46563
-----------------------------------------------------------
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46563
-----------------------------------------------------------
lgtm modulo comments. i'll let @iandownes shepherd this to completion.
src/slave/containerizer/launcher.hpp
<https://reviews.apache.org/r/22852/#comment82000>
From here, it's not clear to me when 'setup' will be called if you are forking the binary at 'path'.
src/slave/containerizer/launcher.cpp
<https://reviews.apache.org/r/22852/#comment82009>
Can setsid() be part of the 'setup'?
src/slave/containerizer/linux_launcher.cpp
<https://reviews.apache.org/r/22852/#comment82012>
Is it safe to call this in a subprocess?
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/22852/#comment82013>
why snakecase?
src/slave/containerizer/mesos_containerizer.hpp
<https://reviews.apache.org/r/22852/#comment82015>
s/preparations/commands/
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment82014>
s/preparations/commands/
- Vinod Kone
On June 21, 2014, 4:57 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 21, 2014, 4:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am fd2c80f
> src/slave/containerizer/launcher.hpp 63c0cb6
> src/slave/containerizer/launcher.cpp c43396a
> src/slave/containerizer/linux_launcher.hpp 60b401f
> src/slave/containerizer/linux_launcher.cpp 85c74f0
> src/slave/containerizer/mesos_containerizer.hpp 1f5908a
> src/slave/containerizer/mesos_containerizer.cpp 61c0a8d
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 25, 2014, 4:56 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer_helper.cpp, line 22
> > <https://reviews.apache.org/r/22852/diff/3/?file=616558#file616558line22>
> >
> > Organizationally, I would have expected the subcommands to be declared here, not in mesos_containerizer.hpp.
> >
> > Then mesos_containerizer.cpp could include this header, which makes sense since mesos_containerizer.hpp is invoking mesos-containerizer-helper.
> >
> > Moreover, I would have s/MesosContainerizerLaunch/MesosContainerizerHelperLaunch/ to capture that this is a subcommand of mesos-containerizer-helper.
Created src/slave/containerizer/mesos directory. Moved mesos_containerizer.h|cpp to this directory. Created a launch.hpp and main.cpp in ths directory.
> On June 25, 2014, 4:56 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 1022
> > <https://reviews.apache.org/r/22852/diff/3/?file=616557#file616557line1022>
> >
> > I thought we did os::su before we launched the preparation commands before. I'm happy to see it this way because I think those commands should be able to run with the same privileges as the mesos-slave (i.e., root), but just wanted to call out these semantics.
Added a comment.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46644
-----------------------------------------------------------
On June 25, 2014, 6:39 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 6:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am 861aad2
> src/slave/containerizer/launcher.hpp 835d7a9
> src/slave/containerizer/launcher.cpp 2b13d1f
> src/slave/containerizer/linux_launcher.hpp 622810c
> src/slave/containerizer/linux_launcher.cpp 7ebccb4
> src/slave/containerizer/mesos_containerizer.hpp 21affae
> src/slave/containerizer/mesos_containerizer.cpp 76c101c
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46644
-----------------------------------------------------------
src/slave/containerizer/launcher.hpp
<https://reviews.apache.org/r/22852/#comment82180>
Thanks for cleaning up all the comment wrapping throughout!!!
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/22852/#comment82176>
I thought we did os::su before we launched the preparation commands before. I'm happy to see it this way because I think those commands should be able to run with the same privileges as the mesos-slave (i.e., root), but just wanted to call out these semantics.
src/slave/containerizer/mesos_containerizer_helper.cpp
<https://reviews.apache.org/r/22852/#comment82178>
Organizationally, I would have expected the subcommands to be declared here, not in mesos_containerizer.hpp.
Then mesos_containerizer.cpp could include this header, which makes sense since mesos_containerizer.hpp is invoking mesos-containerizer-helper.
Moreover, I would have s/MesosContainerizerLaunch/MesosContainerizerHelperLaunch/ to capture that this is a subcommand of mesos-containerizer-helper.
src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/22852/#comment82179>
Thank you for the consistency cleanup throughout here!!!
- Benjamin Hindman
On June 25, 2014, 6:39 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 6:39 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am 861aad2
> src/slave/containerizer/launcher.hpp 835d7a9
> src/slave/containerizer/launcher.cpp 2b13d1f
> src/slave/containerizer/linux_launcher.hpp 622810c
> src/slave/containerizer/linux_launcher.cpp 7ebccb4
> src/slave/containerizer/mesos_containerizer.hpp 21affae
> src/slave/containerizer/mesos_containerizer.cpp 76c101c
> src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
> src/tests/isolator_tests.cpp 0bbec09
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
> On June 25, 2014, 8:07 p.m., Benjamin Hindman wrote:
> > Looks good Jie. Any reason not to have a launch.cpp too? Just keeps mesos/containerizer.cpp smaller.
Added launch.cpp under mesos/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46676
-----------------------------------------------------------
On June 25, 2014, 7:52 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:52 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am 861aad2
> src/slave/containerizer/containerizer.cpp 922ae17
> src/slave/containerizer/launcher.hpp 835d7a9
> src/slave/containerizer/launcher.cpp 2b13d1f
> src/slave/containerizer/linux_launcher.hpp 622810c
> src/slave/containerizer/linux_launcher.cpp 7ebccb4
> src/slave/containerizer/mesos/containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos/launch.hpp PRE-CREATION
> src/slave/containerizer/mesos/main.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp 21affae
> src/slave/containerizer/mesos_containerizer.cpp 76c101c
> src/tests/cgroups_isolator_tests.cpp 5a9704d
> src/tests/containerizer_tests.cpp 8ea7974
> src/tests/isolator_tests.cpp 0bbec09
> src/tests/master_tests.cpp 885e0fb
> src/tests/mesos.hpp eeb8631
> src/tests/mesos.cpp 24af32b
> src/tests/slave_tests.cpp 21fe685
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/#review46676
-----------------------------------------------------------
Ship it!
Looks good Jie. Any reason not to have a launch.cpp too? Just keeps mesos/containerizer.cpp smaller.
- Benjamin Hindman
On June 25, 2014, 7:52 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22852/
> -----------------------------------------------------------
>
> (Updated June 25, 2014, 7:52 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
>
>
> Bugs: MESOS-1404
> https://issues.apache.org/jira/browse/MESOS-1404
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
>
> Also, we leverage the new Subprocess interface to handle redirections.
>
> (BTW: I fixed a few 70 char width comments issue when I doing the refactor).
>
>
> Diffs
> -----
>
> src/Makefile.am 861aad2
> src/slave/containerizer/containerizer.cpp 922ae17
> src/slave/containerizer/launcher.hpp 835d7a9
> src/slave/containerizer/launcher.cpp 2b13d1f
> src/slave/containerizer/linux_launcher.hpp 622810c
> src/slave/containerizer/linux_launcher.cpp 7ebccb4
> src/slave/containerizer/mesos/containerizer.hpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp PRE-CREATION
> src/slave/containerizer/mesos/launch.hpp PRE-CREATION
> src/slave/containerizer/mesos/main.cpp PRE-CREATION
> src/slave/containerizer/mesos_containerizer.hpp 21affae
> src/slave/containerizer/mesos_containerizer.cpp 76c101c
> src/tests/cgroups_isolator_tests.cpp 5a9704d
> src/tests/containerizer_tests.cpp 8ea7974
> src/tests/isolator_tests.cpp 0bbec09
> src/tests/master_tests.cpp 885e0fb
> src/tests/mesos.hpp eeb8631
> src/tests/mesos.cpp 24af32b
> src/tests/slave_tests.cpp 21fe685
>
> Diff: https://reviews.apache.org/r/22852/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/
-----------------------------------------------------------
(Updated June 25, 2014, 7:52 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
Changes
-------
BenH's comments.
Bugs: MESOS-1404
https://issues.apache.org/jira/browse/MESOS-1404
Repository: mesos-git
Description
-------
See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
Also, we leverage the new Subprocess interface to handle redirections.
(BTW: I fixed a few 70 char width comments issue when I doing the refactor).
Diffs (updated)
-----
src/Makefile.am 861aad2
src/slave/containerizer/containerizer.cpp 922ae17
src/slave/containerizer/launcher.hpp 835d7a9
src/slave/containerizer/launcher.cpp 2b13d1f
src/slave/containerizer/linux_launcher.hpp 622810c
src/slave/containerizer/linux_launcher.cpp 7ebccb4
src/slave/containerizer/mesos/containerizer.hpp PRE-CREATION
src/slave/containerizer/mesos/containerizer.cpp PRE-CREATION
src/slave/containerizer/mesos/launch.hpp PRE-CREATION
src/slave/containerizer/mesos/main.cpp PRE-CREATION
src/slave/containerizer/mesos_containerizer.hpp 21affae
src/slave/containerizer/mesos_containerizer.cpp 76c101c
src/tests/cgroups_isolator_tests.cpp 5a9704d
src/tests/containerizer_tests.cpp 8ea7974
src/tests/isolator_tests.cpp 0bbec09
src/tests/master_tests.cpp 885e0fb
src/tests/mesos.hpp eeb8631
src/tests/mesos.cpp 24af32b
src/tests/slave_tests.cpp 21fe685
Diff: https://reviews.apache.org/r/22852/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 22852: Refactored the mesos containerizer launcher to fix
MESOS-1404.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22852/
-----------------------------------------------------------
(Updated June 25, 2014, 6:39 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, and Vinod Kone.
Changes
-------
Review comments.
Bugs: MESOS-1404
https://issues.apache.org/jira/browse/MESOS-1404
Repository: mesos-git
Description
-------
See summary. The main idea is to create a new binary called 'mesos-containerizer-helper' which performs most of the logic of 'execute' in the original code.
Also, we leverage the new Subprocess interface to handle redirections.
(BTW: I fixed a few 70 char width comments issue when I doing the refactor).
Diffs (updated)
-----
src/Makefile.am 861aad2
src/slave/containerizer/launcher.hpp 835d7a9
src/slave/containerizer/launcher.cpp 2b13d1f
src/slave/containerizer/linux_launcher.hpp 622810c
src/slave/containerizer/linux_launcher.cpp 7ebccb4
src/slave/containerizer/mesos_containerizer.hpp 21affae
src/slave/containerizer/mesos_containerizer.cpp 76c101c
src/slave/containerizer/mesos_containerizer_helper.cpp PRE-CREATION
src/tests/isolator_tests.cpp 0bbec09
Diff: https://reviews.apache.org/r/22852/diff/
Testing
-------
make check
Thanks,
Jie Yu