You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2015/09/05 03:20:21 UTC
Review Request 38141: Refactor shared paths in provisioners.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38141/
-----------------------------------------------------------
Review request for mesos and Jiang Yan Xu.
Repository: mesos
Description
-------
Refactor shared paths in provisioners.
Diffs
-----
src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe
src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57
src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33
src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe
src/slave/containerizer/provisioners/paths.hpp PRE-CREATION
src/slave/containerizer/provisioners/paths.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/38141/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 38141: Refactor shared paths in provisioners.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38141/#review97840
-----------------------------------------------------------
Bad patch!
Reviews applied: [38141]
Failed command: make -j3 distcheck
Error:
make dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.25.0"; then find "mesos-0.25.0" -type d ! -perm -200 -exec chmod u+w {} ';' && rm -rf "mesos-0.25.0" || { sleep 5 && rm -rf "mesos-0.25.0"; }; else :; fi
test -d "mesos-0.25.0" || mkdir "mesos-0.25.0"
(cd 3rdparty && make top_distdir=../mesos-0.25.0 distdir=../mesos-0.25.0/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd libprocess && make top_distdir=../../mesos-0.25.0 distdir=../../mesos-0.25.0/3rdparty/libprocess \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d "../../mesos-0.25.0/3rdparty/libprocess" || mkdir "../../mesos-0.25.0/3rdparty/libprocess"
(cd 3rdparty && make top_distdir=../../../mesos-0.25.0 distdir=../../../mesos-0.25.0/3rdparty/libprocess/3rdparty \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd stout && make top_distdir=../../../../mesos-0.25.0 distdir=../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout" || mkdir "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout"
(cd include && make top_distdir=../../../../../mesos-0.25.0 distdir=../../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[6]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n ":" \
|| find "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash /home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh -c -m a+r {} {} \; \
|| chmod -R a+r "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout"
make[5]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
(cd include && make top_distdir=../../../mesos-0.25.0 distdir=../../../mesos-0.25.0/3rdparty/libprocess/include \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n ":" \
|| find "../../mesos-0.25.0/3rdparty/libprocess" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
! -type d ! -perm -400 -exec chmod a+r {} \; -o \
! -type d ! -perm -444 -exec /bin/bash /home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh -c -m a+r {} {} \; \
|| chmod -R a+r "../../mesos-0.25.0/3rdparty/libprocess"
make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
(cd src && make top_distdir=../mesos-0.25.0 distdir=../mesos-0.25.0/src \
am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target `slave/containerizer/provisioners/path.hpp', needed by `distdir'. Stop.
make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory `/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
make: *** [dist] Error 2
- Mesos ReviewBot
On Sept. 5, 2015, 1:20 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2015, 1:20 a.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor shared paths in provisioners.
>
>
> Diffs
> -----
>
> src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe
> src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57
> src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33
> src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe
> src/slave/containerizer/provisioners/paths.hpp PRE-CREATION
> src/slave/containerizer/provisioners/paths.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 38141: Refactor shared paths in provisioners.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38141/#review98097
-----------------------------------------------------------
Ship it!
LGTM!
Could you run `sudo make check GTEST_FILTER="*Provisioner*"` to make sure Appc tests pass?
I originally consolidated all path manipulations into a single paths.hpp|cpp but now with provisioner dir part pulled out it may make sense to put the store specific ones into the store itself, but that's for another patch I will take on myself.
src/slave/containerizer/provisioners/appc/paths.cpp (lines 21 - 23)
<https://reviews.apache.org/r/38141/#comment154347>
No longer needed.
src/slave/containerizer/provisioners/appc/paths.cpp (line 25)
<https://reviews.apache.org/r/38141/#comment154349>
Ditto.
src/slave/containerizer/provisioners/appc/paths.cpp (line 28)
<https://reviews.apache.org/r/38141/#comment154350>
Ditto.
src/slave/containerizer/provisioners/appc/paths.cpp (line 31)
<https://reviews.apache.org/r/38141/#comment154352>
Ditto.
src/slave/containerizer/provisioners/appc/paths.cpp (line 87)
<https://reviews.apache.org/r/38141/#comment154353>
A single blank line.
src/slave/containerizer/provisioners/paths.hpp (lines 46 - 48)
<https://reviews.apache.org/r/38141/#comment154343>
This comment was assuming this was for APPC. Now that it's generic, it should be something like:
```
// NOTE: Each container could have multiple image types, therefore there
// can be the same <container_id> directories under each provisioner e.g.,
// <work_dir>/provisioners/DOCKER, <work_dir>/provisioners/APPC, etc.
```
src/slave/containerizer/provisioners/paths.hpp (lines 49 - 51)
<https://reviews.apache.org/r/38141/#comment154346>
There is a "For appc" here, can the same be true for docker? I think it could.
- Jiang Yan Xu
On Sept. 7, 2015, 2:31 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2015, 2:31 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor shared paths in provisioners.
>
>
> Diffs
> -----
>
> src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe
> src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57
> src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33
> src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe
> src/slave/containerizer/provisioners/paths.hpp PRE-CREATION
> src/slave/containerizer/provisioners/paths.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 38141: Refactor shared paths in provisioners.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38141/#review97949
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38141]
All tests passed.
- Mesos ReviewBot
On Sept. 7, 2015, 9:31 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> -----------------------------------------------------------
>
> (Updated Sept. 7, 2015, 9:31 p.m.)
>
>
> Review request for mesos and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Refactor shared paths in provisioners.
>
>
> Diffs
> -----
>
> src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe
> src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57
> src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33
> src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe
> src/slave/containerizer/provisioners/paths.hpp PRE-CREATION
> src/slave/containerizer/provisioners/paths.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/38141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 38141: Refactor shared paths in provisioners.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38141/
-----------------------------------------------------------
(Updated Sept. 7, 2015, 9:31 p.m.)
Review request for mesos and Jiang Yan Xu.
Repository: mesos
Description
-------
Refactor shared paths in provisioners.
Diffs (updated)
-----
src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe
src/slave/containerizer/provisioners/appc.cpp fc5ee19df51a3543aaf01c2301b976700610ff57
src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7295809d745dd15bee6db1f7e8dd99ab33
src/slave/containerizer/provisioners/appc/paths.cpp e6be851e24886d7c886adad4c7ea29ded17bdcbe
src/slave/containerizer/provisioners/paths.hpp PRE-CREATION
src/slave/containerizer/provisioners/paths.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/38141/diff/
Testing
-------
make check
Thanks,
Timothy Chen