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

[Timothy Chen]

---
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.

[Timothy Chen]

---
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.

[Mesos ReviewBot]

---
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.

[Jiang Yan Xu]

---
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)


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.

[Timothy Chen]

> On Sept. 2, 2015, 3:07 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/backends/copy.cpp, lines 108-116
> > 
> >
> > 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.

[Timothy Chen]

> On Aug. 31, 2015, 5:50 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/backends/copy.cpp, line 19
> > 
> >
> > Move this below.
> > 
> > ```
> > #include "common/status_utils.hpp"
> > 
> > #include "slave/containerizer/provisioners/backends/copy.hpp"
> > ```

I think our newest style guide points that we include the cpp's main header 
file on the top, but I'm fine moving this down.


> On Aug. 31, 2015, 5:50 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/backends/copy.cpp, lines 127-132
> > 
> >
> > Using shell expansion here has others issues:
> > 
> > 1) Spaces (or other $IFS) in the paths
> > 2) Not sure if filename expansions works the same way across shells.
> > 
> > GNU `cp` has a `--no-target-directory` flag that's handy in this 
> > situation but Mac `cp` doesn't have it.
> > 
> > So I think we should probably just rely on a precondition check: return 
> > Failure if os::exists(rootfs) already exists and remove the mkdir in 
> > `provision()`. This allows us to just use the *argv* form of `subprocess` 
> > with `{"cp", "-a", layer, rootfs}`.
> > 
> > AppcProvisioner already expects the Backend to create the `rootfs` and 
> > DockerProvisioner can do the same. 
> > 
> > We can emphaize this by adding a note to `Backend::provision()` 
> > documentation.
> > 
> > Thoughts?

Ok as long we can make sure provisioners output the same layout I definitely 
prefer just doing a straight copy!
I'll make sure the basename of all layers are the same.


- Timothy


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


On Aug. 29, 2015, 7:59 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37921/
> ---
> 
> (Updated Aug. 29, 2015, 7:59 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Copy backend for provisioners.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   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.

[Timothy Chen]

---
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.

[Timothy Chen]

---
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

Review Request 37921: Add Copy backend for provisioners.

[Timothy Chen]

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

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


Repository: mesos


Description
---

Add Copy backend for provisioners.


Diffs
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  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.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37921]

All tests passed.

- Mesos ReviewBot


On Aug. 29, 2015, 7:59 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/37921/
 ---
 
 (Updated Aug. 29, 2015, 7:59 a.m.)
 
 
 Review request for mesos, Jie Yu and Jiang Yan Xu.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add Copy backend for provisioners.
 
 
 Diffs
 -
 
   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
   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