Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57815, 57817, 57818]

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

- Mesos Reviewbot


On May 6, 2017, 3:30 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57818/
> ---
> 
> (Updated May 6, 2017, 3:30 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to verify offers are suppressed based on registration.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp ceee2f4a5d38e0f4200f444769e058d2173de821 
>   src/tests/scheduler_tests.cpp 0f5d9ada6eb880379baf5f106fd2d5b12e9738db 
> 
> 
> Diff: https://reviews.apache.org/r/57818/diff/7/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58925: Updated runtime isolators to use new task_environment member.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 2, 2017, 10:28 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58925/
> ---
> 
> (Updated May 2, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> ffaec5a0112db36c1e9123e361ada63a4aedf0ad 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 08350e638a0f20746e369cdc78c96126f2e1df3f 
> 
> 
> Diff: https://reviews.apache.org/r/58925/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58924: Updated containerizer for isolator task_environment merge.

[Gilbert Song]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 1333 (patched)


Overwriting task environment variable 'name' from 'old value' to 'new 
value'.

maybe better?

btw, mind fix it at launcher/executor.cpp as well?



src/slave/containerizer/mesos/containerizer.cpp
Lines 1343-1345 (patched)


Is this comment needed?



src/slave/containerizer/mesos/containerizer.cpp
Lines 1371 (patched)


for command task specific,


- Gilbert Song


On May 2, 2017, 10:28 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58924/
> ---
> 
> (Updated May 2, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
> 
> 
> Diff: https://reviews.apache.org/r/58924/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58923: Added new ContainerLaunchInfo task_environment.

[Gilbert Song]

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




include/mesos/slave/containerizer.proto
Lines 165-167 (patched)


This comment is not accurate. The `environment` is not always for 
executors. It can be for nested containers (so task_group task inside an 
executor). So let's explain it as `The environment set for the container.`

And not necessary to mention the inheritance here.



include/mesos/slave/containerizer.proto
Lines 189-190 (patched)


I would rephase it as `The environment for command tasks specific.`

And add a `TODO` for deprecating this field once command task is deprecated.



include/mesos/slave/containerizer.proto
Lines 191 (patched)


BTW, let's move this field to the last position since it is for command 
task specific and will be deprecated.


- Gilbert Song


On May 5, 2017, 7:46 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58923/
> ---
> 
> (Updated May 5, 2017, 7:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
> 
> 
> Diff: https://reviews.apache.org/r/58923/diff/2/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 59038: Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59038]

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

- Mesos Reviewbot


On May 6, 2017, 12:50 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59038/
> ---
> 
> (Updated May 6, 2017, 12:50 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Michael Park.
> 
> 
> Bugs: MESOS-7462
> https://issues.apache.org/jira/browse/MESOS-7462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33d5c0ea0182e09b3f3f30d20a33d46c23d81697 
> 
> 
> Diff: https://reviews.apache.org/r/59038/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose 
> --gtest_repeat=1000 --gtest_break_on_failure .`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [58939]

Failed command: python support/apply-reviews.py -n -r 58939

Error:
2017-05-07 03:00:19 URL:https://reviews.apache.org/r/58939/diff/raw/ 
[2628/2628] -> "58939.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/provisioner/provisioner.cpp:218
error: src/slave/containerizer/mesos/provisioner/provisioner.cpp: patch does 
not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/17979/console

- Mesos Reviewbot


On May 5, 2017, 6:07 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated May 5, 2017, 6:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 08350e638a0f20746e369cdc78c96126f2e1df3f 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> be45fc59027f176b43b767e9441fd8089ceec7b4 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58892: Added C++11 scoped enumeration to style guide.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58892]

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

- Mesos Reviewbot


On May 2, 2017, 10:11 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58892/
> ---
> 
> (Updated May 2, 2017, 10:11 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We've been using it for new code now.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md ccb81f48e2edc9c1e7c328cc26e44d658b4c43b4 
> 
> 
> Diff: https://reviews.apache.org/r/58892/diff/2/
> 
> 
> Testing
> ---
> 
> Markdown viewer.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52064: Support for multiple versions of docs.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [52064]

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

- Mesos Reviewbot


On May 5, 2017, 9:23 p.m., Tim Anderegg wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52064/
> ---
> 
> (Updated May 5, 2017, 9:23 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Vinod Kone.
> 
> 
> Bugs: MESOS-3011
> https://issues.apache.org/jira/browse/MESOS-3011
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support for multiple versions of docs.
> 
> 
> Diffs
> -
> 
>   site/Gemfile 4dcd0a5ad452085e6e3bb152a6547575636cd807 
>   site/Gemfile.lock 1393246f06631155bb052669a86e431778e891bf 
>   site/Rakefile 01356891c29f9e69fa0f7813cf87e7662eda400b 
>   site/config.rb 9e6738fccc48365e16221188269c1ed40772eca7 
>   site/data/releases.yml 8bdc3ff11a821f5e1be04ac871c05b16403a82ac 
>   site/source/assets/js/versions.js PRE-CREATION 
>   site/source/layouts/basic.erb 3bf1f55d7d8feafd3caaa7902755404f7e45bae6 
> 
> 
> Diff: https://reviews.apache.org/r/52064/diff/5/
> 
> 
> Testing
> ---
> 
> Testing was done manually to verify that the documentation was built for each 
> version of Mesos that is supported (some older versions do not have 
> compatible documentation).
> 
> 
> Thanks,
> 
> Tim Anderegg
> 
>

Re: Review Request 57815: Added `deactivated_roles` field in `FrameworkInfo`.

[Anindya Sinha]

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

(Updated May 6, 2017, 10:30 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This field is a subset of roles the framework registered as for which
the framework does not want any resources offere to.


Diffs (updated)
-

  include/mesos/mesos.proto 1935f47a52840f6b395ecb2d2829551fa691 
  include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7 


Diff: https://reviews.apache.org/r/57815/diff/5/

Changes: https://reviews.apache.org/r/57815/diff/4-5/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated May 6, 2017, 10:30 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/master_tests.cpp ceee2f4a5d38e0f4200f444769e058d2173de821 
  src/tests/scheduler_tests.cpp 0f5d9ada6eb880379baf5f106fd2d5b12e9738db 


Diff: https://reviews.apache.org/r/57818/diff/7/

Changes: https://reviews.apache.org/r/57818/diff/6-7/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57817: Offers not sent for deactivated roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated May 6, 2017, 10:30 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

If the framework indicates a subset of roles to be deactivated, the
allocator does not offer resources for those roles to such frameworks.
In addition, the master validates the deactivated roles to be a
subset of all the roles being (re)registered by the framework.


Diffs (updated)
-

  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
  src/master/allocator/mesos/hierarchical.hpp 
123f97cf495bff0f822838e09df0d88818f04da6 
  src/master/allocator/mesos/hierarchical.cpp 
b75ed9a20a9a42f958cebbacd91e5e15b0d21394 
  src/master/master.cpp 4e7a161f431624bd78d3e9032eb8587687149cad 
  src/tests/hierarchical_allocator_tests.cpp 
33d5c0ea0182e09b3f3f30d20a33d46c23d81697 


Diff: https://reviews.apache.org/r/57817/diff/7/

Changes: https://reviews.apache.org/r/57817/diff/6-7/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59029: Added a script to publish JARs to maven snapshot repository.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59029]

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

- Mesos Reviewbot


On May 5, 2017, 8:03 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59029/
> ---
> 
> (Updated May 5, 2017, 8:03 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This could be used to publish JARs from nightly builds. Most of the
> code is copied from support/tag.sh.
> 
> 
> Diffs
> -
> 
>   support/snapshot.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59029/diff/1/
> 
> 
> Testing
> ---
> 
> Manually uploaded a snapshot build to maven
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 59001: Added volume secret isolator.

[Kapil Arya]

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

(Updated May 6, 2017, 5:29 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Fixed Makefile.


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


Repository: mesos


Description
---

Added volume secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt 89cbd3f5a93f4891e8272d3b1136059ab1069d01 
  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/isolators/volume/secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/rootfs.cpp fdfecc65a3fcd19d6a4dfa574320f4d1f2755322 
  src/tests/containerizer/volume_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59001/diff/4/

Changes: https://reviews.apache.org/r/59001/diff/3-4/


Testing
---

Added new tests an ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59038: Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF.

[Anindya Sinha]

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


Fix it, then Ship it!




Ship It!


src/tests/hierarchical_allocator_tests.cpp
Line 745 (original), 745 (patched)


nitpik: Maybe change the order of the comment to reflect that the 2 
frameworks are added in roles "a/c" and "d/e" respectively followed by adding 
of the slave.



src/tests/hierarchical_allocator_tests.cpp
Line 783 (original), 783 (patched)


nitpik: Similar comment as the previous one.


- Anindya Sinha


On May 6, 2017, 12:50 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59038/
> ---
> 
> (Updated May 6, 2017, 12:50 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Michael Park.
> 
> 
> Bugs: MESOS-7462
> https://issues.apache.org/jira/browse/MESOS-7462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flakiness in HierarchicalAllocatorTest.NestedRoleDRF.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33d5c0ea0182e09b3f3f30d20a33d46c23d81697 
> 
> 
> Diff: https://reviews.apache.org/r/59038/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="HierarchicalAllocatorTest.*" --verbose 
> --gtest_repeat=1000 --gtest_break_on_failure .`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56193: Use Docker cpus instead of cpu_share if possible.

[Tomasz Janiszewski]

> On Mar 27, 2017, 8:38 po południu, Jie Yu wrote:
> > src/docker/docker.cpp
> > Lines 524 (patched)
> > 
> >
> > Can you test if docker support arbitratry double here like 1.01 
> > (more than 3 digits after decimal point)?
> > 
> > Also, looks like 0.000 means unlimited. I am wondering if we should set 
> > a MIN_CPUS here just to be consistent with the cpu share based semantics.

Yep, 0.000 means unlimited. Docker internay represnets cpus as 
[`NanoCPUs`](https://github.com/moby/moby/blob/a258ef58d8a100467d5d948b026a884ebe58eaf4/opts/opts.go#L344-L383).
 After introducting DockerRunOptions this patch makes no sense until docker 
deprecate old CPUs respurces API. I'll close it.


- Tomasz


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


On Lut 1, 2017, 10:31 po południu, Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56193/
> ---
> 
> (Updated Lut 1, 2017, 10:31 po południu)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7006
> https://issues.apache.org/jira/browse/MESOS-7006
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker 1.13 introduces new flag `--cpus` for limiting cpu usage
> of a container. We can use this flag to avoid cpu share calculation
> and just pass cpus resources to Docker.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
> 
> 
> Diff: https://reviews.apache.org/r/56193/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>