Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[haosdent huang]

> On Feb. 2, 2016, 2:45 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.
> 
> Bernd Mathiske wrote:
> I'd rather not ignore the error message. How can there be an out-of-range 
> char in HTML and CSS files?
> 
> haosdent huang wrote:
> I afraid this may caused by boostrap have some special characters like 
> `ok:before{content:"\e013"}`.
> 
> Bernd Mathiske wrote:
> This should be an encoding using printable cahrs (<=128) only.
> 
> haosdent huang wrote:
> LoL, seems `Michael Lunøe` cause the problem. Need fix the 
> apply_reviews.py

Need do changes in apply_reviews.py to support no-ascii commit messages.
```
diff --git a/support/apply-reviews.py b/support/apply-reviews.py
index ea5e43a..36bfb1e 100755
--- a/support/apply-reviews.py
+++ b/support/apply-reviews.py
@@ -185,7 +185,7 @@ def commit_patch():
   else:
 amend = '-e'

-  cmd = 'git commit --author \'{author}\' {_amend} -am \'{message}\''\
+  cmd = u'git commit --author \'{author}\' {_amend} -am \'{message}\''\
 .format(author=quote(data['author']),
 _amend=amend,
 message=quote(data['message']))
@@ -248,7 +248,7 @@ def reviewboard_data():
   user = url_to_json(reviewboard_user_url(
 review.get('links').get('submitter').get('title'))).get('user')

-  author = '{author} <{email}>'.format(author=user.get('fullname'),
+  author = u'{author} <{email}>'.format(author=user.get('fullname'),
email=user.get('email'))
   message = '\n\n'.join(['{summary}',
   '{description}',
```


- haosdent


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


On Feb. 1, 2016, 11:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 11:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[haosdent huang]

> On Feb. 2, 2016, 2:45 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.
> 
> Bernd Mathiske wrote:
> I'd rather not ignore the error message. How can there be an out-of-range 
> char in HTML and CSS files?
> 
> haosdent huang wrote:
> I afraid this may caused by boostrap have some special characters like 
> `ok:before{content:"\e013"}`.
> 
> Bernd Mathiske wrote:
> This should be an encoding using printable cahrs (<=128) only.

LoL, seems `Michael Lunøe` cause the problem. Need fix the apply_reviews.py


- haosdent


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


On Feb. 1, 2016, 11:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 11:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 42587: Implemented the `NetClsHandleManager` class.

[Anand Mazumdar]

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



Primarily fly-by style comments.


src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 58)


We generally leave a line before the comment and the TODO to correctly 
demarcate where the comment ends and where the TODO begins.

```
// comments
//
// TODO(asridharan): 
//
```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 91)


What does `handles.size()` returns. I am assuming its an `unsigned in`t and 
hence should fail compilation with `-Wsign-compare` on GCC.

Why don't we just do `size_t count = 1`?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 95)


Should fit on one line?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 96)


I see that we name `NetClsHandles` as `handles`.

Why not name `NetClsHandle` as just `handle`?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 120)


Remove period at the end.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 131)


How about simplifying this to:

```
if (condition) {
  return Nothing();
}

return Error(...);
```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 154)


Remove period at the end.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 165)


How about simplifying this to:

```
if (condition) {
  return Nothing();
}

return Error(...);
```


- Anand Mazumdar


On Feb. 3, 2016, 6:46 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> ---
> 
> (Updated Feb. 3, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the `NetClsHandleManager` class.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42587/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42781: Added the --cgroups_net_cls_primary_handle flag to the slave.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 7:47 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added the --cgroups_net_cls_primary_handle flag to the slave.


Diffs (updated)
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

Tested the flag by launching a mesos agent and checking that the slave takes 
the `--cgroups_net_cls_major_handles` flag.


Thanks,

Avinash sridharan

Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 7:46 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 42780: Changed the NetClsIsolatorTest to check for net_cls handles.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 7:47 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Changed the NetClsIsolatorTest to check for net_cls handles.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Anand Mazumdar]

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



Primarily trivial comments around style/error messages.


src/linux/cgroups.hpp (line 656)


Should fit in one line?



src/linux/cgroups.cpp (line 2463)


Ditto.



src/linux/cgroups.cpp (line 2471)


Space after `net_cls.classid` and a colon too



src/linux/cgroups.cpp (line 2476)


Wondering if we should return more info to the caller here?



src/linux/cgroups.cpp (line 2492)


Space after colon



src/linux/cgroups.cpp (line 2495)


Period at the end.


- Anand Mazumdar


On Feb. 3, 2016, 7:17 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43096/
> ---
> 
> (Updated Feb. 3, 2016, 7:17 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function in cgroup for supporting net_cls subsystem.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
>   src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 
> 
> Diff: https://reviews.apache.org/r/43096/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[Bernd Mathiske]

> On Feb. 1, 2016, 6:45 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.
> 
> Bernd Mathiske wrote:
> I'd rather not ignore the error message. How can there be an out-of-range 
> char in HTML and CSS files?
> 
> haosdent huang wrote:
> I afraid this may caused by boostrap have some special characters like 
> `ok:before{content:"\e013"}`.

This should be an encoding using printable cahrs (<=128) only.


- Bernd


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


On Feb. 1, 2016, 3:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[haosdent huang]

> On Feb. 2, 2016, 2:45 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.
> 
> Bernd Mathiske wrote:
> I'd rather not ignore the error message. How can there be an out-of-range 
> char in HTML and CSS files?

I afraid this may caused by boostrap have some special characters like 
`ok:before{content:"\e013"}`.


- haosdent


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


On Feb. 1, 2016, 11:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 11:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 43072: Use full screen width for Mesos UI using Bootstraps (v3.3.6) container-fluid

[Bernd Mathiske]

> On Feb. 1, 2016, 6:45 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [43072]
> > 
> > Failed command: ./support/apply-review.sh -n -r 43072
> > 
> > Error:
> > 2016-02-02 02:45:28 URL:https://reviews.apache.org/r/43072/diff/raw/ 
> > [226634/226634] -> "43072.patch" [1]
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 342, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 321, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 139, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 180, in commit_patch
> > data = patch_data()
> >   File "support/apply-reviews.py", line 199, in patch_data
> > return reviewboard_data()
> >   File "support/apply-reviews.py", line 252, in reviewboard_data
> > email=user.get('email'))
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf8' in 
> > position 11: ordinal not in range(128)
> > 
> > Full log: https://builds.apache.org/job/mesos-reviewbot/11187/console
> 
> haosdent huang wrote:
> I think we could ignore this error message.

I'd rather not ignore the error message. How can there be an out-of-range char 
in HTML and CSS files?


- Bernd


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


On Feb. 1, 2016, 3:36 p.m., Michael Lunøe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43072/
> ---
> 
> (Updated Feb. 1, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-2585
> https://issues.apache.org/jira/browse/MESOS-2585
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The solution to our problem is solved by updating the Bootstrap CSS file, as 
> container-fluid was introduced later in Bootstrap.
> 
> I have updated to lastest stable release of Bootstrap v3.3.6. Looking through 
> the change log, the only thing that we are using in the Mesos UI and changed 
> is the "hide" class (deprecated), so instances of these have been replaced by 
> its successor ".hidden" class.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 6b18056 
>   src/webui/master/static/css/bootstrap-3.0.3.min.css c547283 
>   src/webui/master/static/css/bootstrap-3.3.6.min.css PRE-CREATION 
>   src/webui/master/static/framework.html 9b28820 
>   src/webui/master/static/home.html d6cde1e 
>   src/webui/master/static/index.html 25caf53 
>   src/webui/master/static/slave.html bc46885 
>   src/webui/master/static/slave_executor.html 9d582d5 
>   src/webui/master/static/slave_framework.html 96d788f 
> 
> Diff: https://reviews.apache.org/r/43072/diff/
> 
> 
> Testing
> ---
> 
> - Mobile: http://cl.ly/2R2O0m1a1G3Q/Image%202016-02-01%20at%2015.21.53.png
> - 1680 x 1050px screen: 
> http://cl.ly/0K1o112u3L2I/Image%202016-02-01%20at%2015.23.37.png
> - 2880 x 1800px screen: 
> http://cl.ly/1M0L3M2X0J2i/Image%202016-02-01%20at%2015.22.50.png
> 
> Visual test in latest Chrome, Firefox, Safari and IE. No functional changes 
> introduced.
> 
> 
> Thanks,
> 
> Michael Lunøe
> 
>

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Avinash sridharan]

