You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/05/13 02:47:53 UTC
Review Request 34137: Add support for container image provisioners.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
-------
The MesosContainerizer can optionally provision a root filesystem for the containers.
Diffs
-----
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e
src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
src/slave/containerizer/provisioner.hpp PRE-CREATION
src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
Diff: https://reviews.apache.org/r/34137/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jie Yu <yu...@gmail.com>.
> On July 17, 2015, 1:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 630
> > <https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630>
> >
> > Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous!
> >
> > I would prefer we resort to our old style defer style (e.g., introduce `_provision`).
>
> Jiang Yan Xu wrote:
> [=] captures slaveId by value (copy) so it won't be invalid?
>
> But after when to use lambdas, I think this is a good point and we should establish some best practices.
> Google style guide has these guidelines: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions
>
> Avoiding default captures is one of them; limiting the length of lambdas is another.
>
> We should document these, at least reference Google's.
>
> And how about lambdas that simply call another method vs. bind? :)
Aha, good call. I checked the C++11 standard, $5.1.2/14 says that capturing a variable of reference type by copy will create a copy of the value referenced instead of creating a copy of the reference. Thus the lambda will have its own copy of the value that the reference was referencing when it was created.
However, looks like gcc has a bug related to capturing a reference type by using [=] so we should probably avoid that as much as possible
http://stackoverflow.com/questions/6529177/capturing-reference-variable-by-copy-in-c0x-lambda
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92000
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 16, 2015, 6:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 630
> > <https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line630>
> >
> > Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous!
> >
> > I would prefer we resort to our old style defer style (e.g., introduce `_provision`).
[=] captures slaveId by value (copy) so it won't be invalid?
But after when to use lambdas, I think this is a good point and we should establish some best practices.
Google style guide has these guidelines: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions
Avoiding default captures is one of them; limiting the length of lambdas is another.
We should document these, at least reference Google's.
And how about lambdas that simply call another method vs. bind? :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92000
-----------------------------------------------------------
On July 11, 2015, 9:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On July 17, 2015, 1:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, line 24
> > <https://reviews.apache.org/r/34137/diff/3/?file=1009145#file1009145line24>
> >
> > Where is this? This won't compile!
You're right it won't, wierd it compiled for me :( let's fix this!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92000
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jie Yu <yu...@gmail.com>.
> On July 17, 2015, 1:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 637-643
> > <https://reviews.apache.org/r/34137/diff/3/?file=1009143#file1009143line637>
> >
> > No test for checkpointing???
I decided to punt this in this review. Will follow up with a test shortly. The idea is to create a TestProvisioner which will prepare a chroot based on copying the host filesystem (similar to that in launch_tests.cpp).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92000
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92000
-----------------------------------------------------------
Tim, I probably wound wait for Vinod's shipit before committing this.
There seems to be a bug in the code as well (besides the obvious broken build issue).
src/slave/containerizer/mesos/containerizer.cpp (line 629)
<https://reviews.apache.org/r/34137/#comment145846>
Hum, looks like a bug since, for example, slaveId is a reference and will be invalid when the lambda is called. In general, I think we should avoid using [=] for lambdas because its dangeous!
I would prefer we resort to our old style defer style (e.g., introduce `_provision`).
src/slave/containerizer/mesos/containerizer.cpp (lines 636 - 642)
<https://reviews.apache.org/r/34137/#comment145843>
No test for checkpointing???
src/slave/containerizer/mesos/containerizer.cpp (line 1236)
<https://reviews.apache.org/r/34137/#comment145848>
`_____destroy` is a void function. We should do:
```
_____destroy(...);
return;
```
src/slave/containerizer/mesos/containerizer.cpp (line 1247)
<https://reviews.apache.org/r/34137/#comment145849>
Ditto.
src/slave/containerizer/mesos/containerizer.cpp (line 1268)
<https://reviews.apache.org/r/34137/#comment145850>
s/cleanup/future/
src/slave/containerizer/provisioner.hpp (lines 37 - 38)
<https://reviews.apache.org/r/34137/#comment145836>
This should be #include <mesos/slave/isolator.hpp>
src/slave/containerizer/provisioner.hpp (lines 52 - 54)
<https://reviews.apache.org/r/34137/#comment145837>
Recover containers? What container? This is a little misleading. How about
```
Recover root filesystems for containers...
````
src/slave/containerizer/provisioner.cpp (line 24)
<https://reviews.apache.org/r/34137/#comment145840>
Where is this? This won't compile!
- Jie Yu
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 7:41 p.m., Vinod Kone wrote:
> > src/slave/containerizer/provisioner.cpp, lines 43-56
> > <https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43>
> >
> > Why do you need foreach loop here if you were going to return error anyway?
We need the foreach to go over all the provisioners though, as there could be more than one although there is one listed for now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91662
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 7:41 p.m., Vinod Kone wrote:
> > can you fix the "depends on" please?
I think the two listed are the correct dependencies for this rb?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91662
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 7:41 p.m., Vinod Kone wrote:
> > src/slave/containerizer/provisioner.cpp, lines 43-56
> > <https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43>
> >
> > Why do you need foreach loop here if you were going to return error anyway?
>
> Timothy Chen wrote:
> We need the foreach to go over all the provisioners though, as there could be more than one although there is one listed for now.
I can remove the for loop and just return empty map, and also add a log that provisioners are not supported yet if something is specified.
Sounds good?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91662
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91662
-----------------------------------------------------------
can you fix the "depends on" please?
src/slave/containerizer/provisioner.cpp (lines 41 - 44)
<https://reviews.apache.org/r/34137/#comment145206>
Why do you need foreach loop here if you were going to return error anyway?
- Vinod Kone
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92339
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (lines 185 - 186)
<https://reviews.apache.org/r/34137/#comment146447>
Hum, you put provisioner.cpp under OS_LINUX guard. There'll be a link error on OSX since MesosContainerizer supports all platforms.
- Jie Yu
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review92462
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91422
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/
-----------------------------------------------------------
(Updated July 11, 2015, 9:47 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
The MesosContainerizer can optionally provision a root filesystem for the containers.
Diffs (updated)
-----
include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
src/slave/containerizer/provisioner.hpp PRE-CREATION
src/slave/containerizer/provisioner.cpp PRE-CREATION
src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
Diff: https://reviews.apache.org/r/34137/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/
-----------------------------------------------------------
(Updated July 10, 2015, 5:05 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
The MesosContainerizer can optionally provision a root filesystem for the containers.
Diffs
-----
include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
src/slave/containerizer/provisioner.hpp PRE-CREATION
src/slave/containerizer/provisioner.cpp PRE-CREATION
src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
Diff: https://reviews.apache.org/r/34137/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Vinod Kone <vi...@gmail.com>.
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > can you please set the dependency for this review correctly? it's hard to understand which reviews introduced the code that this review depends on.
none of the dropped issues have comments. did you forget to hit publish on your comments?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91014
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 418
> > <https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line418>
> >
> > Is it possible for rootfs to not exist when we are here? If not, there should be a CHECK and a comment on what guarantees its presence (the fact that there is a forked pid?).
the field rootfs is just a option, so I think that's why you can just pass that in.
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > include/mesos/slave/isolator.hpp, lines 70-71
> > <https://reviews.apache.org/r/34137/diff/2/?file=989753#file989753line70>
> >
> > We don't align arguments for constructors this way IIRC.
> >
> > ExecutorRunState(arg1,
> > arg2,
> > ....);
Looks like the latest revision has the right format right?
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1249
> > <https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line1249>
> >
> > why include the containerid? doesn't the caller of this method already know that?
The caller knows this, but since this shows up in the log it's easier to correlate the error with a container id.
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1280
> > <https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line1280>
> >
> > ditto. why include the container id?
You're right I don't tihnk containerId here is needed.
> On July 8, 2015, 11:34 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 818
> > <https://reviews.apache.org/r/34137/diff/2/?file=989756#file989756line818>
> >
> > where is the update to launchFlags to add 'rootfs' ? which review?
Looks like this field is already added in master.
commit 15bf9f67e3d9489e49b2176e1e4221a1a47fd0c9
Author: Ian Downes <id...@twitter.com>
Date: Thu Dec 18 15:46:15 2014 -0800
Support chrooting in MesosContainerizer launch helper.
Review: https://reviews.apache.org/r/31444/
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91014
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review91014
-----------------------------------------------------------
can you please set the dependency for this review correctly? it's hard to understand which reviews introduced the code that this review depends on.
include/mesos/slave/isolator.hpp (lines 70 - 71)
<https://reviews.apache.org/r/34137/#comment144221>
We don't align arguments for constructors this way IIRC.
ExecutorRunState(arg1,
arg2,
....);
src/slave/containerizer/mesos/containerizer.cpp (line 400)
<https://reviews.apache.org/r/34137/#comment144231>
kill new line.
src/slave/containerizer/mesos/containerizer.cpp (line 418)
<https://reviews.apache.org/r/34137/#comment144232>
Is it possible for rootfs to not exist when we are here? If not, there should be a CHECK and a comment on what guarantees its presence (the fact that there is a forked pid?).
src/slave/containerizer/mesos/containerizer.cpp (lines 612 - 614)
<https://reviews.apache.org/r/34137/#comment144234>
Why do you want to consider that? Don't we want to support both type of containers on the same host?
src/slave/containerizer/mesos/containerizer.cpp (lines 622 - 623)
<https://reviews.apache.org/r/34137/#comment144240>
shouldn't there be a check to see if 'image' even exists in the protobuf?
src/slave/containerizer/mesos/containerizer.cpp (line 632)
<https://reviews.apache.org/r/34137/#comment144242>
If you've extracted 'image' into a temp variable instead of 'type' above in #622, you could fit 'provision()' in one line.
src/slave/containerizer/mesos/containerizer.cpp (line 651)
<https://reviews.apache.org/r/34137/#comment144244>
So what happens if the provisioner provisions the file system but the slave restarts before checkpointing 'rootfs'? Is that safe? We don't leak anything?
src/slave/containerizer/mesos/containerizer.cpp (line 790)
<https://reviews.apache.org/r/34137/#comment144245>
when was flags.sandbox_directory introduced? which review?
src/slave/containerizer/mesos/containerizer.cpp (lines 815 - 816)
<https://reviews.apache.org/r/34137/#comment144247>
+1
src/slave/containerizer/mesos/containerizer.cpp (line 817)
<https://reviews.apache.org/r/34137/#comment144251>
where is the update to launchFlags to add 'rootfs' ? which review?
src/slave/containerizer/mesos/containerizer.cpp (line 1248)
<https://reviews.apache.org/r/34137/#comment144248>
why include the containerid? doesn't the caller of this method already know that?
src/slave/containerizer/mesos/containerizer.cpp (line 1279)
<https://reviews.apache.org/r/34137/#comment144249>
ditto. why include the container id?
src/slave/containerizer/provisioner.hpp (lines 59 - 60)
<https://reviews.apache.org/r/34137/#comment144228>
What does this return?
src/slave/containerizer/provisioner.cpp (line 37)
<https://reviews.apache.org/r/34137/#comment144229>
if (flags.provisioners.isNone()) {
}
src/slave/flags.cpp (line 59)
<https://reviews.apache.org/r/34137/#comment144222>
s/./(e.g., appc, docker)./
- Vinod Kone
On July 7, 2015, 7:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 7:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/
-----------------------------------------------------------
(Updated July 7, 2015, 12:42 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Updated dependencies.
Repository: mesos
Description
-------
The MesosContainerizer can optionally provision a root filesystem for the containers.
Diffs
-----
include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
src/slave/containerizer/provisioner.hpp PRE-CREATION
src/slave/containerizer/provisioner.cpp PRE-CREATION
src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
Diff: https://reviews.apache.org/r/34137/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review89227
-----------------------------------------------------------
include/mesos/slave/isolator.hpp (line 71)
<https://reviews.apache.org/r/34137/#comment141822>
We actually don't need the underscores anymore right? According to the updated style?
- Timothy Chen
On June 22, 2015, 4:44 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:44 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review89975
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (line 626)
<https://reviews.apache.org/r/34137/#comment142882>
I think we should include the type in the failure message too.
src/slave/containerizer/mesos/containerizer.cpp (line 1247)
<https://reviews.apache.org/r/34137/#comment142883>
Include the type in the warning message so it's easier to debug.
- Timothy Chen
On June 22, 2015, 4:44 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:44 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
> On June 29, 2015, 12:41 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, lines 319-320
> > <https://reviews.apache.org/r/34137/diff/2/?file=989755#file989755line319>
> >
> > Why not store ContainerInfo directly?
It's optional so I decided to store the required ExecutorInfo and use the container ContainerInfo when appropriate.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review89293
-----------------------------------------------------------
On July 10, 2015, 5:05 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 10, 2015, 5:05 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 29, 2015, 7:41 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner.cpp, lines 46-47
> > <https://reviews.apache.org/r/34137/diff/2/?file=989758#file989758line46>
> >
> > Seems like this belongs to a later patch. AppcProvisioner is not introduced yet.
+1
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review89293
-----------------------------------------------------------
On July 7, 2015, 7:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 7:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review89293
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.hpp (lines 319 - 320)
<https://reviews.apache.org/r/34137/#comment142551>
Why not store ContainerInfo directly?
src/slave/containerizer/mesos/containerizer.cpp (lines 642 - 646)
<https://reviews.apache.org/r/34137/#comment142560>
Indent two more spaces.
src/slave/containerizer/mesos/containerizer.cpp (lines 789 - 790)
<https://reviews.apache.org/r/34137/#comment142225>
```
map<string, string> env = executorEnvironment(
executorInfo,
containers_[containerId]->rootfs.isSome()
? flags.sandbox_directory
: directory,
slaveId,
slavePid,
checkpoint,
flags.recovery_timeout);
```
?
Just a comment, not an issue.
src/slave/containerizer/mesos/containerizer.cpp (lines 815 - 816)
<https://reviews.apache.org/r/34137/#comment142224>
Does
```
containers_[containerId]->rootfs.isSome()
? flags.sandbox_directory
: directory;
```
look better?
Just thought this boarders "too much jaggedness" in the sytle guilde.
src/slave/containerizer/mesos/containerizer.cpp (lines 1254 - 1259)
<https://reviews.apache.org/r/34137/#comment142213>
Shift one more space to the right for alignment.
src/slave/containerizer/mesos/containerizer.cpp (lines 1278 - 1280)
<https://reviews.apache.org/r/34137/#comment142214>
Four space indentation.
src/slave/containerizer/mesos/containerizer.cpp (line 1279)
<https://reviews.apache.org/r/34137/#comment142223>
s/" :"/": "/
src/slave/containerizer/provisioner.cpp (lines 46 - 47)
<https://reviews.apache.org/r/34137/#comment142194>
Seems like this belongs to a later patch. AppcProvisioner is not introduced yet.
src/slave/paths.cpp (lines 243 - 247)
<https://reviews.apache.org/r/34137/#comment141883>
Indent two more spaces.
src/tests/containerizer_tests.cpp (line 344)
<https://reviews.apache.org/r/34137/#comment142215>
Kill empty line.
- Jiang Yan Xu
On June 22, 2015, 9:44 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:44 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
> src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/
-----------------------------------------------------------
(Updated June 22, 2015, 9:44 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
- Support multiple provisioners
- Add recover() to provisioner interface
- Checkpoint and recover optional container rootfs
Repository: mesos
Description
-------
The MesosContainerizer can optionally provision a root filesystem for the containers.
Diffs (updated)
-----
include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
src/slave/containerizer/provisioner.hpp PRE-CREATION
src/slave/containerizer/provisioner.cpp PRE-CREATION
src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5
src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec
src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
Diff: https://reviews.apache.org/r/34137/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
> On May 28, 2015, 9:49 p.m., Paul Brett wrote:
> > src/slave/containerizer/mesos/containerizer.hpp, line 245
> > <https://reviews.apache.org/r/34137/diff/1/?file=957263#file957263line245>
> >
> > To many underscores - can we come up with a better name?
We can refactor later.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review85624
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/containerizer/provisioner.cpp PRE-CREATION
> src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
> src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
> src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f
> src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2
> src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128
> src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7
> src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review85624
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.hpp
<https://reviews.apache.org/r/34137/#comment137264>
To many underscores - can we come up with a better name?
- Paul Brett
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Ian Downes <ia...@gmail.com>.
> On May 14, 2015, 12:41 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 193
> > <https://reviews.apache.org/r/34137/diff/1/?file=957264#file957264line193>
> >
> > Not sure if I follow this logic or if I'm missing something, but the provisioners hashmap seems to be a local variable and I don't see anything populating it? And how will it contain anything?
This is to support the AppC provisioner in a later review, and subsequent provisioners.
> On May 14, 2015, 12:41 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner.hpp, line 35
> > <https://reviews.apache.org/r/34137/diff/1/?file=957265#file957265line35>
> >
> > What would recover do?
Don't know actually, so I removed the TODO. If needed it'll be clear.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review83824
-----------------------------------------------------------
On May 12, 2015, 5:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34137: Add support for container image
provisioners.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34137/#review83824
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34137/#comment134893>
Not sure if I follow this logic or if I'm missing something, but the provisioners hashmap seems to be a local variable and I don't see anything populating it? And how will it contain anything?
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34137/#comment134898>
What happens if they specify a image but no provisioner is there?
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34137/#comment134895>
This seems like a wierd style to me, how about:
return provisioner.get()->provision(
containerId,
executorInfo.container().mesos().image())
.then(defer(self(),
&Self::_provision,
containerId,
executorInfo,
directory,
lambda::_1));
This seems more aligned at least.
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34137/#comment134897>
So currently looks like a containerizer is only able to support one provisioner then?
I don't think it's a big deal, but I think we do want to be able to support multiple image provisioners in mesos containerizer in the future, so perhaps at least leave a comment that we need to change this in the future.
src/slave/containerizer/provisioner.hpp
<https://reviews.apache.org/r/34137/#comment134888>
What would recover do?
- Timothy Chen
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The MesosContainerizer can optionally provision a root filesystem for the containers.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.hpp 3e18617b0dbac58176bfd41dc550898eb6a4a79e
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioner.hpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34137/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>