You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/04/02 08:39:10 UTC
Re: Review Request 45370: Implemented prepare() for dvd isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 2, 2016, 6:39 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Repository: mesos
Description
-------
Implemented prepare() for dvd isolator.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
The execute.cpp is mainly for test, will remove this file once test finished.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 13, 2016, 8:54 p.m., James DeFelice wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 113
> > <https://reviews.apache.org/r/45370/diff/4/?file=1324030#file1324030line113>
> >
> > other `--flag` params are computed elsewhere (e.g. DvdClient::mount); should do the same here. Just construct Parameters instead
It was done in https://reviews.apache.org/r/45360/
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128741
-----------------------------------------------------------
On 四月 14, 2016, 6:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 14, 2016, 6:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for dvd isolator.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128741
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 110)
<https://reviews.apache.org/r/45370/#comment192206>
options in ExternalMount is a repeated Parameter, which has key and value. Looks like this needs some refactoring.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 113)
<https://reviews.apache.org/r/45370/#comment192207>
other `--flag` params are computed elsewhere (e.g. DvdClient::mount); should do the same here. Just construct Parameters instead
- James DeFelice
On April 4, 2016, 9:21 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 9:21 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for dvd isolator.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> The execute.cpp is mainly for test, will remove this file once test finished.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 5, 2016, 5:47 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 110
> > <https://reviews.apache.org/r/45370/diff/4/?file=1324030#file1324030line110>
> >
> > ```
> > foreach (const string& driverOption,
> > volume.source().docker_volume().driver_options()) {
> > driverOptions += " --volumeopts=" + driverOption;
> > }
> > ```
This is now changed to `Parameters`
> On 四月 5, 2016, 5:47 p.m., haosdent huang wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 62
> > <https://reviews.apache.org/r/45370/diff/4/?file=1324030#file1324030line62>
> >
> > How about use `which` when it avaiable? https://issues.apache.org/jira/browse/MESOS-4576
Sometimes the `dvdcli` may not in `PATH` and we may not able to use `which` to get the dvdcli path.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review127145
-----------------------------------------------------------
On 四月 14, 2016, 6:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 14, 2016, 6:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for dvd isolator.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review127145
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp (line 21)
<https://reviews.apache.org/r/45370/#comment190281>
Why need `<tuple>` and `<subprocess.hpp>` here?
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp (line 74)
<https://reviews.apache.org/r/45370/#comment190279>
A bit confusing why add `using` here? Because of line limit? How about
```
multihashmap<ContainerID, process::Owned<dvd::spec::ExternalMount>>
containermountmap infos;
```
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 62)
<https://reviews.apache.org/r/45370/#comment190283>
How about use `which` when it avaiable? https://issues.apache.org/jira/browse/MESOS-4576
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 98)
<https://reviews.apache.org/r/45370/#comment190285>
`vector`? And add `using std::vector` above? And I think `vector<ExternalMount>` without `spec` would be better
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 110)
<https://reviews.apache.org/r/45370/#comment190288>
```
foreach (const string& driverOption,
volume.source().docker_volume().driver_options()) {
driverOptions += " --volumeopts=" + driverOption;
}
```
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 133)
<https://reviews.apache.org/r/45370/#comment190300>
Add a line above.
src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp (line 29)
<https://reviews.apache.org/r/45370/#comment190293>
I think a method which accept these parameters are enough? This way looks more like Java.
- haosdent huang
On April 4, 2016, 9:21 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 9:21 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for dvd isolator.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> The execute.cpp is mainly for test, will remove this file once test finished.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128946
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 111)
<https://reviews.apache.org/r/45370/#comment192400>
Shrink the first space to above line.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 124 - 125)
<https://reviews.apache.org/r/45370/#comment192406>
We need a `if` to check here, otherwise it can cause segfault if `driver_options` is none.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 127 - 134)
<https://reviews.apache.org/r/45370/#comment192407>
How about moving `.setContainerId(stringify(containerId))` next line?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 164 - 165)
<https://reviews.apache.org/r/45370/#comment192408>
Save the raw cmd as a variable and pass this cmd to VLOG(1).
- Gilbert Song
On April 13, 2016, 11:29 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 13, 2016, 11:29 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 21, 2016, 9:36 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, line 77
> > <https://reviews.apache.org/r/45370/diff/8/?file=1355000#file1355000line77>
> >
> > Should we use a hashset for `dockerVolumeInfos`?
> >
> > I am thinking if a user has two `DockerVolumleInfo` with exactly the same `dockerVolume`, `containerPath` and `driverOptions`, do we still bind mount them twice?
+1, I think that we should throw error for such case, what do you think?
> On 四月 21, 2016, 9:36 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 259-274
> > <https://reviews.apache.org/r/45370/diff/8/?file=1355001#file1355001line259>
> >
> > I would prefer remove `vector<string> messages` and return a failure once you get the first non-ready future. Please add necessary failure message.
Gilbert, I was following similar logic in https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L671-L680 , I think that this can give end user a more clear message for all errors and is helpful to trouble shooting, what do you think?
> On 四月 21, 2016, 9:36 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 213
> > <https://reviews.apache.org/r/45370/diff/8/?file=1355001#file1355001line213>
> >
> > const string volumePath
Why not `string&`? Any more comments? ;-)
> On 四月 21, 2016, 9:36 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 141-144
> > <https://reviews.apache.org/r/45370/diff/8/?file=1355001#file1355001line141>
> >
> > Not yours, just leave a note here.
> >
> > @jieyu, currently we have different places (e.g., slave.cpp, filesystem/linux.cpp, mesos/containerizer.cpp) explicitly exclude diff volume type.
> >
> > As we may have more volume types in the future, we should find a universal way to handle diff volume type exclussion.
I will update here as following as we now have only one source: DOCKER_VOLUME.
if (!volume.has_source()) {
continue;
}
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review129959
-----------------------------------------------------------
On 四月 21, 2016, 2:21 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 21, 2016, 2:21 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 21, 2016, 9:36 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, line 77
> > <https://reviews.apache.org/r/45370/diff/8/?file=1355000#file1355000line77>
> >
> > Should we use a hashset for `dockerVolumeInfos`?
> >
> > I am thinking if a user has two `DockerVolumleInfo` with exactly the same `dockerVolume`, `containerPath` and `driverOptions`, do we still bind mount them twice?
>
> Guangya Liu wrote:
> +1, I think that we should throw error for such case, what do you think?
I was not returning error but just ignore this mount. Another point is that I think that we should only care about drvier and name when checking if one container using two different volumes.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review129959
-----------------------------------------------------------
On 四月 21, 2016, 2:21 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 21, 2016, 2:21 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review129959
-----------------------------------------------------------
Thanks Guangya! This is great! Need to do one more pass later.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 77)
<https://reviews.apache.org/r/45370/#comment193528>
Should we use a hashset for `dockerVolumeInfos`?
I am thinking if a user has two `DockerVolumleInfo` with exactly the same `dockerVolume`, `containerPath` and `driverOptions`, do we still bind mount them twice?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 136)
<https://reviews.apache.org/r/45370/#comment193531>
Mentioned before, please move the first space to above line. Thanks:)
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 141 - 144)
<https://reviews.apache.org/r/45370/#comment193533>
Not yours, just leave a note here.
@jieyu, currently we have different places (e.g., slave.cpp, filesystem/linux.cpp, mesos/containerizer.cpp) explicitly exclude diff volume type.
As we may have more volume types in the future, we should find a universal way to handle diff volume type exclussion.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 152 - 167)
<https://reviews.apache.org/r/45370/#comment193536>
Why not merge these two `if`?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 208)
<https://reviews.apache.org/r/45370/#comment193539>
`foreach (...)`, plus a space:)
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 213)
<https://reviews.apache.org/r/45370/#comment193540>
const string volumePath
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 259 - 274)
<https://reviews.apache.org/r/45370/#comment193542>
I would prefer remove `vector<string> messages` and return a failure once you get the first non-ready future. Please add necessary failure message.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 279)
<https://reviews.apache.org/r/45370/#comment193543>
ditto.
src/slave/slave.cpp (lines 3808 - 3813)
<https://reviews.apache.org/r/45370/#comment193522>
Pull this out of `if (path::absolute(volume->container_path())) {...}`.
- Gilbert Song
On April 21, 2016, 7:21 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 7:21 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 22, 2016, 6:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 242
> > <https://reviews.apache.org/r/45370/diff/10/?file=1357454#file1357454line242>
> >
> > YOu can iterate through `volumeInfos` here.
>
> Guangya Liu wrote:
> You mean `foreach` is not good for this?
Got it, I already created the `volumeInfos` above, so can use it directly.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130133
-----------------------------------------------------------
On 四月 22, 2016, 4:08 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 22, 2016, 4:08 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 22, 2016, 6:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, line 77
> > <https://reviews.apache.org/r/45370/diff/10/?file=1357453#file1357453line77>
> >
> > Should that be a hashset given that we don't allow duplicate?
The problem is that `hashset` do not support using `DockerVolumeInfo` as its value, so I was using a vector instead, any comments?
> On 四月 22, 2016, 6:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 242
> > <https://reviews.apache.org/r/45370/diff/10/?file=1357454#file1357454line242>
> >
> > YOu can iterate through `volumeInfos` here.
You mean `foreach` is not good for this?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130133
-----------------------------------------------------------
On 四月 22, 2016, 4:08 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 22, 2016, 4:08 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 22, 2016, 6:54 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, line 77
> > <https://reviews.apache.org/r/45370/diff/10/?file=1357453#file1357453line77>
> >
> > Should that be a hashset given that we don't allow duplicate?
>
> Guangya Liu wrote:
> The problem is that `hashset` do not support using `DockerVolumeInfo` as its value, so I was using a vector instead, any comments?
yeah, vector sounds fine.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130133
-----------------------------------------------------------
On April 22, 2016, 4:08 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 4:08 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130133
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 65)
<https://reviews.apache.org/r/45370/#comment193796>
s/DockerVolumeInfo/VolumeInfo/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (lines 65 - 70)
<https://reviews.apache.org/r/45370/#comment193817>
We typically just put stuff that's needed for cleanup in 'Info'. Also, remember that any fields in 'Info' should be able to be recovered after agent restarts.
So I would suggest that we don't put that in Info:
```
struct VolumeInfo
{
DockerVolume volume;
string containerPath;
Option<hashmap<...>> options;
};
struct Info
{
vector<DockerVolume> volumes;
};
```
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 67)
<https://reviews.apache.org/r/45370/#comment193797>
s/dockerVolume/volume/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (line 77)
<https://reviews.apache.org/r/45370/#comment193823>
Should that be a hashset given that we don't allow duplicate?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 139)
<https://reviews.apache.org/r/45370/#comment193821>
s/dockerVolumeInfos/volumeInfos/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 141 - 143)
<https://reviews.apache.org/r/45370/#comment193808>
This sentense is broken. Please fix the gramma.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 144)
<https://reviews.apache.org/r/45370/#comment193799>
Instead of that, let's introduce a hash function for DockerVolume. You can put the hash function in state.hpp.
```
template <>
struct hash<mesos::internal::slave::DockerVolume>
{
...
};
```
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 144 - 145)
<https://reviews.apache.org/r/45370/#comment193800>
s/dockerVolumeKey/volumes/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 151 - 152)
<https://reviews.apache.org/r/45370/#comment193801>
Please check the source.type as well. We might want to add more types later.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 154)
<https://reviews.apache.org/r/45370/#comment193805>
Once you define the hash for DockerVolume, you don't have to do that.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 156 - 159)
<https://reviews.apache.org/r/45370/#comment193806>
I would return a Failure here.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 168)
<https://reviews.apache.org/r/45370/#comment193807>
I don't think this check is necessary.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 169)
<https://reviews.apache.org/r/45370/#comment193819>
Let's kill this temp variable as well. We try to avoid temp variables if possible.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 170 - 171)
<https://reviews.apache.org/r/45370/#comment193818>
I would remove this logging.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 200 - 202)
<https://reviews.apache.org/r/45370/#comment193836>
I would add 'volume' to 'info.volumes' after we successfully create the containerDir and successfully checkpoint the state file.
This can save us from some unnecessary cleanup if any of the above failed.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 215 - 217)
<https://reviews.apache.org/r/45370/#comment193813>
I would move this log below after the checkpointing is done.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 229)
<https://reviews.apache.org/r/45370/#comment193837>
s/checkpointed/checkpoint/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 235)
<https://reviews.apache.org/r/45370/#comment193840>
s/external/docker/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 241)
<https://reviews.apache.org/r/45370/#comment193838>
Please use const ref here.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 242)
<https://reviews.apache.org/r/45370/#comment193839>
YOu can iterate through `volumeInfos` here.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 258)
<https://reviews.apache.org/r/45370/#comment193842>
you can bind `volumeInfos` here.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 266)
<https://reviews.apache.org/r/45370/#comment193844>
s/volumes/futures/
src/slave/slave.cpp (lines 3813 - 3818)
<https://reviews.apache.org/r/45370/#comment193794>
Can you do that in a separate patch?
- Jie Yu
On April 22, 2016, 4:08 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 4:08 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130311
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On April 24, 2016, 9:44 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 24, 2016, 9:44 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 24, 2016, 9:44 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 6:29 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 174-188
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358423#file1358423line174>
> >
> > Let's not create any side effect at this moment. Let's do that after creating the checkpoint. Also, don't you need to do mkdir for containerPath if rootfs is not used?
>
> Jie Yu wrote:
> I'll make the change for you.
>
> Guangya Liu wrote:
> +1 to create the mount point after checkpoint succeed.
It may not that straightforward to update the logic here, the current logic in prepare() is as this:
1) traverse all volumes, create the checkpoint volume hashset and vector volumeInfo, voluemInfo including containerPath, DockerVolume and mount options.
2) checkpoint the hashset volume.
If want to create the the containerPath after creating the checkpoint, then we may need to travse all volumes again to generate the containerPath and then create the containerPath.
So my thinking for the logic change is that:
1) In `prepare`, traverse all volumes and only generate the checkpoint hashset volume list.
2) Checkpoint the volumes.
3) In `_prepare`, may need add a new parameter `ExecutorInfo`, then in `_prepare`, travrse all the volumes again to get containerPath.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130286
-----------------------------------------------------------
On 四月 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 6:29 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 174-188
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358423#file1358423line174>
> >
> > Let's not create any side effect at this moment. Let's do that after creating the checkpoint. Also, don't you need to do mkdir for containerPath if rootfs is not used?
>
> Jie Yu wrote:
> I'll make the change for you.
>
> Guangya Liu wrote:
> +1 to create the mount point after checkpoint succeed.
>
> Guangya Liu wrote:
> It may not that straightforward to update the logic here, the current logic in prepare() is as this:
> 1) traverse all volumes, create the checkpoint volume hashset and vector volumeInfo, voluemInfo including containerPath, DockerVolume and mount options.
> 2) checkpoint the hashset volume.
>
> If want to create the the containerPath after creating the checkpoint, then we may need to travse all volumes again to generate the containerPath and then create the containerPath.
>
> So my thinking for the logic change is that:
> 1) In `prepare`, traverse all volumes and only generate the checkpoint hashset volume list.
> 2) Checkpoint the volumes.
> 3) In `_prepare`, may need add a new parameter `ExecutorInfo`, then in `_prepare`, travrse all the volumes again to get containerPath.
Correct a typo:
3) In _prepare, may need add a new parameter `ContainerConfig`, then in _prepare, travrse all the volumes again to get containerPath.
@Yu Jie, can you please show more comments for what does `don't you need to do mkdir for containerPath if rootfs is not used?`, how to check if a rootfs is not used? I have updated the patch according to your comments, please help check. thanks.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130286
-----------------------------------------------------------
On 四月 24, 2016, 9:44 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 24, 2016, 9:44 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 6:29 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 174-188
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358423#file1358423line174>
> >
> > Let's not create any side effect at this moment. Let's do that after creating the checkpoint. Also, don't you need to do mkdir for containerPath if rootfs is not used?
>
> Jie Yu wrote:
> I'll make the change for you.
+1 to create the mount point after checkpoint succeed.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130286
-----------------------------------------------------------
On 四月 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 24, 2016, 6:29 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 174-188
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358423#file1358423line174>
> >
> > Let's not create any side effect at this moment. Let's do that after creating the checkpoint. Also, don't you need to do mkdir for containerPath if rootfs is not used?
I'll make the change for you.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130286
-----------------------------------------------------------
On April 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130286
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 174 - 188)
<https://reviews.apache.org/r/45370/#comment194005>
Let's not create any side effect at this moment. Let's do that after creating the checkpoint. Also, don't you need to do mkdir for containerPath if rootfs is not used?
- Jie Yu
On April 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 24, 2016, 7:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, lines 65-70
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358422#file1358422line65>
> >
> > Also, I realized that we don't need this struct. We just need the mount targets in `_prepare()`.
I'll make the change for you.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130287
-----------------------------------------------------------
On April 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
> On April 24, 2016, 7:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, lines 65-70
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358422#file1358422line65>
> >
> > Also, I realized that we don't need this struct. We just need the mount targets in `_prepare()`.
>
> Jie Yu wrote:
> I'll make the change for you.
In fact, I'll move it to the cpp file. I don't think we need it in the header.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130287
-----------------------------------------------------------
On April 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 7:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp, lines 65-70
> > <https://reviews.apache.org/r/45370/diff/11/?file=1358422#file1358422line65>
> >
> > Also, I realized that we don't need this struct. We just need the mount targets in `_prepare()`.
>
> Jie Yu wrote:
> I'll make the change for you.
>
> Jie Yu wrote:
> In fact, I'll move it to the cpp file. I don't think we need it in the header.
Yes, moving it to cpp file may be better, but we still need to construct it in `prepare` and use it in `_prepare`.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130287
-----------------------------------------------------------
On 四月 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130287
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp (lines 65 - 70)
<https://reviews.apache.org/r/45370/#comment194006>
Also, I realized that we don't need this struct. We just need the mount targets in `_prepare()`.
- Jie Yu
On April 23, 2016, 7:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 7:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review130293
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 276)
<https://reviews.apache.org/r/45370/#comment194010>
Should we rename `volumes` as `mountPoints`?
- Gilbert Song
On April 23, 2016, 12:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 23, 2016, 12:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 23, 2016, 7:06 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/containerizer/mesos/isolators/docker/volume/state.hpp bc0c3299f9f3e351aaa20652b784bccbfd40c1a5
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 22, 2016, 4:08 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/slave.cpp a365e8f5f8d63bf80614fccf6adf35a4fee07526
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 22, 2016, 6:36 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 21, 2016, 2:21 p.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 21, 2016, 1:45 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
Diff: https://reviews.apache.org/r/45370/diff/
Testing (updated)
-------
make
make check
Will update `Sequence` later.
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 14, 2016, 6:29 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Changes
-------
Address comments for haosdent, gilbert and James.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 14, 2016, 6:06 a.m.)
Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
Summary (updated)
-----------------
Implemented prepare() for volume isolator.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description (updated)
-------
Implemented prepare() for volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/45370/diff/
Testing (updated)
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45370: Implemented prepare() for dvd isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review126980
-----------------------------------------------------------
src/cli/execute.cpp (lines 323 - 341)
<https://reviews.apache.org/r/45370/#comment190111>
let's keep this as a private branch for testing. This should not be merged into cli/execute.cpp
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp (line 77)
<https://reviews.apache.org/r/45370/#comment190113>
Add comments.
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (lines 105 - 106)
<https://reviews.apache.org/r/45370/#comment190112>
Use `const string&`
- Gilbert Song
On April 4, 2016, 2:21 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 2:21 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for dvd isolator.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> The execute.cpp is mainly for test, will remove this file once test finished.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 13, 2016, 9:05 p.m., James DeFelice wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 106
> > <https://reviews.apache.org/r/45370/diff/4/?file=1324030#file1324030line106>
> >
> > we should probably validate (or else sanitize) the contents of driver, name, and options
I did not check because they are `required` values.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128744
-----------------------------------------------------------
On 四月 14, 2016, 6:06 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 14, 2016, 6:06 a.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for volume isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 13, 2016, 9:05 p.m., James DeFelice wrote:
> > src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp, line 106
> > <https://reviews.apache.org/r/45370/diff/4/?file=1324030#file1324030line106>
> >
> > we should probably validate (or else sanitize) the contents of driver, name, and options
>
> Guangya Liu wrote:
> I did not check because they are `required` values.
But as `options` is optional, so I only validate `options`
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128744
-----------------------------------------------------------
On 四月 21, 2016, 2:21 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated 四月 21, 2016, 2:21 p.m.)
>
>
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
> src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> Will update `Sequence` later.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for dvd isolator.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/#review128744
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 106)
<https://reviews.apache.org/r/45370/#comment192209>
we should probably validate (or else sanitize) the contents of driver, name, and options
- James DeFelice
On April 4, 2016, 9:21 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> -----------------------------------------------------------
>
> (Updated April 4, 2016, 9:21 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented prepare() for dvd isolator.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
> src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/45370/diff/
>
>
> Testing
> -------
>
> The execute.cpp is mainly for test, will remove this file once test finished.
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45370: Implemented prepare() for dvd isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45370/
-----------------------------------------------------------
(Updated 四月 4, 2016, 9:21 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented prepare() for dvd isolator.
Diffs (updated)
-----
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/45370/diff/
Testing
-------
The execute.cpp is mainly for test, will remove this file once test finished.
Thanks,
Guangya Liu