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/05/13 02:48:15 UTC
Review Request 34142: AppC provisioner.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/
-----------------------------------------------------------
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
-------
Discovers image(s), fetches to the image store, then provisions using
a backend.
Diffs
-----
src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
Diff: https://reviews.apache.org/r/34142/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34142: AppC provisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 2, 2015, 1:48 a.m., Timothy Chen wrote:
> > include/mesos/mesos.proto, line 1300
> > <https://reviews.apache.org/r/34142/diff/2/?file=989783#file989783line1300>
> >
> > I believe we discussed this, but different acVersion will most likely have different schema.
> >
> > Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too.
>
> Timothy Chen wrote:
> Actually at this point seems like AppC image spec most likely won't have a new version with OCP, so perhaps we don't have to. I'll let you decide :)
We should at least say in the comment what acVersion this proto definiton is based on.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90204
-----------------------------------------------------------
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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 12:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Timothy Chen <tn...@apache.org>.
> On July 2, 2015, 8:48 a.m., Timothy Chen wrote:
> > include/mesos/mesos.proto, line 1300
> > <https://reviews.apache.org/r/34142/diff/2/?file=989783#file989783line1300>
> >
> > I believe we discussed this, but different acVersion will most likely have different schema.
> >
> > Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too.
Actually at this point seems like AppC image spec most likely won't have a new version with OCP, so perhaps we don't have to. I'll let you decide :)
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90204
-----------------------------------------------------------
On June 22, 2015, 4:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:57 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90204
-----------------------------------------------------------
include/mesos/mesos.proto (line 1300)
<https://reviews.apache.org/r/34142/#comment143208>
I believe we discussed this, but different acVersion will most likely have different schema.
Unless we intend to only support one version (or anything that's backward compatible), we should version the protobuf too.
- Timothy Chen
On June 22, 2015, 4:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:57 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review94969
-----------------------------------------------------------
src/slave/containerizer/provisioners/appc.cpp (lines 471 - 481)
<https://reviews.apache.org/r/34142/#comment149709>
Will move this into the bind mount backend.
- 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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 12:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review91503
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (line 65)
<https://reviews.apache.org/r/34142/#comment144997>
Probably left over from a previous review?
src/slave/containerizer/provisioners/appc.hpp (line 54)
<https://reviews.apache.org/r/34142/#comment144958>
Who outside appc.cpp uses it?
src/slave/containerizer/provisioners/appc.hpp (line 56)
<https://reviews.apache.org/r/34142/#comment144959>
I see that this is used by Store. Possible to move to the Store base class to break circular dependency?
src/slave/containerizer/provisioners/appc.hpp (lines 58 - 61)
<https://reviews.apache.org/r/34142/#comment144938>
Looks like this is only used in discovery. Can it be moved to Discovery so we can break the
Dicovery -> Appc -> Discovery review dependency?
src/slave/containerizer/provisioners/appc.hpp (lines 63 - 67)
<https://reviews.apache.org/r/34142/#comment144939>
Move to cpp if this is only used by appc.cpp?
src/slave/containerizer/provisioners/appc.hpp (lines 64 - 67)
<https://reviews.apache.org/r/34142/#comment144940>
Indentation.
src/slave/containerizer/provisioners/appc.hpp (line 93)
<https://reviews.apache.org/r/34142/#comment144933>
explicit
src/slave/containerizer/provisioners/appc.hpp (line 101)
<https://reviews.apache.org/r/34142/#comment144936>
Move to cpp?
src/slave/containerizer/provisioners/appc.hpp (line 125)
<https://reviews.apache.org/r/34142/#comment145668>
According the spec the ordering is significant so we better document it here.
In fact the implementation does return the list of image in **topological** order (dependencies go before dependents is the list), which is great. It doesn't address duplicates though, which is something we can improve.
src/slave/containerizer/provisioners/appc.hpp (line 127)
<https://reviews.apache.org/r/34142/#comment145221>
s/hash/id/ ?
As I commented on another review, I think it's less error-prone if we consistently use `id` rather than `hash`.
src/slave/containerizer/provisioners/appc.hpp (line 132)
<https://reviews.apache.org/r/34142/#comment145222>
s/hash/id/ ?
src/slave/containerizer/provisioners/appc.hpp (line 148)
<https://reviews.apache.org/r/34142/#comment145208>
What are additional fileds you imagine may be added in the future?
src/slave/containerizer/provisioners/appc.cpp (line 56)
<https://reviews.apache.org/r/34142/#comment144957>
Such as checking `if(acKind == "ImageManifest")`?
src/slave/containerizer/provisioners/appc.cpp (line 84)
<https://reviews.apache.org/r/34142/#comment145514>
IIUC about the spec there is not a "canonical name" but this is a template for SimpleDiscovery. e.g. MetaDiscovery, if one were to implement it, doesn't follow this.
Can we move it to SimpleDisovery and make LocalDiscovery (which may be more aptly named LocalSimpleDiscovery?) a subclass of SimpleDiscovery?
src/slave/containerizer/provisioners/appc.cpp (line 113)
<https://reviews.apache.org/r/34142/#comment145519>
Given the TODO above I am not sure if we still should do this at all. Maybe it's sufficient to just fill in the missing "arch" label and trust the operator will do the right thing?
src/slave/containerizer/provisioners/appc.cpp (lines 287 - 288)
<https://reviews.apache.org/r/34142/#comment145564>
Inline this? Moreover, with the path manipulation getting hairy in the code (different places remove and append "rootfs") I think we'd better do something akin to **slave/paths.hpp**.
src/slave/containerizer/provisioners/appc.cpp (lines 322 - 323)
<https://reviews.apache.org/r/34142/#comment145930>
This is probably where the lambda becomes to long and a callback with a descriptive name (the old style) is better?
src/slave/containerizer/provisioners/appc.cpp (lines 342 - 343)
<https://reviews.apache.org/r/34142/#comment145762>
We typically break after the operator I think :) (e.g. https://github.com/apache/mesos/blob/bfe6c07b79550bb3d1f2ab6f5344d740e6eb6f60/3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp#L494)
src/slave/containerizer/provisioners/appc.cpp (lines 349 - 351)
<https://reviews.apache.org/r/34142/#comment145253>
<< 2 spaces
src/slave/containerizer/provisioners/appc.cpp (lines 365 - 375)
<https://reviews.apache.org/r/34142/#comment145464>
Indentation: it should be two spaces right of the dot on `.then()`.
src/slave/containerizer/provisioners/appc.cpp (line 409)
<https://reviews.apache.org/r/34142/#comment145231>
Misaligned.
src/slave/containerizer/provisioners/appc.cpp (line 423)
<https://reviews.apache.org/r/34142/#comment145463>
fetchAll() already recursively traverses the dependency graph right?
src/slave/containerizer/provisioners/appc.cpp (line 424)
<https://reviews.apache.org/r/34142/#comment145797>
There is possibility of poentially downloading identical images if done in parallel right? Also we need to maintain the image provisioning order.
src/slave/containerizer/provisioners/appc.cpp (line 446)
<https://reviews.apache.org/r/34142/#comment145775>
Be more explicit with `Option<string>::none()`?
- 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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 12:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review97499
-----------------------------------------------------------
This is no longer valid right? Can we discard all ian's patches?
- 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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 7:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/
-----------------------------------------------------------
(Updated July 7, 2015, 12:43 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
-------
Discovers image(s), fetches to the image store, then provisions using
a backend.
Diffs
-----
include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
Diff: https://reviews.apache.org/r/34142/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34142: AppC provisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 143-151
> > <https://reviews.apache.org/r/34142/diff/2/?file=989787#file989787line143>
> >
> > What if the candidate is over-specified (has more labels)? Should this still be a match?
According to the spec it should be a match.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90173
-----------------------------------------------------------
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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 12:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 203
> > <https://reviews.apache.org/r/34142/diff/2/?file=989787#file989787line203>
> >
> > Maybe generalize the Failure message to something like "Provisioner and Image type mismatch"
Changed to `return Failure("Unsupported container image type: " + stringify(image.type()));`
> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 484-487
> > <https://reviews.apache.org/r/34142/diff/2/?file=989787#file989787line484>
> >
> > Do we want to remove everything on a failure? This could make finding the failure point difficult.
We can probably leave the rootfs dir behind to make it easier to debug.
But another issue is that we cannot actually directly remove the rootfs here because it could be in a state that additionally procedures are necessary to reverse the effect of a partially provisioned rootfs. (e.g. If multiple mounts are required for some backends such as overlayfs and the failure occurred between them).
The important thing is that the provisioner is not supposed to know all these things, the backends are.
Added a TODO to sort this out.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90173
-----------------------------------------------------------
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/34142/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 12:43 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Lily Chen <li...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review90173
-----------------------------------------------------------
src/slave/containerizer/provisioners/appc.cpp (lines 143 - 151)
<https://reviews.apache.org/r/34142/#comment143167>
What if the candidate is over-specified (has more labels)? Should this still be a match?
src/slave/containerizer/provisioners/appc.cpp (line 203)
<https://reviews.apache.org/r/34142/#comment143168>
Maybe generalize the Failure message to something like "Provisioner and Image type mismatch"
src/slave/containerizer/provisioners/appc.cpp (lines 484 - 487)
<https://reviews.apache.org/r/34142/#comment143174>
Do we want to remove everything on a failure? This could make finding the failure point difficult.
- Lily Chen
On June 22, 2015, 4:57 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:57 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
> src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
> src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
> src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/
-----------------------------------------------------------
(Updated June 22, 2015, 9:57 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
- Use protobuf representation of Appc Image Manifest
- Implemented recover
Repository: mesos
Description
-------
Discovers image(s), fetches to the image store, then provisions using
a backend.
Diffs (updated)
-----
include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712
src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b
src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2
Diff: https://reviews.apache.org/r/34142/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34142: AppC provisioner.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review83564
-----------------------------------------------------------
Bad patch!
Reviews applied: [34134, 34135, 34134]
Failed command: ./support/apply-review.sh -n -r 34134
Error:
2015-05-13 05:25:01 URL:https://reviews.apache.org/r/34134/diff/raw/ [18243/18243] -> "34134.patch" [1]
error: patch failed: include/mesos/slave/isolator.hpp:98
error: include/mesos/slave/isolator.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolator.cpp:57
error: src/slave/containerizer/isolator.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/cpushare.hpp:48
error: src/slave/containerizer/isolators/cgroups/cpushare.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/cpushare.cpp:261
error: src/slave/containerizer/isolators/cgroups/cpushare.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/mem.hpp:48
error: src/slave/containerizer/isolators/cgroups/mem.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/mem.cpp:240
error: src/slave/containerizer/isolators/cgroups/mem.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/perf_event.hpp:46
error: src/slave/containerizer/isolators/cgroups/perf_event.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/cgroups/perf_event.cpp:225
error: src/slave/containerizer/isolators/cgroups/perf_event.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/filesystem/shared.hpp:47
error: src/slave/containerizer/isolators/filesystem/shared.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/filesystem/shared.cpp:76
error: src/slave/containerizer/isolators/filesystem/shared.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/namespaces/pid.hpp:64
error: src/slave/containerizer/isolators/namespaces/pid.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/namespaces/pid.cpp:157
error: src/slave/containerizer/isolators/namespaces/pid.cpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/posix.hpp:66
error: src/slave/containerizer/isolators/posix.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/posix/disk.hpp:85
error: src/slave/containerizer/isolators/posix/disk.hpp: patch does not apply
error: patch failed: src/slave/containerizer/isolators/posix/disk.cpp:105
error: src/slave/containerizer/isolators/posix/disk.cpp: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:609
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
error: patch failed: src/tests/containerizer_tests.cpp:73
error: src/tests/containerizer_tests.cpp: patch does not apply
error: patch failed: src/tests/isolator.hpp:45
error: src/tests/isolator.hpp: patch does not apply
error: patch failed: src/tests/isolator_tests.cpp:171
error: src/tests/isolator_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
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/34142/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Ian Downes <ia...@gmail.com>.
> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, line 50
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line50>
> >
> > nit: return type could be const?
Why? I don't see what it would add?
> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 60-62
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line60>
> >
> > If we say the dependency itself "is an" AppcImage that "has n" AppcImage(dependencies). We can kill the dependency type?
> >
> > AppcImage could then recursively resolve the dependencies on its own, using the composite pattern described above, i.e., AppcProvisionerProcess simply needs to call rootImage.resolve(), who loops over dependent images and calls image.resolve().
> >
> > We can then have fetch, fetchDependencies (not needed anymore), fetchURIs and match togerther go into AppcImage::resolve. AppcImage will now need functionality provided by discover and store; AppcPP only uses backend and does mounts.
The distinction is that AppcImage::Dependency is something that is desired, and is specified by a name plus possibly labels and a hash, but has not been realized (fetched). An AppcImage only refers to an actual image that has been fetched and its properties come from the actual image data (extracted from the json manifest and or by computing the hash).
AppcImage(::Dependency) is meant to be a simple data structure, the coordination of the discovery, fetch and store is coordinated by the provisioner.
> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, line 101
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line101>
> >
> > nit: newline after ','
std::string, std::string are the template parameters for the hashmap, i.e., the whole thing forms the type for labels.
> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 74-75
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line74>
> >
> > const-able?
Code refactored to remove this.
> On May 18, 2015, 3:20 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.hpp, lines 135-139
> > <https://reviews.apache.org/r/34142/diff/1/?file=957287#file957287line135>
> >
> > some of these should be const?
Code refactored to remove this.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review84199
-----------------------------------------------------------
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/34142/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:48 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34142: AppC provisioner.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34142/#review84199
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34142/#comment135361>
can tweak the constructors so that the containerizer doesn't need to be exposed to ProvisionerProcesses, which is implementation detail?
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135339>
nit: return type could be const?
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135375>
If we say the dependency itself "is an" AppcImage that "has n" AppcImage(dependencies). We can kill the dependency type?
AppcImage could then recursively resolve the dependencies on its own, using the composite pattern described above, i.e., AppcProvisionerProcess simply needs to call rootImage.resolve(), who loops over dependent images and calls image.resolve().
We can then have fetch, fetchDependencies (not needed anymore), fetchURIs and match togerther go into AppcImage::resolve. AppcImage will now need functionality provided by discover and store; AppcPP only uses backend and does mounts.
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135340>
const-able?
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135341>
nit: newline after ','
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135342>
nit: craft a list and then return "-".join(list)?
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135343>
some of these should be const?
src/slave/containerizer/provisioners/appc.hpp
<https://reviews.apache.org/r/34142/#comment135358>
nit: const-able?
src/slave/containerizer/provisioners/appc.cpp
<https://reviews.apache.org/r/34142/#comment135360>
Is it possible to tweak the store{process}'s constructors to hide the implemention detail of there existing a StoreProcess?
- 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/34142/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
>
>
> Diffs
> -----
>
> src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
> src/slave/containerizer/provisioners/appc.hpp PRE-CREATION
> src/slave/containerizer/provisioners/appc.cpp PRE-CREATION
> src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032
> src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932
>
> Diff: https://reviews.apache.org/r/34142/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>