Re: Review Request 37881: Implemented AppcProvisioner.

2015-09-02 Thread Jiang Yan Xu 


> On Aug. 31, 2015, 3:50 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 381-391
> > 
> >
> > Could it be possible that one backend return Failure while the other 
> > one returns true?

Nope. It will fail the `collect()`. But anyways I am now always returning a 
true as long as `collect()` is ready.


- Jiang Yan


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


On Sept. 2, 2015, 3:47 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Sept. 2, 2015, 3:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, 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.

2015-09-02 Thread Jiang Yan Xu 


> On Aug. 31, 2015, 10:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, line 47
> > 
> >
> > Can you just use a string->create func hashmap so that you don't need 
> > Image::Type_Parse.
> > 
> > ```
> > hashmap creators = {
> >   {"appc", ::AppcProvisioner::create}
> > }
> > ```

If the creator uses string keys and the result provisioner map is keyed by 
Image::Type (i.e. `hashmap`), there is a 
mismatch and I have to convert the string "appc" into Image::APPC, which I 
either use another protobuf generated method or hard code it, either of which 
is cleaner than the current form.


> On Aug. 31, 2015, 10:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 180-187
> > 
> >
> > AppcProvisioner is a wrapper of AppcProvisionerProcess. In our code 
> > base, we delegate all calls to the underlying process. There're several 
> > benefits of this. One benefit is that all logics are in the same function 
> > (not splitted like this case).
> > 
> > I still suggest that you move those checks to 
> > AppcProvisionerProcess::provision.

Oh I thought this was in AppcProvisioner::create() (review board gave me the 
wrong idea). For AppcProvisioner::provision() of course, this was a oversight.


> On Aug. 31, 2015, 10:33 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, line 228
> > 
> >
> > What about those containers that does not specify a container info but 
> > we filled in with default container info with appc image? What should we do?

Oh yes, this is important. Thanks for noticing! It should be now correctly 
handled.


- Jiang Yan


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


On Sept. 2, 2015, 3:47 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Sept. 2, 2015, 3:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, 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.

2015-09-02 Thread Jiang Yan Xu 

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

(Updated Sept. 2, 2015, 3:47 p.m.)


Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.


Changes
---

Comments and some refactoring in paths.cpp.


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.

2015-08-30 Thread Jie Yu 


 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.

2015-08-30 Thread Timothy Chen 

---
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.

2015-08-30 Thread Jiang Yan Xu 

---
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.

2015-08-30 Thread Mesos ReviewBot 

---
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.

2015-08-30 Thread Jiang Yan Xu 


 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.

2015-08-30 Thread Jiang Yan Xu 


 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 `Tryhashmapstring, OwnedBackend`?

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.

2015-08-29 Thread Jiang Yan Xu 


 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.

2015-08-29 Thread Jiang Yan Xu 

---
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.

2015-08-28 Thread Jie Yu 


 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:

Re: Review Request 37881: Implemented AppcProvisioner.

2015-08-28 Thread Jie Yu 


 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.

2015-08-28 Thread Jiang Yan Xu 


 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.

2015-08-28 Thread Jie Yu 

---
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