Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Jiang Yan Xu 

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

(Updated Aug. 25, 2015, 5:19 p.m.)


Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.


Changes
---

Comments. NNFR.


Bugs: MESOS-3190
https://issues.apache.org/jira/browse/MESOS-3190


Repository: mesos


Description
---

Introduced bind-mount based provisioner Backend.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/backend.hpp 
46120e8420cc491a0decbd88301f89d6dfcff120 
  src/slave/containerizer/provisioners/backend.cpp 
6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
  src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
  src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 

Diff: https://reviews.apache.org/r/37747/diff/


Testing
---

sudo make check. Added one test.


Thanks,

Jiang Yan Xu

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37722, 37747]

All tests passed.

- Mesos ReviewBot


On Aug. 25, 2015, 10:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 10:59 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.hpp 
> 46120e8420cc491a0decbd88301f89d6dfcff120 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/37747/diff/
> 
> 
> Testing
> ---
> 
> sudo make check. Added one test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Jie Yu 

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

Ship it!



src/slave/containerizer/provisioners/backend.cpp (line 24)


Add a blank line above.



src/slave/flags.hpp (line 51)


See my comments below. It's not used, please remove it.



src/slave/flags.cpp (lines 67 - 71)


This is no longer used in this patch. Could you please remove it.



src/tests/containerizer/provisioner_backend_tests.cpp (line 63)


Add one line above.



src/tests/containerizer/provisioner_backend_tests.cpp (line 67)


Kill one line.



src/tests/containerizer/provisioner_backend_tests.cpp (line 85)


You can do:

```
AWAIT_READY(backends["bind"]->provision(...));
```



src/tests/containerizer/provisioner_backend_tests.cpp (line 95)


backends["bind"]->destroy(...)


- Jie Yu


On Aug. 25, 2015, 10:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 10:59 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.hpp 
> 46120e8420cc491a0decbd88301f89d6dfcff120 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/37747/diff/
> 
> 
> Testing
> ---
> 
> sudo make check. Added one test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Jiang Yan Xu 


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 41
> > 
> >
> > No need for the process ID generation unless it's proven needed.

It's much harder to do this when debugging becauase you don't know which 
process to add the id to: https://issues.apache.org/jira/browse/MESOS-1457

But I understand that we are not doing this consistently. OK for dropping it. 
/sigh


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 45
> > 
> >
> > I would sugguest you save the slave flags at least.
> > 
> > ```
> > public:
> >   BindBackendProcess(const Flags& _flags)
> > : flags(_flags) {}
> > 
> > private:
> >   const Flags flags;
> > ```

Chatted offline. Not saving it for now because it's not needed for BindBackend. 
we can add it back when necessary of course.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 107
> > 
> >
> > Can you try if MS_BIND | MS_RDONLY works here (so that you can save the 
> > remount below).
> > 
> > Also, I think you might want to do a recursive bind mount in case the 
> > layer itself has some mounts underneath it.
> > 
> > YOu can drop a TODO here.

MS_BIND | MS_RDONLY in one call doesn't work. Remount is necessary.

MS_REC: Not necessary for APPC but added the TODO. It's only necessary if 
mounts are used during image preparation for a single layer. Bind mount already 
limits the number of layers to be 1. Feels unlikely to me but a note here is 
definitely helpful.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 121
> > 
> >
> > The read-only bind mount introduces a problem that the filesystem 
> > isolator cannot create the mount point for the sandbox anymore if it does 
> > not exist.
> > 
> > Please add a NOTE states that all mount points needed must already be 
> > present in the rootfs.

Thanks for spotting this! This is quite a limitation for BindBackend's 
usability but luckily the user can work around this.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 143
> > 
> >
> > Please add a TODO here saying that if recursive bind mount is used 
> > above, here you need to check `strings::contains(entry.target, rootfs)`.

`strings::startsWith(entry.target, rootfs)`. Added a note.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 144
> > 
> >
> > Any reason use a detached UMOUNT here? The os::rmdir will fail if 
> > unmount hasn't finished yet.

Removed MNT_DETACH flag.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_backend_tests.cpp, line 45
> > 
> >
> > Could you please call it ProvisionerBindBackendTest.

