mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 37881: Implemented AppcProvisioner.
Date Fri, 28 Aug 2015 21:49:57 GMT


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, lines 43-46
> > <https://reviews.apache.org/r/37881/diff/1/?file=1057720#file1057720line43>
> >
> >     I would love to get this TODO solved in this patch. It should be pretty straightfoward,
right? Just hard code them. And right now, only Appc is supported.

Fine with me.


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 79-97
> > <https://reviews.apache.org/r/37881/diff/1/?file=1057722#file1057722line79>
> >
> >     Some high level comments.
> >     
> >     I think it will be beneficial if we can move the fetch and discovery logic to
the Store. For the MVP, we don't have to impl. that in the Store.
> >     
> >     The Store only has one interface, which is the 'get' method. 'get' takes an
Image::Appc and returns a vector of paths to layers. THis semantics is more natural and easy
to understand: get layers for my image, if some layers are already cached, return them directly,
otherwise, discover it and fetch it.
> >     
> >     The job of the provisioner is to get layers from the Store and stack them into
a rootfs using the backend. It also manages the rootfs directories.
> >     
> >     To me, this is a more natural split of functionalities among components. In
the future, we can also potentially unify the impl. of the provisioner (i.e., one single provisioner
for both Docker and Appc images). All the image specific details are hiden in the Store interface.
> >     
> >     Thoughts?

I am OK the this direction but a few points:

1) I think a "dumb" store that only returns what it has and stores what is given to it is
natual too. (So how about stripping the fetching logic from the store and put it in the provisioner?
This way the provisioner is the "manager" that controls all the dumb single-purpose helpers.)

2) Unifying the provisioner would be nice but one of the remaining obstacles is that we need
to make sure we have a common "provisioner direcotry layout", otherwise they need to be further
extracted out from the provisioner and the provisioner becomes so thin that there is no point
in having a unified provisioner anymore. --> So I am not totally sure if this will happen.

3) In the future there may be a lot of more logic required for the new Store if it is responsible
for doing all the heavy-lifting: cache eviction, P2P fetching, registering new provision requests
with ongoing fetching processes, etc. But I guess this is not an argument for keeping these
in the provisioner, we should create other single purpose class to handle them, we just need
to design a Store API that's takes these into consideration.

For now since none of these aformentioned functionality is necessary for our "read-only, no
dependency" provisioner let's find a way to check this as long as it doesn't make it hard
to refactor in the future.


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/flags.cpp, lines 72-76
> > <https://reviews.apache.org/r/37881/diff/1/?file=1057726#file1057726line72>
> >
> >     Why do you still need this flag?

We instantiate all supported backends but this flag specifies the user's choice for new images.
I don't think we can make a perfect guess based soley on system compatibility: what if the
user only has small images and don't have the mount point created?


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, line 27
> > <https://reviews.apache.org/r/37881/diff/1/?file=1057720#file1057720line27>
> >
> >     See my comments below. This is no longer needed.

OK.


- Jiang Yan


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


On Aug. 28, 2015, 2:04 a.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
>     https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829

>   src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7

>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020

> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> -------
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message