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 09:13:15 UTC
Re: Review Request 45375: Implemented cleanup() for dvd isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 2, 2016, 7:13 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Summary (updated)
-----------------
Implemented cleanup() for dvd isolator.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description
-------
Implemented cleanup() for dvd isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION
src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/45375/diff/
Testing (updated)
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130056
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 374)
<https://reviews.apache.org/r/45375/#comment193674>
I saw you have already `using process::Owned` before?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 375)
<https://reviews.apache.org/r/45375/#comment193675>
How about reformat it like
```
foreachvalue (process::Owned<Info> info, infos) {
foreach (const DockerVolumeInfo& dockerVolumeInfo,
info->dockerVolumeInfos) {
// The docker volume driver do not enable volume name or driver
// include ":".
const string volumeKey = dockerVolumeInfo.dockerVolume.driver() + ":" +
dockerVolumeInfo.dockerVolume.name();
```
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 395)
<https://reviews.apache.org/r/45375/#comment193682>
I suggest reformat this as well.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 408)
<https://reviews.apache.org/r/45375/#comment193686>
Actually the behaviour here a bit like `Counter` located in `./3rdparty/libprocess/include/process/metrics/counter.hpp`. However `Counter` only used in metrics.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 417)
<https://reviews.apache.org/r/45375/#comment193695>
Seems concat them by `:` and split them by `:` eventually a bit not nice. Is it possible that we use a `pair` as key type?
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 418)
<https://reviews.apache.org/r/45375/#comment193689>
Add `CHECK_EQ(2, tokens.size())` here?
- haosdent huang
On April 22, 2016, 7:08 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 7:08 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130193
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 370)
<https://reviews.apache.org/r/45375/#comment193885>
s/volumeReference/references/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 372)
<https://reviews.apache.org/r/45375/#comment193883>
const Owned<Info>&
Please make sure to use const ref if possible in all other places.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 373 - 374)
<https://reviews.apache.org/r/45375/#comment193886>
`foreach (const DockerVolume& volume, info->volumes)`
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 375 - 378)
<https://reviews.apache.org/r/45375/#comment193887>
This is not necessary if you define a hash function as I mentioned in the earlier patch.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 392)
<https://reviews.apache.org/r/45375/#comment193888>
s/dockerVolumeInfo/volume/
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 394 - 395)
<https://reviews.apache.org/r/45375/#comment193889>
No needed.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 402)
<https://reviews.apache.org/r/45375/#comment193891>
You can avoid this loop. Just combine with the loop above.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 408)
<https://reviews.apache.org/r/45375/#comment193894>
"Unmounting ..."
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 412 - 414)
<https://reviews.apache.org/r/45375/#comment193895>
This is not needed any more.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 456)
<https://reviews.apache.org/r/45375/#comment193896>
for container <contianer_id>
- Jie Yu
On April 22, 2016, 4:13 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 22, 2016, 4:13 p.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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 5:49 p.m., Gilbert Song wrote:
> > This patch looks good to me. Almost ship it!
> >
> > Just one concern:
> >
> > Should we move the ref count logic out if we decide the future default behavior without ref count? then, add ref count logic together with the agent flag in another patch.
>
> Guangya Liu wrote:
> Thanks Gilbert, I will update this part when we reach an agreement on the agent flag, we can discuss in sync up meeting.
I filed a JIRA to trace this https://issues.apache.org/jira/browse/MESOS-5264
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130314
-----------------------------------------------------------
On 四月 24, 2016, 9:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated 四月 24, 2016, 9:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 5:49 p.m., Gilbert Song wrote:
> > This patch looks good to me. Almost ship it!
> >
> > Just one concern:
> >
> > Should we move the ref count logic out if we decide the future default behavior without ref count? then, add ref count logic together with the agent flag in another patch.
Thanks Gilbert, I will update this part when we reach an agreement on the agent flag, we can discuss in sync up meeting.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130314
-----------------------------------------------------------
On 四月 24, 2016, 9:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated 四月 24, 2016, 9:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130314
-----------------------------------------------------------
This patch looks good to me. Almost ship it!
Just one concern:
Should we move the ref count logic out if we decide the future default behavior without ref count? then, add ref count logic together with the agent flag in another patch.
- Gilbert Song
On April 24, 2016, 2:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 24, 2016, 2:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130312
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 350)
<https://reviews.apache.org/r/45375/#comment194024>
const ref please.
- Gilbert Song
On April 24, 2016, 2:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 24, 2016, 2:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 24, 2016, 5:47 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 418-419
> > <https://reviews.apache.org/r/45375/diff/8/?file=1358683#file1358683line418>
> >
> > containerDir contains the containerId, right?
Yes, but seems specifying the `containerId` is more clear as people can see the `containerId` if they are not familar with the checkpoint logic in dvd isolator, make sense?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130313
-----------------------------------------------------------
On 四月 24, 2016, 9:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated 四月 24, 2016, 9:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130313
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 418 - 419)
<https://reviews.apache.org/r/45375/#comment194026>
containerDir contains the containerId, right?
- Gilbert Song
On April 24, 2016, 2:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 24, 2016, 2:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review130316
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On April 24, 2016, 9:50 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 24, 2016, 9:50 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 24, 2016, 9:50 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 cleanup() for docker volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
Diff: https://reviews.apache.org/r/45375/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 23, 2016, 7:58 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 cleanup() for docker volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
Diff: https://reviews.apache.org/r/45375/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 22, 2016, 4:13 p.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 cleanup() for docker volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
Diff: https://reviews.apache.org/r/45375/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 22, 2016, 7:08 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 cleanup() for docker volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
Diff: https://reviews.apache.org/r/45375/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
> On 四月 22, 2016, 1:04 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 378-380
> > <https://reviews.apache.org/r/45375/diff/4/?file=1354513#file1354513line378>
> >
> > Seems like we should capture the first `:` here. Otherwise, we have to document that users should not include any `:` in a volume name.
It is not valid to contain `:` in the volume name, the volume creation will be failed.
root@mesos002:~# dvdcli create --volumedriver=convoy --volumename=a:2
ERRO[0000] Invalid volume a:2. Must be only contains 0-9, a-z, dash(-), underscore(_) and dot(.) volumeName=a:2
> On 四月 22, 2016, 1:04 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 345
> > <https://reviews.apache.org/r/45375/diff/4/?file=1354513#file1354513line345>
> >
> > add `!`.
good catch ;-)
> On 四月 22, 2016, 1:04 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, lines 399-408
> > <https://reviews.apache.org/r/45375/diff/4/?file=1354513#file1354513line399>
> >
> > ditto.
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 四月 22, 2016, 1:04 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp, line 420
> > <https://reviews.apache.org/r/45375/diff/4/?file=1354513#file1354513line420>
> >
> > Will `container directory` be confused?
LOG(INFO) << "Removed the container docker volume checkpoint directory '"
<< containerDir << "'";
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review129968
-----------------------------------------------------------
On 四月 21, 2016, 3:43 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated 四月 21, 2016, 3:43 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review129968
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 331 - 333)
<https://reviews.apache.org/r/45375/#comment193545>
Please add
```
VLOG(1) << "Ignoring cleanup request for unknown container: "
<< containerId;```
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 338)
<https://reviews.apache.org/r/45375/#comment193547>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 339)
<https://reviews.apache.org/r/45375/#comment193548>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 342)
<https://reviews.apache.org/r/45375/#comment193549>
const string
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 345)
<https://reviews.apache.org/r/45375/#comment193550>
add `!`.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 360)
<https://reviews.apache.org/r/45375/#comment193611>
const string
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 368)
<https://reviews.apache.org/r/45375/#comment193551>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 378 - 380)
<https://reviews.apache.org/r/45375/#comment193614>
Seems like we should capture the first `:` here. Otherwise, we have to document that users should not include any `:` in a volume name.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 399 - 408)
<https://reviews.apache.org/r/45375/#comment193616>
ditto.
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 420)
<https://reviews.apache.org/r/45375/#comment193618>
Will `container directory` be confused?
- Gilbert Song
On April 20, 2016, 8:43 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 8:43 p.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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/#review129859
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46245, 45373, 45326, 45270, 46426, 46428, 45360, 46180, 45370, 45375]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 21, 2016, 3:43 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45375/
> -----------------------------------------------------------
>
> (Updated April 21, 2016, 3:43 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 cleanup() for docker volume isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
> src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
>
> Diff: https://reviews.apache.org/r/45375/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 45375: Implemented cleanup() for docker volume
isolator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45375/
-----------------------------------------------------------
(Updated 四月 21, 2016, 3:43 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Summary (updated)
-----------------
Implemented cleanup() for docker volume isolator.
Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013
Repository: mesos
Description (updated)
-------
Implemented cleanup() for docker volume isolator.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp bedc687cc280d0b721fb84801039fd3614364cca
src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 915e5ae755a55a02b7dfcda88165f27346cad955
Diff: https://reviews.apache.org/r/45375/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu