You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2015/08/28 11:03:46 UTC
Review Request 37881: Implemented AppcProvisioner.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/
-----------------------------------------------------------
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
Thanks,
Jiang Yan Xu
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner.cpp, line 42
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059944#file1059944line42>
> >
> > I think we should only allow creating the appc provisioner with linux, since I believe most of the appc components requires it right?
With CopyBackend it's not, right?
> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioner.hpp, line 48
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059943#file1059943line48>
> >
> > Perhaps its worth mentioning if any of the provisioner is failed to create.
The comment is now:
```
// Create provisioners based on specified flags. An error is returned if
// any of the provisioners specified in --provisioner failed to be created.
```
Reverted the Provisioner::create() semantics. See my reply to Jie's comment above.
> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 106
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line106>
> >
> > I can't remember, but if the directory exists does it still throw as well?
> >
> > We should assume the directory should be there after recovery?
If the directory already exists it succeeds. os::mkdir() ignores any EEXIST by default (which is convenient!)
> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 125
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line125>
> >
> > Why does this not return a Try? We always have a backend specified right?
You mean `Try<hashmap<string, Owned<Backend>>>`?
The thought was that we are always going to attempt to create all supported backends, and some of them will fail on some systems e.g., OverlayBackend, but its OK. We do this in order to protect against potential leaked mounts after --appc_backend flag change.
Therefore the "error" case is captured by the hashmap being empty and a Try is unnecessary.
> On Aug. 30, 2015, 12:33 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 276
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line276>
> >
> > Ideally we should still log the return value though.
Now doing
```
backends[backend]->destroy(rootfs)
.onFailed([rootfs](const std::string& error) {
LOG(WARNING) << "Failed to destroy orphan rootfs '" << rootfs
<< "': "<< error;
});
```
to log it as a warning.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review97015
-----------------------------------------------------------
On Aug. 30, 2015, 6:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 6:49 p.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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review97015
-----------------------------------------------------------
src/slave/containerizer/provisioner.hpp (line 48)
<https://reviews.apache.org/r/37881/#comment152722>
Perhaps its worth mentioning if any of the provisioner is failed to create.
src/slave/containerizer/provisioner.cpp (line 42)
<https://reviews.apache.org/r/37881/#comment152731>
I think we should only allow creating the appc provisioner with linux, since I believe most of the appc components requires it right?
src/slave/containerizer/provisioner.cpp (line 64)
<https://reviews.apache.org/r/37881/#comment152724>
This should be provisioners[imageType]
src/slave/containerizer/provisioners/appc.cpp (line 106)
<https://reviews.apache.org/r/37881/#comment152725>
I can't remember, but if the directory exists does it still throw as well?
We should assume the directory should be there after recovery?
src/slave/containerizer/provisioners/appc.cpp (line 125)
<https://reviews.apache.org/r/37881/#comment152726>
Why does this not return a Try? We always have a backend specified right?
src/slave/containerizer/provisioners/appc.cpp (line 268)
<https://reviews.apache.org/r/37881/#comment152727>
should be if (!backends.contains(backend)) ?
src/slave/containerizer/provisioners/appc.cpp (line 276)
<https://reviews.apache.org/r/37881/#comment152728>
Ideally we should still log the return value though.
src/slave/containerizer/provisioners/appc.cpp (line 358)
<https://reviews.apache.org/r/37881/#comment152730>
s/by provisioned by/provisioned by/
- Timothy Chen
On Aug. 30, 2015, 4:28 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 4:28 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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Aug. 30, 2015, 12:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 83
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line83>
> >
> > Why not const as well?
Because operator[] on backends is not const. But I switched to use `backends.get(backend).get()` to avoid using `[]` and made it const.
> On Aug. 30, 2015, 12:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 131-132
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line131>
> >
> > Let's try to make the indentation in this file consistent (don't mix).
> >
> > Either
> > ```
> > return Error(xxx,
> > yyy);
> > ```
> >
> > Or
> > ```
> > return Error(
> > xxx,
> > yyy);
> > ```
Fixed a few inconsistencies to use option 1 for string concatenations.
> On Aug. 30, 2015, 12:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 174-181
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line174>
> >
> > Let's put all the logics in AppcProvisionerProcess (which is the convention we used consistently in the code base).
In store.cpp there used to be Store::create() and StoreProcess::create() and Store::create() simply called StoreProcess::create() and had StoreProcess::create() handle argument validation and preparation and your comment was to get rid of StoreProcess::create() and just do the validation in Store::create() and then call `StoreProcess::StoreProcess()`: https://reviews.apache.org/r/37311/diff/2?file=1036566#file1036566line50
It is the same situation here.
FWIW `MesosContainerizer::create()` does similar validation and IIRC the rule is for regular methods: Inserting logic into `XYZ::doSomething()` before it delegates to `XYZProcess::doSomething()` is aganist the expectation that `XYZ::doSomething()` is entirely asychronous.
> On Aug. 30, 2015, 12:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 328-332
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line328>
> >
> > We try to avoid race as much as possible, even if 'backends' here is read only.
> >
> > Can you pull this code into `_provision`? I found it more readable then using a lambda here.
> >
> > Lambda is useful if it does not rely on 'this' (i.e., a static method).
> >
> > ```
> > return store->get(image)
> > .then(defer(self(), &Self::_provision, containerId, rootfs, lambda::_1));
> >
> > Future<string> _provision(...)
> > {
> > ...
> > return backends[..]->provision(layers, rootfs)
> > .then([rootfs]() { return rootfs; });
> > }
> > ```
First-time labmda user here, thanks!
> On Aug. 30, 2015, 12:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 372-381
> > <https://reviews.apache.org/r/37881/diff/2/?file=1059946#file1059946line372>
> >
> > Hum, that makes me wonder if we need to return bool for backends. Looks like that's not necessary. Also, you need to consider the case where provision fails partially.
FWIW current linux.cpp is not reading the result from `Future<bool> Provisioner::destory()` either, but let's revist this after this patch.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review97014
-----------------------------------------------------------
On Aug. 30, 2015, 6:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 6:49 p.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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review97014
-----------------------------------------------------------
1st pass. Haven't checked the recovery logic yet.
src/slave/containerizer/provisioners/appc.cpp (line 80)
<https://reviews.apache.org/r/37881/#comment152716>
Maybe s/provisionerDir/root/ ?
The 'provisioner' part is redundant because the class name already contains "Privisioner".
src/slave/containerizer/provisioners/appc.cpp (line 83)
<https://reviews.apache.org/r/37881/#comment152715>
Why not const as well?
src/slave/containerizer/provisioners/appc.cpp (line 106)
<https://reviews.apache.org/r/37881/#comment152718>
"... provisioner root directory"
src/slave/containerizer/provisioners/appc.cpp (line 114)
<https://reviews.apache.org/r/37881/#comment152719>
Ditto.
src/slave/containerizer/provisioners/appc.cpp (line 118)
<https://reviews.apache.org/r/37881/#comment152717>
CHECK_SOME
src/slave/containerizer/provisioners/appc.cpp (lines 131 - 132)
<https://reviews.apache.org/r/37881/#comment152720>
Let's try to make the indentation in this file consistent (don't mix).
Either
```
return Error(xxx,
yyy);
```
Or
```
return Error(
xxx,
yyy);
```
src/slave/containerizer/provisioners/appc.cpp (lines 174 - 181)
<https://reviews.apache.org/r/37881/#comment152721>
Let's put all the logics in AppcProvisionerProcess (which is the convention we used consistently in the code base).
src/slave/containerizer/provisioners/appc.cpp (lines 328 - 332)
<https://reviews.apache.org/r/37881/#comment152729>
We try to avoid race as much as possible, even if 'backends' here is read only.
Can you pull this code into `_provision`? I found it more readable then using a lambda here.
Lambda is useful if it does not rely on 'this' (i.e., a static method).
```
return store->get(image)
.then(defer(self(), &Self::_provision, containerId, rootfs, lambda::_1));
Future<string> _provision(...)
{
...
return backends[..]->provision(layers, rootfs)
.then([rootfs]() { return rootfs; });
}
```
src/slave/containerizer/provisioners/appc.cpp (lines 334 - 339)
<https://reviews.apache.org/r/37881/#comment152723>
I think the containerizer will still call containerizer->destroy if provisioning fails. That means the filesystem isolator cleanup will be invoked, meaning that provisioner->destroy will be called as well.
So, I think we should rely on 'destroy' here to cleanup those partially constructed rootfses.
In other words, no need for the onFailed callback here.
I would suggest you take a look at the port mapping isolator.
src/slave/containerizer/provisioners/appc.cpp (lines 365 - 367)
<https://reviews.apache.org/r/37881/#comment152732>
Let's try not to abuse lambda here.
This code is buggy because 'info.erase' can be executed in other threads! Race condition!
Also, if you take a look at the port mapping isolator. We remove Info from infos first and then start to do cleaning. The cleaning can fail of course. In that case, we rely on the 'recovery' logic to properly clean them up later.
src/slave/containerizer/provisioners/appc.cpp (lines 372 - 381)
<https://reviews.apache.org/r/37881/#comment152734>
Hum, that makes me wonder if we need to return bool for backends. Looks like that's not necessary. Also, you need to consider the case where provision fails partially.
- Jie Yu
On Aug. 30, 2015, 4:28 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 4:28 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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review97052
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37929, 37880, 37881]
All tests passed.
- Mesos ReviewBot
On Aug. 31, 2015, 1:49 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2015, 1:49 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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/
-----------------------------------------------------------
(Updated Aug. 30, 2015, 6:49 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
Tim and Jie's comments.
Bugs: MESOS-2796
https://issues.apache.org/jira/browse/MESOS-2796
Repository: mesos
Description
-------
Implemented AppcProvisioner.
Diffs (updated)
-----
src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99
src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
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
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/
-----------------------------------------------------------
(Updated Aug. 29, 2015, 9:28 p.m.)
Review request for mesos, Jie Yu and Timothy Chen.
Changes
-------
Jie's comments. Note that the changes for recommended store API change are done here: https://reviews.apache.org/r/37929/
Bugs: MESOS-2796
https://issues.apache.org/jira/browse/MESOS-2796
Repository: mesos
Description
-------
Implemented AppcProvisioner.
Diffs (updated)
-----
src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99
src/slave/containerizer/provisioner.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
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
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96855
-----------------------------------------------------------
Patch looks great!
Reviews applied: [37880, 37881]
All tests passed.
- Mesos ReviewBot
On Aug. 28, 2015, 9: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, 9: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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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.
>
> Jiang Yan Xu wrote:
> Fine with me.
>
> Jiang Yan Xu wrote:
> I have changed the semantics to Provisioner::create to this:
>
> ```
> // Create all supported provisioners. Return error if provisioners
> // explicitly specified in '--provisioners' have failed to be created.
> ```
>
> So we still need the flag.
>
> The reason is that let's say the AppcProvisioner::create() somehow fails while DockerProvisioner::create() succeeds but the operator intends to make sure Appc works within the cluster. He probably wants the slave to fail so he can fix the box rather than waiting until tasks shown as LOST much later. The default for `--provisioners` is empty so there is no burden on the operator if they don't care about this.
>
> Jie Yu wrote:
> Hum, that semantics is even more weired. If that's the case, I would rather stick to the original semantics: only creates provisioner that's specified by the operator and fails if any of them fails to be created. What do you think?
SG. In fact, on second thought, we cannot reliably detect which provisioners "is supported" because it requires image-related infra to be set up for the cluster! It's not a local decision to make and we should rely on the operator's explict intention for this.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 29, 2015, 9:28 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 9:28 p.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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 28, 2015, 7: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?
>
> Jiang Yan Xu wrote:
> 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?
User does not care which backend we use, right? We just pick a backend that works. If there are multiple supported backend, just pick the one that's the best.
The flag is a little wiered to me because what if bind backend is not supported but copy backend is supported, the user specifes the backend to be bind, should we fail, or fall back to the copy backend? I think we can punt on this flag for now since we only have one backend right now.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 28, 2015, 9: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, 9: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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.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?
>
> Jiang Yan Xu wrote:
> 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.
>
> Jie Yu wrote:
> Those are valid points! Let me express my rationale in some depth:
>
> Store manages store_dir and privisioner manages rootfs_dir. A natural split is that: any thing involved in updating 'store_dir' should be handled by Store and anything involved in updating 'rootfs_dir' should be handled by rootfs_dir.
>
> In that sense, fetching and caching should be handled by the Store. As you said, you can introduce sub-component in 'Store' to dealing with Discovery, fetching, caching, etc.
>
> Another reason I want fetching and caching to be done in Store, independent of provisioner, is that: maybe, in the future, we can support 'pre-fetching' which is not associated with any provisioning at all. Putting all such logics in the provisioner will complicate it a lot. A component with less dependency is definitely more managable.
OK I am going to make the recommended change beause I realized that this is basically the two patterns for caching:
1) Cache-aside: the cache returns none if it's not in it and the application reads the data and put it in the cache.
2) Read-through: the cache fetches the data transparently if it's not cached.
We cannot do 1) cleanly because given the size of the data on the disk we need the data to be downloaded directly into the cache and the application shouldn't need to know how the cache manages its data.
So we can just do 2).
- 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 28, 2015, 7: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?
>
> Jiang Yan Xu wrote:
> 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?
>
> Jie Yu wrote:
> User does not care which backend we use, right? We just pick a backend that works. If there are multiple supported backend, just pick the one that's the best.
>
> The flag is a little wiered to me because what if bind backend is not supported but copy backend is supported, the user specifes the backend to be bind, should we fail, or fall back to the copy backend? I think we can punt on this flag for now since we only have one backend right now.
>
> Jiang Yan Xu wrote:
> Sorry I meant the operator and the image in the default container info: If the operator just downloads an Appc image from the interenet which doesn't have the mountpoint for BindBackend but he does't have the choice to choose CopyBackend instead (which we should add soon)..
OK, I think that's fine. Not a big deal.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 28, 2015, 9: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, 9: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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 28, 2015, 7: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?
>
> Jiang Yan Xu wrote:
> 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.
Those are valid points! Let me express my rationale in some depth:
Store manages store_dir and privisioner manages rootfs_dir. A natural split is that: any thing involved in updating 'store_dir' should be handled by Store and anything involved in updating 'rootfs_dir' should be handled by rootfs_dir.
In that sense, fetching and caching should be handled by the Store. As you said, you can introduce sub-component in 'Store' to dealing with Discovery, fetching, caching, etc.
Another reason I want fetching and caching to be done in Store, independent of provisioner, is that: maybe, in the future, we can support 'pre-fetching' which is not associated with any provisioning at all. Putting all such logics in the provisioner will complicate it a lot. A component with less dependency is definitely more managable.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 28, 2015, 9: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, 9: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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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.
>
> Jiang Yan Xu wrote:
> Fine with me.
I have changed the semantics to Provisioner::create to this:
```
// Create all supported provisioners. Return error if provisioners
// explicitly specified in '--provisioners' have failed to be created.
```
So we still need the flag.
The reason is that let's say the AppcProvisioner::create() somehow fails while DockerProvisioner::create() succeeds but the operator intends to make sure Appc works within the cluster. He probably wants the slave to fail so he can fix the box rather than waiting until tasks shown as LOST much later. The default for `--provisioners` is empty so there is no burden on the operator if they don't care about this.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 29, 2015, 9:28 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2015, 9:28 p.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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 28, 2015, 7: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.
>
> Jiang Yan Xu wrote:
> Fine with me.
>
> Jiang Yan Xu wrote:
> I have changed the semantics to Provisioner::create to this:
>
> ```
> // Create all supported provisioners. Return error if provisioners
> // explicitly specified in '--provisioners' have failed to be created.
> ```
>
> So we still need the flag.
>
> The reason is that let's say the AppcProvisioner::create() somehow fails while DockerProvisioner::create() succeeds but the operator intends to make sure Appc works within the cluster. He probably wants the slave to fail so he can fix the box rather than waiting until tasks shown as LOST much later. The default for `--provisioners` is empty so there is no burden on the operator if they don't care about this.
Hum, that semantics is even more weired. If that's the case, I would rather stick to the original semantics: only creates provisioner that's specified by the operator and fails if any of them fails to be created. What do you think?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
On Aug. 30, 2015, 4:28 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2015, 4:28 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.hpp 541dd4e0b2f0c92a45c00cab6132a2be69654838
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> 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?
>
> Jiang Yan Xu wrote:
> 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?
>
> Jie Yu wrote:
> User does not care which backend we use, right? We just pick a backend that works. If there are multiple supported backend, just pick the one that's the best.
>
> The flag is a little wiered to me because what if bind backend is not supported but copy backend is supported, the user specifes the backend to be bind, should we fail, or fall back to the copy backend? I think we can punt on this flag for now since we only have one backend right now.
Sorry I meant the operator and the image in the default container info: If the operator just downloads an Appc image from the interenet which doesn't have the mountpoint for BindBackend but he does't have the choice to choose CopyBackend instead (which we should add soon)..
- 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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37881/#review96903
-----------------------------------------------------------
src/slave/containerizer/provisioner.cpp (line 27)
<https://reviews.apache.org/r/37881/#comment152581>
See my comments below. This is no longer needed.
src/slave/containerizer/provisioner.cpp (lines 43 - 46)
<https://reviews.apache.org/r/37881/#comment152580>
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.
src/slave/containerizer/provisioners/appc.cpp (lines 79 - 97)
<https://reviews.apache.org/r/37881/#comment152598>
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?
src/slave/flags.hpp (line 53)
<https://reviews.apache.org/r/37881/#comment152583>
See my comments below. Why you still need this flag?
src/slave/flags.cpp (lines 72 - 76)
<https://reviews.apache.org/r/37881/#comment152582>
Why do you still need this flag?
- Jie Yu
On Aug. 28, 2015, 9: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, 9: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
>
>
Re: Review Request 37881: Implemented AppcProvisioner.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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 (updated)
-------
sudo make check.
More test cases coming.
Thanks,
Jiang Yan Xu