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/01 02:56:55 UTC

Re: Review Request 37921: Add Copy backend for provisioners.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/
-----------------------------------------------------------

(Updated Sept. 1, 2015, 12:56 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Bugs: MESOS-2968
    https://issues.apache.org/jira/browse/MESOS-2968


Repository: mesos


Description
-------

Add Copy backend for provisioners.


Diffs (updated)
-----

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
  src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 

Diff: https://reviews.apache.org/r/37921/diff/


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Timothy Chen <tn...@apache.org>.

> On Sept. 2, 2015, 3:07 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/backends/copy.cpp, lines 108-116
> > <https://reviews.apache.org/r/37921/diff/1-3/?file=1059333#file1059333line108>
> >
> >     Wow, I guess I hadn't realized what you meant by "making sure the layers have the same basename" and I overlooked the fact that when multiple layers are applied every layer other than the 1st one is going to have the rootfs dir already created...
> >     
> >     I think it's too much of a restriction to impose the same basename for all rootfes and it's not the backend's position to know that they should all be the same.
> >     
> >     Given that there is unfortunately no standard way of doing this, I think I'll be OK with either of the following two approaches:
> >     
> >     1) The following two commands can make sure the source overwrites the target event if it is a diretory.
> >     
> >     GNU cp:
> >     cp -aT source target // -T makes sure the contents are copied.
> >     
> >     OSX cp:
> >     cp a source/ target // The trailing slash makes sure the contents instead of the directory is copied.
> >     
> >     So we can do something like 
> >     
> >     #ifdef __APPLE__
> >       source += "/"
> >     #else
> >       options += "T"
> >     #endif
> >     
> >     2) Use your previous approach with "/*" expansion but with quotes around the paths.
> >     
> >     Neither is ideal but is no worse than the system commands we are already invoking.
> >     
> >     I am OK with either. What do you think?

hmm, I think I'll go for 1) option.


- Timothy


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/#review97376
-----------------------------------------------------------


On Sept. 1, 2015, 12:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
>     https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/#review97376
-----------------------------------------------------------



src/slave/containerizer/provisioners/backends/copy.cpp (lines 105 - 113)
<https://reviews.apache.org/r/37921/#comment153260>

    Wow, I guess I hadn't realized what you meant by "making sure the layers have the same basename" and I overlooked the fact that when multiple layers are applied every layer other than the 1st one is going to have the rootfs dir already created...
    
    I think it's too much of a restriction to impose the same basename for all rootfes and it's not the backend's position to know that they should all be the same.
    
    Given that there is unfortunately no standard way of doing this, I think I'll be OK with either of the following two approaches:
    
    1) The following two commands can make sure the source overwrites the target event if it is a diretory.
    
    GNU cp:
    cp -aT source target // -T makes sure the contents are copied.
    
    OSX cp:
    cp a source/ target // The trailing slash makes sure the contents instead of the directory is copied.
    
    So we can do something like 
    
    #ifdef __APPLE__
      source += "/"
    #else
      options += "T"
    #endif
    
    2) Use your previous approach with "/*" expansion but with quotes around the paths.
    
    Neither is ideal but is no worse than the system commands we are already invoking.
    
    I am OK with either. What do you think?


- Jiang Yan Xu


On Aug. 31, 2015, 5:58 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Aug. 31, 2015, 5:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
>     https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/
-----------------------------------------------------------

(Updated Sept. 3, 2015, 1:24 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Bugs: MESOS-2968
    https://issues.apache.org/jira/browse/MESOS-2968


Repository: mesos


Description
-------

Add Copy backend for provisioners.


Diffs (updated)
-----

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
  src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 

Diff: https://reviews.apache.org/r/37921/diff/


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/#review97508
-----------------------------------------------------------


Patch looks great!

Reviews applied: [37921]

All tests passed.

- Mesos ReviewBot


On Sept. 2, 2015, 4:54 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2015, 4:54 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
>     https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/#review97509
-----------------------------------------------------------

Ship it!



src/slave/containerizer/provisioners/backends/copy.hpp (lines 41 - 42)
<https://reviews.apache.org/r/37921/#comment153414>

    This requirement is no longer true.



src/slave/containerizer/provisioners/backends/copy.cpp (line 135)
<https://reviews.apache.org/r/37921/#comment153419>

    Are we sure if the layer previous didn't have the trailing slash. 
    
    AppcProvisioner does make sure but within this file perhaps it won't hurt to check. i.e., trim the tail first and then add a trailing list.



src/slave/containerizer/provisioners/backends/copy.cpp (line 139)
<https://reviews.apache.org/r/37921/#comment153420>

    One blank line above.


- Jiang Yan Xu


On Sept. 2, 2015, 9:54 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Sept. 2, 2015, 9:54 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
>     https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/
-----------------------------------------------------------

(Updated Sept. 2, 2015, 4:54 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Bugs: MESOS-2968
    https://issues.apache.org/jira/browse/MESOS-2968


Repository: mesos


Description
-------

Add Copy backend for provisioners.


Diffs (updated)
-----

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
  src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 

Diff: https://reviews.apache.org/r/37921/diff/


Testing
-------

make check


Thanks,

Timothy Chen

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/#review97243
-----------------------------------------------------------


Patch looks great!

Reviews applied: [37921]

All tests passed.

- Mesos ReviewBot


On Sept. 1, 2015, 12:58 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> -----------------------------------------------------------
> 
> (Updated Sept. 1, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2968
>     https://issues.apache.org/jira/browse/MESOS-2968
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
>   src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 
> 
> Diff: https://reviews.apache.org/r/37921/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37921: Add Copy backend for provisioners.

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37921/
-----------------------------------------------------------

(Updated Sept. 1, 2015, 12:58 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Bugs: MESOS-2968
    https://issues.apache.org/jira/browse/MESOS-2968


Repository: mesos


Description
-------

Add Copy backend for provisioners.


Diffs (updated)
-----

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
  src/slave/containerizer/provisioners/backend.cpp 2f7c335f62fdeb27526ab9a38a07c097422ae92b 
  src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_backend_tests.cpp d321850613223a2357ca1646a9d988d05171772c 

Diff: https://reviews.apache.org/r/37921/diff/


Testing
-------

make check


Thanks,

Timothy Chen