> On Feb. 3, 2016, 1:32 a.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, line 2489
> > 
> >
> > Where is the stringify function?
> > 
> > Have you verified that if we can just write a decimal number to 
> > net_cls.classid, or it has to be a hex number?

Hi Jie,
 Tried it out. We can write a decimal number direclty. The stringify is the 
stout::stringify function. It was being used here as well:
 
https://github.com/apache/mesos/blob/765c025dd43e04360b29c19bd9a66837954c5a20/src/linux/cgroups.cpp#L960
 
 
 I couldn't find an overloaded cgorups::write that took the value as an int, 
and hence had to stringify before using cgroups::write


- Avinash


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


On Feb. 3, 2016, 7:17 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43096/
> ---
> 
> (Updated Feb. 3, 2016, 7:17 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function in cgroup for supporting net_cls subsystem.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
>   src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 
> 
> Diff: https://reviews.apache.org/r/43096/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 7:17 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function in cgroup for supporting net_cls subsystem.


Diffs (updated)
-

  src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
  src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42186: Added tests for recovery for HTTP based executors.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42181, 43131, 42844, 42185, 42186]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 3, 2016, 2:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42186/
> ---
> 
> (Updated Feb. 3, 2016, 2:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6683a081ac231f1e275a3cdb8ee841da430a9f66 
> 
> Diff: https://reviews.apache.org/r/42186/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 7:11 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Added unit-test for `NetClsHandleManager`.


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


Repository: mesos


Description (updated)
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Review Request 43134: Added output operator for unversioned executor protobuf.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This change adds the missing output operator for the unversioned executor 
protobuf. These are not used in the source code currently and hence were 
missing. The executor library uses the `v1` protobufs and the output operator 
for them was already added.


Diffs
-

  include/mesos/executor/executor.hpp a7d40f6acaf24e086fb46ecfb74e97612ba326d4 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42587: Implemented the `NetClsHandleManager` class.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 6:46 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Implemented the `NetClsHandleManager` class.


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


Repository: mesos


Description (updated)
---

Implemented the `NetClsHandleManager` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42586: Defined the NetClsHandleManager class.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 6:44 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Defined the NetClsHandleManager class.


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


Repository: mesos


Description
---

This class will be responsible for keeping track of the free and allocated
net_cls handles.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42586: Defined the NetClsHandleManager class.

[Avinash sridharan]

> On Feb. 1, 2016, 5:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 51-52
> > 
> >
> > `s/majHandle/_major/`
> > `s/minHandle/_minor/`
> > 
> > Also, remove the space right after `NetClsHandle`

Using primary and secondary instead of major and minor.


> On Feb. 1, 2016, 5:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 65-75
> > 
> >
> > Why do we need this?
> 
> Avinash sridharan wrote:
> Was using this data structure to pass on the usage of handles for a given 
> major handle. It was useful in unit-testing, but thought will be useful in 
> implementing a usage interface to the NetClsHandleMgr class. Should I keep it?

Removed this structure. We no longer need it.


> On Feb. 1, 2016, 5:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 78-85
> > 
> >
> > I would suggest that we don't define a new data structure for this. Can 
> > you just inline it in NetClsHandleManager?

Simplified the data structure. We no longer need an explicit NetClsHandles 
structure.


> On Feb. 1, 2016, 5:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 108-113
> > 
> >
> > We do you need this in this patch?
> 
> Avinash sridharan wrote:
> There are quite a few places where we are throwing erros and need to 
> convert the major and minor handle into a hex string, hence thought implement 
> this as a static function (maybe inline) . Should I keep this?

Removed this structure from this patch. Will re-introduce it when we introduce 
the implementation.


> On Feb. 1, 2016, 5:49 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, line 129
> > 
> >
> > What's the key for this map?

Added a comment to elaborate on the key.


- Avinash


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


On Feb. 3, 2016, 6:44 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42586/
> ---
> 
> (Updated Feb. 3, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will be responsible for keeping track of the free and allocated
> net_cls handles.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42586/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43118: Corrected mistakes in docs for volume/reservation HTTP endpoints.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43118]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 3, 2016, 12:39 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43118/
> ---
> 
> (Updated Feb. 3, 2016, 12:39 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Bugs: MESOS-4421
> https://issues.apache.org/jira/browse/MESOS-4421
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected mistakes in docs for volume/reservation HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 9657c3f75dcc17dfd4d6391b35da3dbc0a6c547d 
>   docs/reservation.md 25337109ff19240f926667961a59323bbfeb9956 
> 
> Diff: https://reviews.apache.org/r/43118/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43124: Clarified units (megabytes) for "disk" and "mem" resource types.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43124]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 3, 2016, 12:25 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43124/
> ---
> 
> (Updated Feb. 3, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4377
> https://issues.apache.org/jira/browse/MESOS-4377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that I also removed an old TODO from `mesos.proto`: it has been partially
> implemented, and in general the need for more first-class resource values is
> well-known (e.g., many JIRAs on this topic).
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 53a27f48c94baa9b25a072e0fc503bde0b240ba2 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed 
> 
> Diff: https://reviews.apache.org/r/43124/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43122: Updated Rakefile to automatically generate doxygen pages.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43120, 43121, 43122]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 3, 2016, 12:15 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43122/
> ---
> 
> (Updated Feb. 3, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Neil Conway.
> 
> 
> Bugs: MESOS-4584
> https://issues.apache.org/jira/browse/MESOS-4584
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   site/Rakefile 0ce4b7975f95ab6930f0b2674191930df9ab5b20 
> 
> Diff: https://reviews.apache.org/r/43122/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 42186: Added tests for recovery for HTTP based executors.

[Anand Mazumdar]

> On Feb. 2, 2016, 11:02 p.m., Vinod Kone wrote:
> > src/tests/slave_recovery_tests.cpp, line 1046
> > 
> >
> > I think you should start the slave with fixed id like in the above 
> > test, to ensure the executor tries to connect to the right endpoint.

Fixed. I had not done so earlier since for recovery mode `cleanup`, the 
executor would be killed anyways.


- Anand


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


On Feb. 3, 2016, 2:34 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42186/
> ---
> 
> (Updated Feb. 3, 2016, 2:34 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6683a081ac231f1e275a3cdb8ee841da430a9f66 
> 
> Diff: https://reviews.apache.org/r/42186/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42185: Added an example executor based on the new V1 API.

[Anand Mazumdar]

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

(Updated Feb. 3, 2016, 2:34 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

This change adds a custom executor based on the new executor library.


Diffs (updated)
-

  src/Makefile.am fac17f4bac3b2ddda0384dec8b0ed6f960bd24d1 
  src/examples/test_http_executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43131: Modified existing usage of Slave constructor.

[Anand Mazumdar]

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

(Updated Feb. 3, 2016, 2:34 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Modified description


Summary (updated)
-

Modified existing usage of Slave constructor.


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


Repository: mesos


Description
---

This change adds the argument process ID wherever the `Slave` object is 
constructed.


Diffs
-

  src/local/local.cpp 582d4a10444831b0753d20650698e5d3b51cca9f 
  src/slave/main.cpp a412cebcaf533feaff54e8ed398ae7007263e51f 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42186: Added tests for recovery for HTTP based executors.

[Anand Mazumdar]

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

(Updated Feb. 3, 2016, 2:34 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 6683a081ac231f1e275a3cdb8ee841da430a9f66 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 43131: Modified existing usage of Slave constructors.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change adds the argument process ID wherever the `Slave` object is 
constructed.


Diffs
-

  src/local/local.cpp 582d4a10444831b0753d20650698e5d3b51cca9f 
  src/slave/main.cpp a412cebcaf533feaff54e8ed398ae7007263e51f 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42844: Drop `404 NotFound` responses in the executor library.

[Anand Mazumdar]

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

(Updated Feb. 3, 2016, 2:32 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Modifed deps


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


Repository: mesos


Description
---

Previously, we did not use to drop `404 NotFound` responses in the library and 
send `Event::Error` to executor. However, this can be trigerred upon an agent 
restart when it has not yet set up HTTP routes.


Diffs
-

  src/executor/executor.cpp 92334ffb8af83b1c86c44bd406147893c7c6daa3 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 42181: Added a process ID argument to Slave constructor.

[Anand Mazumdar]

> On Feb. 2, 2016, 10:05 p.m., Vinod Kone wrote:
> > src/tests/cluster.cpp, lines 409-430
> > 
> >
> > instead of doing if else blocks can you use a ternary operator for the 
> > 1st argument in the constructor?
> > 
> > ```
> > slave.slave = new Slave::Slave(
> >   id.isSome() ? id.get() : process::ID::generate("slave"),
> >   ...
> >   ...);
> > ```
> > 
> > infact instead of declaring an overloaded constructor in slave.hpp and 
> > chaining constructors, can you make 'id' an argument with default value?
> > 
> > ```
> > Slave(...,
> >   const std::string& id = process::ID::generate("slave"))
> > ```
> > 
> > Thoughts?

As per our discussion, modified the existing `Slave` constructor to take an 
"id" as argument. Also, cleaned up other places in code to use this new 
constructor.


- Anand


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


On Feb. 3, 2016, 2:31 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42181/
> ---
> 
> (Updated Feb. 3, 2016, 2:31 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the existing `Slave` constructor to take in the process 
> ID string as an argument. This is necessary for testing recovery of HTTP 
> based executors. (Previously, every invocation of `StartSlave` used to 
> generate a new ID via `ID::generate`). There was no process delegate set via 
> `process::initialize` that led to problems for the existing HTTP based 
> executor to connect back to the slave.
> 
> Also, modified the tests to introduce a new `StartSlave` function that takes 
> this process ID as argument.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
>   src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
>   src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
>   src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
>   src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488 
>   src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 
> 
> Diff: https://reviews.apache.org/r/42181/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42181: Added a process ID argument to Slave constructor.

[Anand Mazumdar]

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

(Updated Feb. 3, 2016, 2:31 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


Summary (updated)
-

Added a process ID argument to Slave constructor.


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


Repository: mesos


Description (updated)
---

This change modifies the existing `Slave` constructor to take in the process ID 
string as an argument. This is necessary for testing recovery of HTTP based 
executors. (Previously, every invocation of `StartSlave` used to generate a new 
ID via `ID::generate`). There was no process delegate set via 
`process::initialize` that led to problems for the existing HTTP based executor 
to connect back to the slave.

Also, modified the tests to introduce a new `StartSlave` function that takes 
this process ID as argument.


Diffs (updated)
-

  src/slave/slave.hpp 632640293f14fb97aea6d7dd2c462c1f604638fb 
  src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
  src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
  src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
  src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43105: Ensured the allocator does not double account resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42633, 42636, 42657, 42658, 42672, 41950, 42911, 43105]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 2, 2016, 11:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43105/
> ---
> 
> (Updated Feb. 2, 2016, 11:13 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If `addFramework()` is called before `addSlave()` double accounting
> of resources is possible. We have not observed bugs because Mesos
> currently always adds a framework in the allocator after all agents
> with the framework's tasks have been added.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp e163669c9c4e4c98572968f18987704b60722a79 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1a07d69016407e5aad2209586da37fecbcddb765 
> 
> Diff: https://reviews.apache.org/r/43105/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Jie Yu]

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




src/linux/cgroups.hpp (line 655)


We don't add extra indentation here. Please take a look at the code around 
you:)



src/linux/cgroups.hpp (line 658)


2 lines apart.



src/linux/cgroups.cpp (line 2459)


Add one extra line here.



src/linux/cgroups.cpp (lines 2466 - 2481)


indentation should be 2 here.



src/linux/cgroups.cpp (line 2467)


s/handle/read/



src/linux/cgroups.cpp (lines 2469 - 2471)


```
return Error("Unable to read 'net_cls.classid':" + read.error());
```

The caller is responsible for printing the cgroup/hierarchy.



src/linux/cgroups.cpp (line 2474)


s/iHandle/handle/



src/linux/cgroups.cpp (lines 2476 - 2478)


```
return Error("Not a valid number");
```



src/linux/cgroups.cpp (lines 2483 - 2484)


one extra line here.



src/linux/cgroups.cpp (line 2489)


Where is the stringify function?

Have you verified that if we can just write a decimal number to 
net_cls.classid, or it has to be a hex number?



src/linux/cgroups.cpp (lines 2501 - 2503)


```
return Error("Failed to write 'net_cls.classid': " + write.error());
```


- Jie Yu


On Feb. 3, 2016, 12:47 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43096/
> ---
> 
> (Updated Feb. 3, 2016, 12:47 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function in cgroup for supporting net_cls subsystem.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
>   src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 
> 
> Diff: https://reviews.apache.org/r/43096/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42841: WIP: Introducing appc image fetcher.

[Jojy Varghese]

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

(Updated Feb. 3, 2016, 1:31 a.m.)


Review request for Jie Yu.


Repository: mesos


Description
---

WIP: Introducing appc image fetcher.


Diffs (updated)
-

  src/CMakeLists.txt 6b2c7cbffa57d44b913419ad68fc66eaeca169d0 
  src/Makefile.am fac17f4bac3b2ddda0384dec8b0ed6f960bd24d1 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp PRE-CREATION 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

make check; local image server


Thanks,

Jojy Varghese

Review Request 43127: Introduced Appc image cache.

[Jojy Varghese]

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

Review request for mesos and Jie Yu.


Bugs: MESOS-4439 and MESOS-4575
https://issues.apache.org/jira/browse/MESOS-4439
https://issues.apache.org/jira/browse/MESOS-4575


Repository: mesos


Description
---

Image cache will cache metadata about all Appc images stores in the store's
image directory. This cache could be useful to:
  - Quickly query if an image is present in the store.
  - By sharing the cache with other components like fetcher, an image can be
  checked if its present before fetching it.


Diffs
-

  src/slave/containerizer/mesos/provisioner/appc/cache.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/cache.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/appc/store.cpp 
73c4df858a70da3d4cc4a1cb15092165f6ff8fe4 
  src/tests/containerizer/provisioner_appc_tests.cpp 
e3d08d9e49df93d5290099c8bfd917f60c93e51b 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 42586: Defined the NetClsHandleMgr class.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 74)


s/NetClsHandleMgr/NetClsHandleManager/



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 83)


s/allocMajor/allocPrimary/



src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (line 91)


Two things:
1) Can you `s/_netClsHandles/used/`?
2) Can you just use bitset<0x1> here? No need for the constant.


- Jie Yu


On Feb. 3, 2016, 12:43 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42586/
> ---
> 
> (Updated Feb. 3, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will be responsible for keeping track of the free and allocated
> net_cls handles.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42586/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 43120: Removed site/Gemfile.lock.

[Kapil Arya]

> On Feb. 2, 2016, 7:27 p.m., Kevin Klues wrote:
> > You can't just remove this file.  It specifies the versions of everything 
> > that should be installed in a fresh docker image upon calling 'bundle 
> > insgtall'.  If you saw that it was being overwritten, it means you had a 
> > (possibly incompatible) newer version of some tools already installed on 
> > your system, which was causing newer versions of things to be pulled in.  
> > We need this committed so that we have proper versioning for known good 
> > versions of the tools installed on a fresh install.

The problem was that it wasn't getting overwritten :-). I had newer versions of 
tools; more specifically, eventmachine version 1.0.9 was installed. However, 
this file expected version 1.0.3. So either I had to update or remove this file 
by hand before running rake. I wonder if anyone else has run into such problems 
and if there are better alternatives.

For me, I ended up not using the docker image and just used the stuff from 
mesos source tree to generate the publish folder,  but I understand that this 
might fail for others. In that case, we can just drop this RR and continue with 
the other RRs :-).


- Kapil


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


On Feb. 2, 2016, 7:15 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43120/
> ---
> 
> (Updated Feb. 2, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Neil Conway.
> 
> 
> Bugs: MESOS-4584
> https://issues.apache.org/jira/browse/MESOS-4584
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This file is automatically generated by `bundle install` and can cause
> conflicts if pre-generated.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock 065107ec25f0eff28e2a09051b61152d49f77f9a 
> 
> Diff: https://reviews.apache.org/r/43120/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 42782: Added a unit-test to test net_cls major handles set from command line.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:54 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a unit-test to test net_cls major handles set from command line.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 43107: Passed agent flag --cgroup_net_cls_primary_handle to net_cls isolator.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:52 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

The `cgroup/net_cls` isolator will use the primary handle, passed by the 
operator
using the --cgroup_net_cls_primary_handle, to initialize the `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check

Launched a slave by passing invalid flags to --cgroup_net_cls_primary_handle 
and made sure the slave correctly detects these invalid handles. 

Launched a slave with valid handle to make sure that the slave launches 
correctly.


Thanks,

Avinash sridharan

Re: Review Request 42781: Added the --cgroups_net_cls_primary_handle flag to the slave.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:51 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added the --cgroups_net_cls_primary_handle flag to the slave.


Diffs (updated)
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 24a23325cc255d0d7b7af7ed096b6d3012ad75c7 

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


Testing
---

Tested the flag by launching a mesos agent and checking that the slave takes 
the `--cgroups_net_cls_major_handles` flag.


Thanks,

Avinash sridharan

Re: Review Request 42780: Changed the NetClsIsolatorTest to check for net_cls handles.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:49 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Changed the NetClsIsolatorTest to check for net_cls handles.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:48 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:47 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function in cgroup for supporting net_cls subsystem.


Diffs (updated)
-

  src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
  src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 43019: Fixed non-camel case in protobuf field.

[Jie Yu]

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




include/mesos/slave/isolator.proto (lines 71 - 72)


Can we go through a deprecation cycle to do the renaming. In other words, 
can we keep the existing fields and add two new fields. We set both in 0.28 so 
that we don't break the build for existing isolators.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43019/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed non-camel case in protobuf field.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.proto 8b7c54afc4467084b5d22387e55689d06078b504 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 57168c837aabf85c85866ca9d4b5480f281e3143 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
> 34aa660204911ce4f22c7735af36c9f1af8b12b9 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 24ff8194bb5ea86a31224d2d2c20d367472168aa 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> bce5ab9a1913ee7cebaf55b5ee0b42fa4a325e87 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> 59ffd13920fb1c4fb8dbf04a2823d9cee75a8f27 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> d5dd93d892e3cde2011e208a050eec2b31891f54 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 182fe9217a5da9af603d6f9c203a1689eff4ca1b 
> 
> Diff: https://reviews.apache.org/r/43019/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:45 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42586: Defined the NetClsHandleMgr class.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:43 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This class will be responsible for keeping track of the free and allocated
net_cls handles.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

[Avinash sridharan]

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

(Updated Feb. 3, 2016, 12:44 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 43118: Corrected mistakes in docs for volume/reservation HTTP endpoints.

[Neil Conway]

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

(Updated Feb. 3, 2016, 12:39 a.m.)


Review request for mesos, Greg Mann and Jie Yu.


Changes
---

Fix per Greg's review.


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


Repository: mesos


Description
---

Corrected mistakes in docs for volume/reservation HTTP endpoints.


Diffs (updated)
-

  docs/persistent-volume.md 9657c3f75dcc17dfd4d6391b35da3dbc0a6c547d 
  docs/reservation.md 25337109ff19240f926667961a59323bbfeb9956 

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


Testing
---

Previewed in site-docker.


Thanks,

Neil Conway

Re: Review Request 43118: Corrected mistakes in docs for volume/reservation HTTP endpoints.

[Greg Mann]

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


Fix it, then Ship it!





docs/persistent-volume.md (line 356)


s/created/destroyed/

s/deletion/destroy/, in keeping with the pattern above & below?


- Greg Mann


On Feb. 2, 2016, 10:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43118/
> ---
> 
> (Updated Feb. 2, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Bugs: MESOS-4421
> https://issues.apache.org/jira/browse/MESOS-4421
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected mistakes in docs for volume/reservation HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 9657c3f75dcc17dfd4d6391b35da3dbc0a6c547d 
>   docs/reservation.md 25337109ff19240f926667961a59323bbfeb9956 
> 
> Diff: https://reviews.apache.org/r/43118/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43120: Removed site/Gemfile.lock.

[Kevin Klues]

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



You can't just remove this file.  It specifies the versions of everything that 
should be installed in a fresh docker image upon calling 'bundle insgtall'.  If 
you saw that it was being overwritten, it means you had a (possibly 
incompatible) newer version of some tools already installed on your system, 
which was causing newer versions of things to be pulled in.  We need this 
committed so that we have proper versioning for known good versions of the 
tools installed on a fresh install.

- Kevin Klues


On Feb. 3, 2016, 12:15 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43120/
> ---
> 
> (Updated Feb. 3, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Neil Conway.
> 
> 
> Bugs: MESOS-4584
> https://issues.apache.org/jira/browse/MESOS-4584
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This file is automatically generated by `bundle install` and can cause
> conflicts if pre-generated.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock 065107ec25f0eff28e2a09051b61152d49f77f9a 
> 
> Diff: https://reviews.apache.org/r/43120/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Review Request 43124: Clarified units (megabytes) for "disk" and "mem" resource types.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Note that I also removed an old TODO from `mesos.proto`: it has been partially
implemented, and in general the need for more first-class resource values is
well-known (e.g., many JIRAs on this topic).


Diffs
-

  docs/attributes-resources.md 53a27f48c94baa9b25a072e0fc503bde0b240ba2 
  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 43083: Supported working dir in docker runtime isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 1029 - 1034)


We shouldn't allow executor to cd into an arbitrary directory if filesystem 
isolation is not used (because that'll create security issue).

I would do the following:
```
if (rootfs.isSome()) {
  launchFlags.directory = workingDir.isSome()
? workingDir.get()
: flags.sandbox_directory;
} else {
  // NOTE: If the executor shares the host filesystem, we
  // should not allow them to 'cd' into an arbitrary directory
  // because that'll create security issues.
  if (workingDir.isSome()) {
LOG(WARNING) << "Ignore working directory '" << workingDir.get()
 << "' specified in container launch info for container "
 << containerId << " since the executor is using the "
 << "host filesystem";
  }
  launchFlags.directory = directory;
}
```



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 112 - 113)


I think we should distinguish between sandbox_direcotry and 
working_directory. For instance, for the marathon docker image, 
sandbox_directory is still /mnt/mesos/sandbox while  working_directory is 
/marathon.

So please add another flag to command executor.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 114 - 121)


if (!containerConfig.has_task_info()) {
  // Custom executor case.
  Option dir = getWorkingDirectory(containerConfig);
  if (dir.isSome()) {
launchInfo.set_working_directory(dir.get());
  }
}



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 357 - 359)


I would make this function very simple: get the working directory specified 
in the docker image.

I'll move the logics of setting --working_directory to 
getExecutorLaunchCommand.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 361 - 363)


Ditto on container_config?


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported working dir in docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43083/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43022: Added protobuf fields for docker runtime isolator.

[Jie Yu]

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




include/mesos/slave/isolator.proto (line 115)


Let's use working_directory here.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43022/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4517
> https://issues.apache.org/jira/browse/MESOS-4517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf fields for docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.proto 8b7c54afc4467084b5d22387e55689d06078b504 
> 
> Diff: https://reviews.apache.org/r/43022/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 43120: Removed site/Gemfile.lock.

[Kapil Arya]

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

Review request for mesos, Joseph Wu and Neil Conway.


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


Repository: mesos


Description
---

This file is automatically generated by `bundle install` and can cause
conflicts if pre-generated.


Diffs
-

  site/Gemfile.lock 065107ec25f0eff28e2a09051b61152d49f77f9a 

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


Testing
---


Thanks,

Kapil Arya

Review Request 43122: Updated Rakefile to automatically generate doxygen pages.

[Kapil Arya]

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

Review request for mesos, Joseph Wu and Neil Conway.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  site/Rakefile 0ce4b7975f95ab6930f0b2674191930df9ab5b20 

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


Testing
---


Thanks,

Kapil Arya

Review Request 43121: Updated Doxygen main page to use relative links.

[Kapil Arya]

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

Review request for mesos, Joseph Wu and Neil Conway.


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


Repository: mesos


Description
---

Previously, it was using absolute links which made it harder to test
links locally.


Diffs
-

  src/main.dox d5b29cf96b18ba84ac39f1747c4a174a5bfbc2e1 

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


Testing
---


Thanks,

Kapil Arya

Re: Review Request 43118: Corrected mistakes in docs for volume/reservation HTTP endpoints.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43118]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 2, 2016, 10:44 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43118/
> ---
> 
> (Updated Feb. 2, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Bugs: MESOS-4421
> https://issues.apache.org/jira/browse/MESOS-4421
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected mistakes in docs for volume/reservation HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 9657c3f75dcc17dfd4d6391b35da3dbc0a6c547d 
>   docs/reservation.md 25337109ff19240f926667961a59323bbfeb9956 
> 
> Diff: https://reviews.apache.org/r/43118/diff/
> 
> 
> Testing
> ---
> 
> Previewed in site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 42983: Implemented the `status` method in `MesosContainerizer`.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `status` method in `MesosContainerizer`.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
811ab7937279c4a55da450c136f9fcb1303ea0d5 
  src/slave/containerizer/mesos/containerizer.cpp 
4b504dbb58823ce7675f1d2048dcc7a27c05663d 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 43082: Added new flag to command executor for command passing.

[Jie Yu]

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




src/launcher/executor.cpp (line 143)


Why 'executorLaunchCommand'? This is the task command, right?

I would simply use 'command' for this naming here.



src/launcher/executor.cpp (lines 143 - 164)


I would restructure the code here for better readability:
```
// Determine the command to launch the task.
CommandInfo command;

if (taskCommand.isSome()) {
  ...
  command = parse.get();
} else if (task.has_command()) {
  command = task.command();
} else {
  CHECK_SOME(override)
<< "Expecting task " << task.task_id()
<< " to have a command!";
}

if (override.isNone()) {
  // TODO(jieyu): For now, ...
  if (command.shell()) {
CHECK(command.has_value()) << "...";
  } else {
CHECK(command.has_value()) << "...";
  }
}
```



src/launcher/executor.cpp (line 179)


YOu forgot to update this!



src/launcher/executor.cpp (line 277)


You may want to rename it to 'commandString'.



src/launcher/executor.cpp (line 746)


Can you add a TODO here to deprecate this flag since no one is using it, 
and it'll cause confusing to your newly added flag.



src/launcher/executor.cpp (lines 767 - 769)


If specified, this is the overrided command for launching the task (instead 
of the command from TaskInfo).


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43082/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4004
> https://issues.apache.org/jira/browse/MESOS-4004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new flag to command executor for command passing.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 356d311fdf97b2c4663c60e13ede7cdb71a264c7 
> 
> Diff: https://reviews.apache.org/r/43082/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 42982: Defined a virtual `status` method for Containerizer.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Defined a virtual `status` method for Containerizer.


Diffs
-

  src/slave/containerizer/containerizer.hpp 
08d1ed3cf50f50b5b6e71df60bffbd141490ad32 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42947: Implemented the `status` method in `CgroupNetClsIsolatorProcess`.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 11:19 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `status` method in `CgroupNetClsIsolatorProcess`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42782: Added a unit-test to test net_cls major handles set from command line.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 11:14 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added a unit-test to test net_cls major handles set from command line.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Review Request 43107: Passed agent flag --cgroup_net_cls_primary_handle to net_cls isolator.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

The `cgroup/net_cls` isolator will use the primary handle, passed by the 
operator
using the --cgroup_net_cls_primary_handle, to initialize the `NetClsHandleMgr`.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check

Launched a slave by passing invalid flags to --cgroup_net_cls_primary_handle 
and made sure the slave correctly detects these invalid handles. 

Launched a slave with valid handle to make sure that the slave launches 
correctly.


Thanks,

Avinash sridharan

Review Request 43105: Ensured the allocator does not double account resources.

[Alexander Rukletsov]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

If `addFramework()` is called before `addSlave()` double accounting
of resources is possible. We have not observed bugs because Mesos
currently always adds a framework in the allocator after all agents
with the framework's tasks have been added.


Diffs
-

  include/mesos/master/allocator.hpp e163669c9c4e4c98572968f18987704b60722a79 
  src/master/allocator/mesos/hierarchical.cpp 
1a07d69016407e5aad2209586da37fecbcddb765 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 42781: Added the --cgroups_net_cls_primary_handle flag to the slave.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 11:11 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added the --cgroups_net_cls_primary_handle flag to the slave.


Diffs (updated)
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 75d7429a4e3e1d6259296257c0ace1ade365ac2b 

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


Testing
---

Tested the flag by launching a mesos agent and checking that the slave takes 
the `--cgroups_net_cls_major_handles` flag.


Thanks,

Avinash sridharan

Re: Review Request 42186: Added tests for recovery for HTTP based executors.

[Vinod Kone]

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




src/tests/slave_recovery_tests.cpp (line 420)


s/executor/HTTP based executor/



src/tests/slave_recovery_tests.cpp (line 434)


Can you add a comment here on why you do this for posterity?



src/tests/slave_recovery_tests.cpp (line 460)


Add a TODO here to use a command http executor instead of the custom test 
http executor.



src/tests/slave_recovery_tests.cpp (line 462)


s/default/http/



src/tests/slave_recovery_tests.cpp (line 506)


Was surprised to see a TASK_FINISHED here instead of TASK_RUNNING. I 
commented on the dependent review to fix that. That should change this test 
slightly.



src/tests/slave_recovery_tests.cpp (line 1046)


I think you should start the slave with fixed id like in the above test, to 
ensure the executor tries to connect to the right endpoint.



src/tests/slave_recovery_tests.cpp (line 1072)


ditto. see above.


- Vinod Kone


On Jan. 27, 2016, 4:14 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42186/
> ---
> 
> (Updated Jan. 27, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 6683a081ac231f1e275a3cdb8ee841da430a9f66 
> 
> Diff: https://reviews.apache.org/r/42186/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42185: Added an example executor based on the new V1 API.

[Vinod Kone]

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




src/examples/test_http_executor.cpp (line 154)


I think it makes sense to send a TASK_RUNNING before sending TASK_FINISHED 
as the test_executor does. That's a more realistic behavior of a custome 
executor.


- Vinod Kone


On Jan. 27, 2016, 4:13 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42185/
> ---
> 
> (Updated Jan. 27, 2016, 4:13 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a custom executor based on the new executor library.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bdb34020043f64c07721b3e8b29b221e5b99 
>   src/examples/test_http_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42185/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42185: Added an example executor based on the new V1 API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 27, 2016, 4:13 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42185/
> ---
> 
> (Updated Jan. 27, 2016, 4:13 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a custom executor based on the new executor library.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bdb34020043f64c07721b3e8b29b221e5b99 
>   src/examples/test_http_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42185/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42843: Ensure `ServiceUnavailable` is not received for `Subscribe` calls.

[Anand Mazumdar]

> On Feb. 2, 2016, 10:10 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 502
> > 
> >
> > Looks like HttpProxy can send ServiceUnavailable too for some cases 
> > https://github.com/apache/mesos/blob/b8122f110f826e97f263b982f8fdd14ef53c24f6/3rdparty/libprocess/src/process.cpp#L1137
> > 
> > maybe CHECK isn't prudent here?

Aha, good catch. Let me discard this change.


- Anand


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


On Feb. 2, 2016, 9:56 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42843/
> ---
> 
> (Updated Feb. 2, 2016, 9:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent does not return an `ServiceUnavailable` response for `Subscribe` 
> calls. Added a `CHECK` to ensure this in the executor library.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 92334ffb8af83b1c86c44bd406147893c7c6daa3 
> 
> Diff: https://reviews.apache.org/r/42843/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 43118: Corrected mistakes in docs for volume/reservation HTTP endpoints.

[Neil Conway]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Corrected mistakes in docs for volume/reservation HTTP endpoints.


Diffs
-

  docs/persistent-volume.md 9657c3f75dcc17dfd4d6391b35da3dbc0a6c547d 
  docs/reservation.md 25337109ff19240f926667961a59323bbfeb9956 

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


Testing
---

Previewed in site-docker.


Thanks,

Neil Conway

Re: Review Request 42781: Added the --cgroups_net_cls_primary_handle flag to the slave.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 10:39 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Added the --cgroups_net_cls_primary_handle flag to the slave.


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


Repository: mesos


Description (updated)
---

Added the --cgroups_net_cls_primary_handle flag to the slave.


Diffs (updated)
-

  src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 
  src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 
  src/slave/flags.hpp 23ec158f4a95fa76d657d9eb6a4a6fe30d57e5b6 
  src/slave/flags.cpp 75d7429a4e3e1d6259296257c0ace1ade365ac2b 

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


Testing
---

Tested the flag by launching a mesos agent and checking that the slave takes 
the `--cgroups_net_cls_major_handles` flag.


Thanks,

Avinash sridharan

Re: Review Request 42844: Drop `404 NotFound` responses in the executor library.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 27, 2016, 4:14 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42844/
> ---
> 
> (Updated Jan. 27, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, we did not use to drop `404 NotFound` responses in the library 
> and send `Event::Error` to executor. However, this can be trigerred upon an 
> agent restart when it has not yet set up HTTP routes.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 92334ffb8af83b1c86c44bd406147893c7c6daa3 
> 
> Diff: https://reviews.apache.org/r/42844/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42843: Ensure `ServiceUnavailable` is not received for `Subscribe` calls.

[Vinod Kone]

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




src/executor/executor.cpp (line 502)


Looks like HttpProxy can send ServiceUnavailable too for some cases 
https://github.com/apache/mesos/blob/b8122f110f826e97f263b982f8fdd14ef53c24f6/3rdparty/libprocess/src/process.cpp#L1137

maybe CHECK isn't prudent here?


- Vinod Kone


On Feb. 2, 2016, 9:56 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42843/
> ---
> 
> (Updated Feb. 2, 2016, 9:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent does not return an `ServiceUnavailable` response for `Subscribe` 
> calls. Added a `CHECK` to ensure this in the executor library.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 92334ffb8af83b1c86c44bd406147893c7c6daa3 
> 
> Diff: https://reviews.apache.org/r/42843/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

(Updated Feb. 2, 2016, 5:06 p.m.)


Review request for mesos and Kapil Arya.


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


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs
-

  src/docker/docker.cpp a831726 

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


Testing
---

This patch will first query the docker API for the HostConfig.NetworkMode, 
which is populated with the network name. (Essentially what was passed in --net 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..IPAddress to get the IP address that is 
currently in use by the container.

It appears that even though the docker API has been set up to allow for 
multiple networks, our testing has indicated that it's still only applying one 
network to the container (the last one via the --net argument on the run line). 
I can only speculate that the docker API will change again in the near future, 
but I can't speculate how, so at least this fixes the problem as it stands 
right now.

Tested and working with Docker 1.9.1, Mesos 0.27.0, on Ubuntu 14.04.


Thanks,

Travis Hegner

Re: Review Request 42181: Added a Slave constructor to pass process ID manually.

[Vinod Kone]

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




src/tests/cluster.cpp (lines 409 - 430)


instead of doing if else blocks can you use a ternary operator for the 1st 
argument in the constructor?

```
slave.slave = new Slave::Slave(
  id.isSome() ? id.get() : process::ID::generate("slave"),
  ...
  ...);
```

infact instead of declaring an overloaded constructor in slave.hpp and 
chaining constructors, can you make 'id' an argument with default value?

```
Slave(...,
  const std::string& id = process::ID::generate("slave"))
```

Thoughts?


- Vinod Kone


On Jan. 22, 2016, 8:27 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42181/
> ---
> 
> (Updated Jan. 22, 2016, 8:27 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4255
> https://issues.apache.org/jira/browse/MESOS-4255
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a constructor to Slave that takes in the process ID string 
> as an argument. This is necessary for testing recovery of HTTP based 
> executors. (Previously, every invocation of `StartSlave` used to generate a 
> new ID via `ID::generate`). There was no process delegate set via 
> `process::initialize` that led to problems for the existing HTTP based 
> executor to connect back to the slave.
> 
> Also, modified the tests to introduce a new `StartSlave` function that takes 
> this process ID as argument.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 90d0fecd2d83fd174134870a577ac59d79c0006f 
>   src/tests/cluster.hpp 576dcb8b7e27d1905425aa0f7cb319c19c17c15c 
>   src/tests/cluster.cpp 1a3a038e78999bca2fc3f08725f4ffaf4290bcb9 
>   src/tests/mesos.hpp 3d9ebc6c9dc3cd1be02dc3771fbd847386907fac 
>   src/tests/mesos.cpp 365ebe8335c37bfdb983a5424d4c995fa9b76a22 
> 
> Diff: https://reviews.apache.org/r/42181/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42843: Ensure `ServiceUnavailable` is not received for `Subscribe` calls.

[Anand Mazumdar]

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

(Updated Feb. 2, 2016, 9:56 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Modified description.


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


Repository: mesos


Description (updated)
---

The agent does not return an `ServiceUnavailable` response for `Subscribe` 
calls. Added a `CHECK` to ensure this in the executor library.


Diffs
-

  src/executor/executor.cpp 92334ffb8af83b1c86c44bd406147893c7c6daa3 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 43075: Fixed definition of operator<< for `Call::Type` and `Event::Type`.

[Neil Conway]

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

(Updated Feb. 2, 2016, 9:36 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Minor fixes: add explicit header include, make style consistent with v1 version


Repository: mesos


Description
---

The types were defined in the `mesos::scheduler` namespace, but the
overload of `operator<<` was only defined in the `mesos` namespace.
Because of how ADL works, this usually resulted in the overload
not being found.

Old output: "Dropping 2 call from framework ..."

New output: "Dropping TEARDOWN call from framework ..."


Diffs (updated)
-

  include/mesos/scheduler/scheduler.hpp 
cadd7df11bb6d4210c5abdab5c04586e6686af1d 
  src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83 
  src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43075: Fixed definition of operator<< for `Call::Type` and `Event::Type`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 2, 2016, 9:33 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> ---
> 
> (Updated Feb. 2, 2016, 9:33 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The types were defined in the `mesos::scheduler` namespace, but the
> overload of `operator<<` was only defined in the `mesos` namespace.
> Because of how ADL works, this usually resulted in the overload
> not being found.
> 
> Old output: "Dropping 2 call from framework ..."
> 
> New output: "Dropping TEARDOWN call from framework ..."
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.hpp 
> cadd7df11bb6d4210c5abdab5c04586e6686af1d 
>   src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68 
> 
> Diff: https://reviews.apache.org/r/43075/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43075: Fixed definition of operator<< for `Call::Type` and `Event::Type`.

[Neil Conway]

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

(Updated Feb. 2, 2016, 9:33 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

New approach, per discussion with Vinod.


Summary (updated)
-

Fixed definition of operator<< for `Call::Type` and `Event::Type`.


Repository: mesos


Description (updated)
---

The types were defined in the `mesos::scheduler` namespace, but the
overload of `operator<<` was only defined in the `mesos` namespace.
Because of how ADL works, this usually resulted in the overload
not being found.

Old output: "Dropping 2 call from framework ..."

New output: "Dropping TEARDOWN call from framework ..."


Diffs (updated)
-

  include/mesos/scheduler/scheduler.hpp 
cadd7df11bb6d4210c5abdab5c04586e6686af1d 
  src/sched/sched.cpp 8e51752536041b896e42d44e6e9eae909b0a1d68 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 43075: Fixed error message when dropping a scheduler::Call message.

[Vinod Kone]

> On Feb. 2, 2016, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1801
> > 
> >
> > The output stream operator overload for Call (unversioned) defined in 
> > "mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find 
> > that overload (defined in `mesos` namespace).
> > 
> > It seems to work fine for log statements (that print v1::Call) in 
> > scheduler.cpp. The overload for v1::Call was defined in 
> > "mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the 
> > same namespace as the log statements in scheduler.cpp.
> 
> Neil Conway wrote:
> I _believe_ what is going on here is that the overload of `operator<<` 
> for `mesos::scheduler::Call::Type` is defined in the `mesos` namespace, not 
> the `mesos::scheduler` namespace. Since ADL only considers the "closest 
> enclosing namespace" of the argument (see 
> http://en.cppreference.com/w/cpp/language/adl , case 2(d)), it looks for an 
> overload in `mesos::scheduler` but not `mesos`.
> 
> Defining the overload of `operator<<` in `mesos::scheduler` seems to fix 
> the problem. I think it is generally good practice to group types and 
> functions on those types together into the same namespace, anyway. I'll 
> submit a revised RR doing that shortly.

Yup, that's what I figured out after reading SO. Just doing a `make check` to 
confirm.


- Vinod


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


On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> ---
> 
> (Updated Feb. 2, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Old output: "Dropping 2 call from framework ..."
> 
> New output: "Dropping TEARDOWN call from framework ..."
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83 
> 
> Diff: https://reviews.apache.org/r/43075/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43075: Fixed error message when dropping a scheduler::Call message.

[Neil Conway]

> On Feb. 2, 2016, 7:54 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1801
> > 
> >
> > The output stream operator overload for Call (unversioned) defined in 
> > "mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find 
> > that overload (defined in `mesos` namespace).
> > 
> > It seems to work fine for log statements (that print v1::Call) in 
> > scheduler.cpp. The overload for v1::Call was defined in 
> > "mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the 
> > same namespace as the log statements in scheduler.cpp.

I _believe_ what is going on here is that the overload of `operator<<` for 
`mesos::scheduler::Call::Type` is defined in the `mesos` namespace, not the 
`mesos::scheduler` namespace. Since ADL only considers the "closest enclosing 
namespace" of the argument (see http://en.cppreference.com/w/cpp/language/adl , 
case 2(d)), it looks for an overload in `mesos::scheduler` but not `mesos`.

Defining the overload of `operator<<` in `mesos::scheduler` seems to fix the 
problem. I think it is generally good practice to group types and functions on 
those types together into the same namespace, anyway. I'll submit a revised RR 
doing that shortly.


- Neil


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


On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> ---
> 
> (Updated Feb. 2, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Old output: "Dropping 2 call from framework ..."
> 
> New output: "Dropping TEARDOWN call from framework ..."
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83 
> 
> Diff: https://reviews.apache.org/r/43075/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43081: Supported entrypoint and cmd in docker runtime isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 105)


s/executorLaunchCommand/command/



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 156 - 157)


.. which will be used to launch the executor.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 161)


Do you want to use contianer_config() here?



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 165)


For command task case,



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 165 - 166)


if no need to change the launch command for the user task



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 172 - 175)


1. If 'shell' is true, we will ignore Entrypoint and Cmd from the docker 
image (row 5-8).



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 176 - 178)


If 'shell' is false and 'value' is set, we will ignore the Entrypoint and 
Cmd from the docker image as well (row 3-4).



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 229 - 272)


OK, you pretty much duplicated the logics here below, right? Can we merge 
them? My suggestion earlier should allow you to merge them?



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 233)


container_config() here?



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 276)


return Error?


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43081/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4004
> https://issues.apache.org/jira/browse/MESOS-4004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported entrypoint and cmd in docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43081/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [43093]

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

Error:
2016-02-02 20:06:11 URL:https://reviews.apache.org/r/43093/diff/raw/ 
[1765/1765] -> "43093.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must end in a period.

Full log: https://builds.apache.org/job/mesos-reviewbot/11200/console

- Mesos ReviewBot


On Feb. 2, 2016, 6:52 p.m., Travis Hegner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43093/
> ---
> 
> (Updated Feb. 2, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixes [MESOS-4370]
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp a831726 
> 
> Diff: https://reviews.apache.org/r/43093/diff/
> 
> 
> Testing
> ---
> 
> This patch will first query the docker API for the HostConfig.NetworkMode, 
> which is populated with the network name. (Essentially what was passed in 
> --net  to the docker run command). This name is then used as a key in 
> NetworkSettings.Networks..IPAddress to get the IP address that is 
> currently in use by the container.
> 
> It appears that even though the docker API has been set up to allow for 
> multiple networks, our testing has indicated that it's still only applying 
> one network to the container (the last one via the --net argument on the run 
> line). I can only speculate that the docker API will change again in the near 
> future, but I can't speculate how, so at least this fixes the problem as it 
> stands right now.
> 
> Tested and working with Docker 1.9.1, Mesos 0.27.0, on Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Travis Hegner
> 
>

Re: Review Request 43075: Fixed error message when dropping a scheduler::Call message.

[Vinod Kone]

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




src/master/master.cpp (line 1801)


The output stream operator overload for Call (unversioned) defined in 
"mesos/scheduler/scheduler.hpp". Not sure why the compiler unable to find that 
overload (defined in `mesos` namespace).

It seems to work fine for log statements (that print v1::Call) in 
scheduler.cpp. The overload for v1::Call was defined in 
"mesos/include/mesos/v1/scheduler/scheduler.cpp" though, which is in the same 
namespace as the log statements in scheduler.cpp.


- Vinod Kone


On Feb. 2, 2016, 5:12 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43075/
> ---
> 
> (Updated Feb. 2, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Old output: "Dropping 2 call from framework ..."
> 
> New output: "Dropping TEARDOWN call from framework ..."
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83 
> 
> Diff: https://reviews.apache.org/r/43075/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43037: Support env var in docker runtime isolator.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 96)


s/launchEnvironment/environment/



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 108)


Ditto below.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 110)


Should that be container_config?



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 116 - 117)


I would prefer:
```
foreach(const string& env,
containerConfig.docker().manifest().config().env()) {
}
```



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 122)


You want to print the containerId here as well. So i'll need to pass in the 
'containerId' to this helper method.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 126)


Add an extra line above.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 137)


indentation.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43037/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4383
> https://issues.apache.org/jira/browse/MESOS-4383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support env var in docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43037/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43036: Plugged in docker runtime isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 840 - 841)


Does this compile? Should that be:
```
ContainerConfig::Docker* docker = containerConfig.mutable_docker();
docker->mutable_manifest()->CopyFrom(
provisionInfo->dockerManifest.get());
```



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 78 - 79)


No need for this.



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (lines 81 - 83)


I would move this up as the first check so that you don't have to do 
`executorInfo.has_container()` in later checks


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43036/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4517
> https://issues.apache.org/jira/browse/MESOS-4517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Plugged in docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43036/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43083: Supported working dir in docker runtime isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43019, 43020, 43021, 43022, 43036, 43037, 43081, 43082, 43083]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43083/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4005
> https://issues.apache.org/jira/browse/MESOS-4005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported working dir in docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43083/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43022: Added protobuf fields for docker runtime isolator.

[Jie Yu]

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




include/mesos/slave/isolator.proto (line 85)


Why 'required' here. I would rather keep it 'optional' in case in the 
future, we have v2/v3/v4 manifest.



include/mesos/slave/isolator.proto (line 105)


This is not docker specific right?



include/mesos/slave/isolator.proto (lines 108 - 111)


```
If specified, it'll become the launch command for the custom executor, or 
the launch command for the user task in the case of a command task.
```



include/mesos/slave/isolator.proto (line 114)


Why Docker specific here? It can be used by, for example, appc as well, 
right? Just mention that it's the working directory for the container.



include/mesos/slave/isolator.proto (lines 118 - 122)


Let's not change the numbering here.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43022/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4517
> https://issues.apache.org/jira/browse/MESOS-4517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added protobuf fields for docker runtime isolator.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.proto 8b7c54afc4467084b5d22387e55689d06078b504 
> 
> Diff: https://reviews.apache.org/r/43022/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 42780: Changed the NetClsIsolatorTest to check for net_cls handles.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 7:09 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Changed the NetClsIsolatorTest to check for net_cls handles.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 42780: Changed the NetClsIsolatorTest to check for net_cls handles.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 7:07 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Changed the NetClsIsolatorTest to check for net_cls handles.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 7:05 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function in cgroup for supporting net_cls subsystem.


Diffs (updated)
-

  src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
  src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

(Updated Feb. 2, 2016, 6:52 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs
-

  src/docker/docker.cpp a831726 

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


Testing (updated)
---

This patch will first query the docker API for the HostConfig.NetworkMode, 
which is populated with the network name. (Essentially what was passed in --net 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..IPAddress to get the IP address that is 
currently in use by the container.

It appears that even though the docker API has been set up to allow for 
multiple networks, our testing has indicated that it's still only applying one 
network to the container (the last one via the --net argument on the run line). 
I can only speculate that the docker API will change again in the near future, 
but I can't speculate how, so at least this fixes the problem as it stands 
right now.

Tested and working with Docker 1.9.1, Mesos 0.27.0, on Ubuntu 14.04.


Thanks,

Travis Hegner

Review Request 43093: MESOS-4370 NetworkSettings.IPAddress field is deprectaed in Docker

[Travis Hegner]

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

Review request for mesos and Benjamin Hindman.


Repository: mesos


Description
---

Fixes [MESOS-4370]


Diffs
-

  src/docker/docker.cpp a831726 

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


Testing
---

This patch will first query the docker API for the HostConfig.NetworkMode, 
which is populated with the network name. (Essentially what was passed in --net 
 to the docker run command). This name is then used as a key in 
NetworkSettings.Networks..IPAddress to get the IP address that is 
currently in use by the container.

It appears that even though the docker API has been set up to allow for 
multiple networks, our testing has indicated that it's still only applying one 
network to the container (the last one via the --net argument on the run line). 
I can only speculate that the docker API will change again in the near future, 
but I can't speculate how, so at least this fixes the problem as it stands 
right now.

Tested and working with Docker 1.9.1, Mesos 0.27.0, on Ubuntu 14.04


Thanks,

Travis Hegner

Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.

[Avinash sridharan]

> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 440-443
> > 
> >
> > Can you try to see if writing a decimal number if fine as well? If not, 
> > we should move the helper function into cpp as a file local helper.

Writing a decimal number works. Have moved this into a helper function in 
cgroups, as a separate patch.


> On Feb. 1, 2016, 8:14 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 491-497
> > 
> >
> > I don't think this is necessary.

Agreed.


- Avinash


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


On Feb. 2, 2016, 6:32 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42618/
> ---
> 
> (Updated Feb. 2, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42618/diff/
> 
> 
> Testing
> ---
> 
> make and make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42618: Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 6:32 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.


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


Repository: mesos


Description (updated)
---

Modified the `cgroup/net_cls` isolator to use the `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 43021: Implemented docker runtime isolator interface.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/docker/runtime.hpp (line 26)


s/Isolator/isolator/



src/slave/containerizer/mesos/isolators/docker/runtime.cpp (line 23)


We try to avoid using this now. Let's use 'using process::XXX' explicitly 
here.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43021/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4517
> https://issues.apache.org/jira/browse/MESOS-4517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker runtime isolator interface.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6b2c7cbffa57d44b913419ad68fc66eaeca169d0 
>   src/Makefile.am fac17f4bac3b2ddda0384dec8b0ed6f960bd24d1 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43021/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 43021: Implemented docker runtime isolator interface.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43021/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4517
> https://issues.apache.org/jira/browse/MESOS-4517
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker runtime isolator interface.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 6b2c7cbffa57d44b913419ad68fc66eaeca169d0 
>   src/Makefile.am fac17f4bac3b2ddda0384dec8b0ed6f960bd24d1 
>   src/slave/containerizer/mesos/isolators/docker/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43021/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 43096: Added helper function in cgroup for supporting net_cls subsystem.

[Avinash sridharan]

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

Review request for mesos.


Repository: mesos


Description
---

Added helper function in cgroup for supporting net_cls subsystem.


Diffs
-

  src/linux/cgroups.hpp 83b3e401ac69194bef496087dee32e0532b834ab 
  src/linux/cgroups.cpp bbc1fb3c60e53dffb1e8029942725e7102eb9aee 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42588: Added unit-test for `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 6:25 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleMgr`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 43020: Exposed accurate ProvisionInfo to command executor.

[Jie Yu]

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




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


Add some more comments here:
```
For command tasks (i.e., taskInfo.isSome()), the filesystem image for the 
task is specified in a volume (COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH). We need 
to pass the provisionInfo from that image to isolators through 'prepare'.
```



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


I would do the following:

```
Owned> _provisionInfo(new 
ProvisionInfo(provisionInfo));
```



src/slave/containerizer/mesos/containerizer.cpp (lines 738 - 739)


Merge these in one line.



src/slave/containerizer/mesos/containerizer.cpp (lines 738 - 751)


```
futures.push_back(
  provisioner->provision(containerId, image)
.then([=](const ProvisionInfo& info) -> Future {
  volume->set_host_path(info.rootfs);
  
  // Add some comments here about why we need to do this!
  if (taskInfo.isSome() &&
  volume->container_path() ==
COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH) {
_provisionInfo.reset(new ProvisionInfo(info));
  }
}));
```



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


capturing reference here is not correct. It's already a pointer, no need to 
capturing the reference of a pointer.



src/slave/containerizer/mesos/containerizer.cpp (lines 758 - 761)


No need for this.



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


`*_provisionInfo` here.


- Jie Yu


On Feb. 2, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43020/
> ---
> 
> (Updated Feb. 2, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed accurate ProvisionInfo to command executor.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4b504dbb58823ce7675f1d2048dcc7a27c05663d 
> 
> Diff: https://reviews.apache.org/r/43020/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

[Avinash sridharan]

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

(Updated Feb. 2, 2016, 6:16 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

[Avinash sridharan]

> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 137-139
> > 
> >
> > Hum, having two data fields here looks a little confusing to me. How 
> > about just merging them:
> > 
> > ```
> > // A map for used handles: primary -> bitset for secondary.
> > hashmap> used;
> > ```

Used a typedef for bitset<0x+1> to improve readability.


> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 73-78
> > 
> >
> > I got confused here. minor is a uint16 value. Why this check is 
> > necessary?

Removed this validation. Always set the first element of the bitmask when 
initializing it, so no longer need the check on the secondary handles.


> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 82-96
> > 
> >
> > For testin, I would probably just expose a 'isUsed' method:
> > ```
> > bool isUsed(const NetClsHandle& handle);
> > ```

Removed usage(). Will introduce isUsed with the unit-test patch.


- Avinash


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


On Feb. 2, 2016, 6:16 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> ---
> 
> (Updated Feb. 2, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the `NetClsHandleMgr` class.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42587/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>