Review Request 34141: AppC provsioning backend.

[Ian Downes]

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

Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Repository: mesos


Description
---

Simple copy backend that forms the rootfs by copying each layer.


Diffs
-

  src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
  src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
  src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
  src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34141: AppC provsioning backend.

[Chi Zhang]

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



src/slave/containerizer/provisioners/appc/backend.hpp


Not a big deal, but would it be more flexible for Bankend to take a 
rootImage as an argument instead of a vector, which is already flattened?

Resolving an dependency tree could yield a list of all tree nodes for the 
copy backend.



src/slave/containerizer/provisioners/appc/backend.cpp


put a nothing into the list before the for loop?


- Chi Zhang


On May 13, 2015, 12:48 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated May 13, 2015, 12:48 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Paul Brett]

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



src/slave/containerizer/provisioners/appc/backend.cpp


Use --archive to preserver permissions, timestamps etc?



src/slave/containerizer/provisioners/appc/backend.cpp


Running this as an unconstrained subprocess risks impacting the performance 
of every container on the system.  Could we not launch this inside a container?


- Paul Brett


On May 13, 2015, 12:48 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated May 13, 2015, 12:48 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Ian Downes]

> On May 18, 2015, 4:38 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc/backend.hpp, line 50
> > 
> >
> > Not a big deal, but would it be more flexible for Bankend to take a 
> > rootImage as an argument instead of a vector, which is already flattened?
> > 
> > Resolving an dependency tree could yield a list of all tree nodes for 
> > the copy backend.

I want Backends to be simple and deal only with the flattened list of layers. 
This could be changed in the future if smarter Backends are implemented.


- Ian


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


On May 12, 2015, 5:48 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated May 12, 2015, 5:48 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Ian Downes]

> On May 21, 2015, 1:25 p.m., Paul Brett wrote:
> > src/slave/containerizer/provisioners/appc/backend.cpp, line 136
> > 
> >
> > Running this as an unconstrained subprocess risks impacting the 
> > performance of every container on the system.  Could we not launch this 
> > inside a container?

A full container is not necessary here but there is a ticket open for running 
it with a reduced IO priority to avoid impacting tasks.


- Ian


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


On May 12, 2015, 5:48 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated May 12, 2015, 5:48 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Ian Downes]

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

(Updated June 22, 2015, 9:53 a.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Flag rename, code cleanup, async rm.


Repository: mesos


Description
---

Simple copy backend that forms the rootfs by copying each layer.


Diffs (updated)
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34141: AppC provsioning backend.

[Ian Downes]

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

(Updated July 7, 2015, 12:43 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

Simple copy backend that forms the rootfs by copying each layer.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34141: AppC provsioning backend.

[Timothy Chen]

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



src/slave/containerizer/provisioners/appc/backend.cpp (line 139)


Should we try to keep the logging of each image provisioning to a seperate 
log file in the image folder, instead of just writing out to stdout/stderr?


- Timothy Chen


On July 7, 2015, 7:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 7:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/backend.hpp (lines 25 - 28)


Alphabetic order.



src/slave/containerizer/provisioners/appc/backend.hpp (line 49)


s/image/images.



src/slave/containerizer/provisioners/appc/backend.hpp (lines 98 - 99)


Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (line 1)


Kill this line.



src/slave/containerizer/provisioners/appc/backend.cpp (lines 20 - 28)


Reorder includes.



src/slave/containerizer/provisioners/appc/backend.cpp (line 109)


Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (lines 119 - 120)


Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (line 130)


OSX [doesn't take this 
flag](https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/cp.1.html).

How about `-a`?



src/slave/containerizer/provisioners/appc/backend.cpp (lines 147 - 156)


The style guilde requires:

```
// 2: OK (when no chaining, compare to 1).
instance.method([]() {
  ...;
});
```

i.e. 2 space indentation for the body and have the closing brace aligned 
with the dot in ".then".



src/slave/containerizer/provisioners/appc/backend.cpp (lines 179 - 188)


Ditto.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/backend.hpp (line 41)


In terms of the name `Backend`, I am not sure if it captures what it does 
precisely.

There is Provisioner::provision() and there is Backend::provision().

If "provision" means the overarching action then this specific step is 
really "installing" the downloaded images.

Thoughts?



src/slave/containerizer/provisioners/appc/backend.cpp (lines 128 - 133)


For this to work, should we check the pre-condition: `directory` has to 
already exist,  otherwise `rootfs` is copied to `directory` rather than 
`directory/rootfs`.

I assume this is what we want given the bind mount backend implemetation:

A simple illustration of directory layout (in `paths.hpp`) is hugely 
helpful.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On July 7, 2015, 7:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 7:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>