Named it `BindBackendTest`, I imagine `OverlayfsBackend` can also use this test 
but oh well, let's refactor later.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_backend_tests.cpp, lines 74-76
> > 
> >
> > This is expensive. You don't need a working rootfs as far as I can 
> > tell, right?

Created a dummy fs.


- Jiang Yan


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


On Aug. 25, 2015, 3:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 3:59 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.hpp 
> 46120e8420cc491a0decbd88301f89d6dfcff120 
>   src/slave/containerizer/pro

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Jiang Yan Xu 

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

(Updated Aug. 25, 2015, 3:59 p.m.)


Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.


Changes
---

Comments.


Bugs: MESOS-3190
https://issues.apache.org/jira/browse/MESOS-3190


Repository: mesos


Description
---

Introduced bind-mount based provisioner Backend.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/provisioners/backend.hpp 
46120e8420cc491a0decbd88301f89d6dfcff120 
  src/slave/containerizer/provisioners/backend.cpp 
6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
  src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
  src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 

Diff: https://reviews.apache.org/r/37747/diff/


Testing
---

sudo make check. Added one test.


Thanks,

Jiang Yan Xu

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Jie Yu 

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



src/slave/containerizer/provisioners/backend.cpp (lines 34 - 36)


So bind backend only compiles on linux but backend.cpp compiles on osx as 
well. You need to use ifdef linux guard for "BindBackend::create".



src/slave/containerizer/provisioners/backends/bind.hpp (line 23)


Add a blank line above.



src/slave/containerizer/provisioners/backends/bind.cpp (line 26)


Add a blank line above.



src/slave/containerizer/provisioners/backends/bind.cpp (line 41)


No need for the process ID generation unless it's proven needed.



src/slave/containerizer/provisioners/backends/bind.cpp (line 45)


I would sugguest you save the slave flags at least.

```
public:
  BindBackendProcess(const Flags& _flags)
: flags(_flags) {}

private:
  const Flags flags;
```



src/slave/containerizer/provisioners/backends/bind.cpp (line 51)


Do you want to make sure the current user is root since 'mount' requires 
root permission.



src/slave/containerizer/provisioners/backends/bind.cpp (line 95)


No layer specified.



src/slave/containerizer/provisioners/backends/bind.cpp (line 107)


Can you try if MS_BIND | MS_RDONLY works here (so that you can save the 
remount below).

Also, I think you might want to do a recursive bind mount in case the layer 
itself has some mounts underneath it.

YOu can drop a TODO here.



src/slave/containerizer/provisioners/backends/bind.cpp (line 121)


The read-only bind mount introduces a problem that the filesystem isolator 
cannot create the mount point for the sandbox anymore if it does not exist.

Please add a NOTE states that all mount points needed must already be 
present in the rootfs.



src/slave/containerizer/provisioners/backends/bind.cpp (line 143)


Please add a TODO here saying that if recursive bind mount is used above, 
here you need to check `strings::contains(entry.target, rootfs)`.



src/slave/containerizer/provisioners/backends/bind.cpp (line 144)


Any reason use a detached UMOUNT here? The os::rmdir will fail if unmount 
hasn't finished yet.



src/tests/containerizer/provisioner_backend_tests.cpp (line 39)


Please move this right after using namespace process;



src/tests/containerizer/provisioner_backend_tests.cpp (line 45)


Could you please call it ProvisionerBindBackendTest.



src/tests/containerizer/provisioner_backend_tests.cpp (line 50)


Please wrap the entire class under ifdef linux guard



src/tests/containerizer/provisioner_backend_tests.cpp (line 60)


This is not needed.



src/tests/containerizer/provisioner_backend_tests.cpp (lines 74 - 76)


This is expensive. You don't need a working rootfs as far as I can tell, 
right?


- Jie Yu


On Aug. 25, 2015, 5:22 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37722, 37747]

All tests passed.

- Mesos ReviewBot


On Aug. 25, 2015, 5:22 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/37747/diff/
> 
> 
> Testing
> ---
> 
> sudo make check. Added one test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37747: Introduced bind-mount based provisioner Backend.

2015-08-25 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [37722, 37747]

All tests passed.

- Mesos ReviewBot


On Aug. 25, 2015, 5:22 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> ---
> 
> (Updated Aug. 25, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
> https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/37747/diff/
> 
> 
> Testing
> ---
> 
> sudo make check. Added one test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>