You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/07/07 21:43:36 UTC
Re: Review Request 34141: AppC provsioning backend.
-----------------------------------------------------------
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.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/34141/#comment143901>
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.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/34141/#comment144904>
Alphabetic order.
src/slave/containerizer/provisioners/appc/backend.hpp (line 49)
<https://reviews.apache.org/r/34141/#comment144691>
s/image/images.
src/slave/containerizer/provisioners/appc/backend.hpp (lines 98 - 99)
<https://reviews.apache.org/r/34141/#comment144685>
Indentation.
src/slave/containerizer/provisioners/appc/backend.cpp (line 1)
<https://reviews.apache.org/r/34141/#comment144693>
Kill this line.
src/slave/containerizer/provisioners/appc/backend.cpp (lines 20 - 28)
<https://reviews.apache.org/r/34141/#comment144907>
Reorder includes.
src/slave/containerizer/provisioners/appc/backend.cpp (line 109)
<https://reviews.apache.org/r/34141/#comment144713>
Indentation.
src/slave/containerizer/provisioners/appc/backend.cpp (lines 119 - 120)
<https://reviews.apache.org/r/34141/#comment144722>
Indentation.
src/slave/containerizer/provisioners/appc/backend.cpp (line 130)
<https://reviews.apache.org/r/34141/#comment144923>
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)
<https://reviews.apache.org/r/34141/#comment144924>
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)
<https://reviews.apache.org/r/34141/#comment144925>
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.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/34141/#comment145761>
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)
<https://reviews.apache.org/r/34141/#comment145250>
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
>
>