Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Jan. 11, 2016, 8:15 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 11, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Gilbert Song]

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

(Updated Jan. 13, 2016, 1:19 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Pulled out provisioner from linux filesystem isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
89d18624b08f2c953b9fa21663aeda694d4aac12 
  src/slave/containerizer/mesos/containerizer.cpp 
f3c370aeb331beb6202fd30cd0278877da0b42e0 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
a29f9d184a0d1088577b3168a12bc8c06493a477 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
5bb85034c22caef64054c1629f6fd55d227e48b1 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Jie Yu]

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

Ship it!


Thanks! THis is great!


src/slave/containerizer/mesos/containerizer.cpp (line 1365)


We are not destroying the 'provisioner'. So
```
Failed to destroy the provisioned filesystem when...
```


- Jie Yu


On Jan. 11, 2016, 8:15 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 11, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41814, 41999, 41815, 41816, 41817, 41819, 41820]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 13, 2016, 9:19 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 13, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41814, 41999, 41815, 41816, 41817, 41819]

Failed command: ./support/apply-review.sh -n -r 41819

Error:
 2016-01-12 03:21:08 URL:https://reviews.apache.org/r/41819/diff/raw/ 
[21646/21646] -> "41819.patch" [1]
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:702
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/isolators/filesystem/posix.cpp:91
error: src/slave/containerizer/mesos/isolators/filesystem/posix.cpp: patch does 
not apply

- Mesos ReviewBot


On Jan. 11, 2016, 8:15 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 11, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Gilbert Song]

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

(Updated Jan. 11, 2016, 12:15 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Pulled out provisioner from linux filesystem isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
89d18624b08f2c953b9fa21663aeda694d4aac12 
  src/slave/containerizer/mesos/containerizer.cpp 
f3c370aeb331beb6202fd30cd0278877da0b42e0 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
a29f9d184a0d1088577b3168a12bc8c06493a477 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
5bb85034c22caef64054c1629f6fd55d227e48b1 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41814, 41999, 41815, 41816, 41817, 41819]

Failed command: ./support/apply-review.sh -n -r 41819

Error:
 2016-01-11 01:39:47 URL:https://reviews.apache.org/r/41819/diff/raw/ 
[21646/21646] -> "41819.patch" [1]
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:702
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/isolators/filesystem/posix.cpp:91
error: src/slave/containerizer/mesos/isolators/filesystem/posix.cpp: patch does 
not apply

- Mesos ReviewBot


On Jan. 9, 2016, 2:14 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 9, 2016, 2:14 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Gilbert Song]

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

(Updated Jan. 8, 2016, 6:14 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Pulled out provisioner from linux filesystem isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
89d18624b08f2c953b9fa21663aeda694d4aac12 
  src/slave/containerizer/mesos/containerizer.cpp 
f3c370aeb331beb6202fd30cd0278877da0b42e0 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
a29f9d184a0d1088577b3168a12bc8c06493a477 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
5bb85034c22caef64054c1629f6fd55d227e48b1 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Jie Yu]

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



src/slave/containerizer/mesos/containerizer.cpp (lines 482 - 483)


We should recover the isolators first before recover provisioner (the 
reverse order of launching a container because recover might do cleanup on 
unknown containers).

YOu might want to add `recoverIsolators` and `recoverProvisioner`. Change 
the `_recover` function to be:
```
return recoverIsolators(recoverable, orphans)
  .then(defer(self(), ::recoverProvisioner, recoverable, orphans))
  .then(defer(self(), ::__recover, recoverable, orphans));
```



src/slave/containerizer/mesos/containerizer.cpp (lines 689 - 690)


We mutate the executorInfo here and that mutated executorInfo here needs to 
be passed to `prepare` and `_launch`. This is important for command executors.

I guess what you needs to do here is: rename the existing `_launch` to 
`__launch` and add a new `_launch`. In `launch`, do the following:

```
return provisioner->provision(containerId, executorInfo)
  .then(defer(self(),
  ::_launch,
  containerId,
  executorInfo,
  directory,
  user,
  slaveId,
  slavePid,
  checkpoint,
  lambda::_1));
```

The new `_launch` should be similar to what you have here for `_provision`, 
but in the end, if will call `__launch` with mutated executorInfo.



src/slave/containerizer/mesos/containerizer.cpp (line 711)


I realized another issue with command executors. We don't have to resolve 
it in the patch, but I just want to mention it here so that we don't forget. 
You probably can add a TODO comments here.

For command executors, we modify the executorInfo so that the user image 
will be mounted in as a volume. However, we also need to figure out a way to 
support passing and handling those runtime configurations in the image.

cc @tnachen



src/slave/containerizer/mesos/containerizer.cpp (lines 1447 - 1451)


Again, we should call provisioner->destroy after all isolators have been 
cleaned up. Also, it does not make sense to put provisioner->destroy in 
cleanupIsolators.



src/slave/containerizer/mesos/isolators/filesystem/linux.hpp (lines 45 - 46)


This fits in one line?



src/slave/containerizer/mesos/isolators/filesystem/linux.hpp (lines 77 - 78)


Ditto.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 62 - 63)


Ditto.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 173 - 174)


Fits in one line?



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 157 - 158)


Fits in one line?



src/tests/containerizer/filesystem_isolator_tests.cpp (lines 1008 - 1009)


FIts in one line?


- Jie Yu


On Jan. 6, 2016, 11:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 6, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Gilbert Song]

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

(Updated Jan. 6, 2016, 3:41 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Pulled out provisioner from linux filesystem isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
89d18624b08f2c953b9fa21663aeda694d4aac12 
  src/slave/containerizer/mesos/containerizer.cpp 
f3c370aeb331beb6202fd30cd0278877da0b42e0 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
a29f9d184a0d1088577b3168a12bc8c06493a477 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
5bb85034c22caef64054c1629f6fd55d227e48b1 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 41820: Pulled out provisioner from linux filesystem isolator.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [41814, 41999, 41815, 41816, 41817]

Failed command: ./support/apply-review.sh -n -r 41817

Error:
 2016-01-07 07:13:03 URL:https://reviews.apache.org/r/41817/diff/raw/ 
[11190/11190] -> "41817.patch" [1]
Total errors found: 0
Checking 6 files
Error: Commit message summary (the first line) must not exceed 72 characters.

- Mesos ReviewBot


On Jan. 6, 2016, 11:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41820/
> ---
> 
> (Updated Jan. 6, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4240
> https://issues.apache.org/jira/browse/MESOS-4240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out provisioner from linux filesystem isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 89d18624b08f2c953b9fa21663aeda694d4aac12 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3c370aeb331beb6202fd30cd0278877da0b42e0 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> a29f9d184a0d1088577b3168a12bc8c06493a477 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8af2cecdca947e55d8d39e26d2fc3d42212f36c3 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> 5bb85034c22caef64054c1629f6fd55d227e48b1 
> 
> Diff: https://reviews.apache.org/r/41820/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>