Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 

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

(Updated July 4, 2019, 12:26 a.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James Peach.


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


Repository: mesos


Description
---

This patch adds the isolation capability of flipping the
NO_NEW_PRIVILEGES bit for process control.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
2d04f3c182a4274dda527a3da56c894c3c892a12 
  src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
  src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
  src/slave/containerizer/mesos/containerizer.cpp 
043244841a73fa3f5f7119bc38f6d3a04be8990b 
  src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
b29ec556399a40aa662987a11a2b64e6a16de889 
  src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
  src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Jacob Janco

Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 

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

(Updated July 4, 2019, 12:18 a.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James Peach.


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


Repository: mesos


Description
---

This patch adds the isolation capability of flipping the
NO_NEW_PRIVILEGES bit for process control.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
2d04f3c182a4274dda527a3da56c894c3c892a12 
  src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
  src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
  src/slave/containerizer/mesos/containerizer.cpp 
043244841a73fa3f5f7119bc38f6d3a04be8990b 
  src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
b29ec556399a40aa662987a11a2b64e6a16de889 
  src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
  src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70757/diff/3/

Changes: https://reviews.apache.org/r/70757/diff/2-3/


Testing
---


Thanks,

Jacob Janco

Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 


> On May 31, 2019, 10:31 a.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 1099 (patched)
> > 
> >
> > `PR_SET_NO_NEW_PRIVS` requires Linux kernel 3.5 (according to `man 2 
> > prctl`).
> > 
> > Minimum supported kernel version by Mesos is 2.6.28 - see 
> > http://mesos.apache.org/documentation/latest/building/#system-requirements
> > 
> > There are at least 2 options to fix this problem:
> > 1) bump the minimum kernel version to 3.5, if no one is against it on 
> > the dev/user list
> > 2) remove `include ` and define `PR_SET_NO_NEW_PRIVS` in 
> > the `containerizer/mesos/launch.cpp`. See 
> > https://github.com/apache/mesos/blob/fa410f2fb8efb988590f4da2d4cfffbb2ce70637/src/slave/containerizer/mesos/isolators/linux/seccomp.cpp#L35-L41
> >  and 
> > https://github.com/seccomp/libseccomp/blob/78497a5d1da200ab0356e1189f5efb8724ad70a1/src/system.h#L94-L97
> 
> James Peach wrote:
> To make a kernel version check, you can hoist `kernelVersion` from ns.cpp 
> into common code.

Cool, moved it up


- Jacob


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


On July 3, 2019, 11:30 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated July 3, 2019, 11:30 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2d04f3c182a4274dda527a3da56c894c3c892a12 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 043244841a73fa3f5f7119bc38f6d3a04be8990b 
>   src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> b29ec556399a40aa662987a11a2b64e6a16de889 
>   src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
>   src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 


> On May 31, 2019, 4:24 a.m., James Peach wrote:
> > You should add a test in this review. I'd probably do something reasonable 
> > simple, like launch a task and verify that the `NoNewPrivs` fiels in 
> > `/proc/$PID/status` is what you expect it to be. Take a look at 
> > `src/tests/containerizer/linux_filesystem_isolator_tests.cpp` for an 
> > example of how to construct the test by driving the containerizer directly.
> > 
> > We also need to add feature and upgrade documentation, but if you want to 
> > do that in a later review, that's fine with me.

Yea later review sounds good to me.


> On May 31, 2019, 4:24 a.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/linux/privs.hpp
> > Lines 30 (patched)
> > 
> >
> > I did originally advise you to call this isolator something more open 
> > ended, because I had it in mind that we could extend this to toggle secbits 
> > as well.
> > 
> > I've reconsidered and I think that I gave you bad advice. Let's check 
> > in with the other containerization folks, but I'm inclined to call this 
> > `linux/nnp` and leave it at that.
> > 
> > Sorry for the back & forth.

Yea scoping it down makes sense.


- Jacob


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


On July 3, 2019, 11:30 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated July 3, 2019, 11:30 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2d04f3c182a4274dda527a3da56c894c3c892a12 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 043244841a73fa3f5f7119bc38f6d3a04be8990b 
>   src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> b29ec556399a40aa662987a11a2b64e6a16de889 
>   src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
>   src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 


> On May 31, 2019, 10:31 a.m., Andrei Budnik wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 1098 (patched)
> > 
> >
> > Can this condition ever be `false`, given the isolator assigns `true` 
> > in any case?
> > 
> > What is the semantics of `ContainerLaunchInfo::no_new_privileges` flag? 
> > How does it work in pair with the new isolator?

I don't have a default value set in the proto so I think checking here is sane. 
The user can set this to false/true explicitly.


- Jacob


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


On July 3, 2019, 11:30 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated July 3, 2019, 11:30 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2d04f3c182a4274dda527a3da56c894c3c892a12 
>   src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 043244841a73fa3f5f7119bc38f6d3a04be8990b 
>   src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> b29ec556399a40aa662987a11a2b64e6a16de889 
>   src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
>   src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 70757: Added a NNP isolator.

2019-07-03 Thread Jacob Janco 

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

(Updated July 3, 2019, 11:30 p.m.)


Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James Peach.


Summary (updated)
-

Added a NNP isolator.


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


Repository: mesos


Description
---

This patch adds the isolation capability of flipping the
NO_NEW_PRIVILEGES bit for process control.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
2d04f3c182a4274dda527a3da56c894c3c892a12 
  src/CMakeLists.txt 1d4f541b73c07a307a8b61f217e0cfad5dc095e4 
  src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
  src/slave/containerizer/mesos/containerizer.cpp 
043244841a73fa3f5f7119bc38f6d3a04be8990b 
  src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
b29ec556399a40aa662987a11a2b64e6a16de889 
  src/tests/CMakeLists.txt 3ea8b4400ff3b00470aa147cb8f39f62802727e3 
  src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70757/diff/2/

Changes: https://reviews.apache.org/r/70757/diff/1-2/


Testing
---


Thanks,

Jacob Janco

Re: Review Request 70984: Added a test for 'suppressOffers(roles)'.

2019-07-03 Thread Benjamin Mahler 

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


Fix it, then Ship it!




For this and the other test, let's clarify the commit message:

> Added a scheduler driver test for 'suppressOffers(roles)'.


src/tests/scheduler_driver_tests.cpp
Lines 543-546 (patched)


This looks racy, we should settle to ensure the suppress is processed 
before adding the agent? Otherwise the offer can go to role1.


- Benjamin Mahler


On July 1, 2019, 1:48 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70984/
> ---
> 
> (Updated July 1, 2019, 1:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for 'suppressOffers(roles)'.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_driver_tests.cpp 
> 46af8d2c0fff15974b028521d1434e67bd9217da 
> 
> 
> Diff: https://reviews.apache.org/r/70984/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="*SchedulerDriver*SuppressSingleRole*" 
> --gtest_break_on_failure --gtest_repeat=1000`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70942: Added a test for 'reviveOffers(roles)'.

2019-07-03 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- Benjamin Mahler


On July 3, 2019, 3:55 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70942/
> ---
> 
> (Updated July 3, 2019, 3:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for 'reviveOffers(roles)'.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_driver_tests.cpp 
> 80f3335b0054ed0e458c175700765293dfd6a929 
> 
> 
> Diff: https://reviews.apache.org/r/70942/diff/4/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh 
> --gtest_filter="MesosSchedulerDriverTest*ReviveSingleRole*" 
> --gtest_break_on_failure --gtest_repeat=1000`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70942: Added a test for 'reviveOffers(roles)'.

2019-07-03 Thread Benjamin Mahler 


> On June 25, 2019, 7:44 p.m., Benjamin Mahler wrote:
> > src/tests/scheduler_driver_tests.cpp
> > Lines 514-516 (patched)
> > 
> >
> > This seems unnecessary?
> 
> Andrei Sekretenko wrote:
> Removed this `settle()`. 
> 
> It does nothing useful except for preventing a potential race between 
> `EXPECT_CALL(resourceOffers, ...)` and the driver calling `resourceOffers()` 
> (which is possible only if the offer filters / framework update are broken).
> And testing offer filters / framework update is not the purpose of this 
> test.
> 
> Andrei Sekretenko wrote:
> I was mistaken. There is a reason why we need to `settle()` here. It 
> prevents reordering of updateFramework() and revive() in the allocator, which 
> is possible if master processes REVIVE while UPDATE_FRAMEWORK is being 
> authorized.

thanks for documenting it


- Benjamin


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


On July 3, 2019, 3:55 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70942/
> ---
> 
> (Updated July 3, 2019, 3:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for 'reviveOffers(roles)'.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_driver_tests.cpp 
> 80f3335b0054ed0e458c175700765293dfd6a929 
> 
> 
> Diff: https://reviews.apache.org/r/70942/diff/4/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh 
> --gtest_filter="MesosSchedulerDriverTest*ReviveSingleRole*" 
> --gtest_break_on_failure --gtest_repeat=1000`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70983: Added to the scheduler driver a method to suppress a subset of roles.

2019-07-03 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- Benjamin Mahler


On July 3, 2019, 5:05 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70983/
> ---
> 
> (Updated July 3, 2019, 5:05 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds to the scheduler driver a 'suppressOffers(roles)'
> method, which sends the SUPPRESS call for these roles and adds them to
> the suppressed roles set.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4 
>   src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516 
> 
> 
> Diff: https://reviews.apache.org/r/70983/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70941: Added to the scheduler driver a method to revive a subset of roles.

2019-07-03 Thread Benjamin Mahler 

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


Ship it!




Ship It!

- Benjamin Mahler


On July 3, 2019, 3:52 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70941/
> ---
> 
> (Updated July 3, 2019, 3:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
> which sends the REVIVE call for these roles and removes them from the
> suppressed roles set.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4 
>   src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516 
> 
> 
> Diff: https://reviews.apache.org/r/70941/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70985: Added `suppressOffers(roles)` to V0 Java bindings.

2019-07-03 Thread Benjamin Mahler 

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




src/java/src/org/apache/mesos/MesosSchedulerDriver.java
Lines 402-404 (patched)


Why the extra level of indirection? Why not just make this native like the 
others?


- Benjamin Mahler


On July 3, 2019, 5:06 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70985/
> ---
> 
> (Updated July 3, 2019, 5:06 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `suppressOffers(roles)` to V0 Java bindings.
> 
> 
> Diffs
> -
> 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
> f81a69a7f13f976a09f11409a0ea3f40fccea1b8 
>   src/java/src/org/apache/mesos/MesosSchedulerDriver.java 
> 772a7383328519e63f0bf31908cb4c2129a77315 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> 155e4ac7b0e507c549a2d2de07ab95e215bf39e0 
> 
> 
> Diff: https://reviews.apache.org/r/70985/diff/2/
> 
> 
> Testing
> ---
> 
> Called this method from a modified example framework (WIP).
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70947: Added `reviveOffers(roles)` to V0 Java bindings.

2019-07-03 Thread Benjamin Mahler 

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




src/java/src/org/apache/mesos/MesosSchedulerDriver.java
Lines 396-398 (patched)


Why the extra level of indirection? Why not just make this native like the 
others?


- Benjamin Mahler


On July 3, 2019, 5:03 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70947/
> ---
> 
> (Updated July 3, 2019, 5:03 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `reviveOffers(roles)` to V0 Java bindings.
> 
> 
> Diffs
> -
> 
>   src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
> f81a69a7f13f976a09f11409a0ea3f40fccea1b8 
>   src/java/src/org/apache/mesos/MesosSchedulerDriver.java 
> 772a7383328519e63f0bf31908cb4c2129a77315 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> 155e4ac7b0e507c549a2d2de07ab95e215bf39e0 
> 
> 
> Diff: https://reviews.apache.org/r/70947/diff/3/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="ExamplesTest.JavaFramework" 
> --gtest_break_on_failure --gtest_repeat=10` with a patch from 
> https://reviews.apache.org/r/70946/
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70995: Fixed bugs in updating framework roles in the hierarchial allocator.

2019-07-03 Thread Benjamin Mahler 

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




src/master/allocator/mesos/hierarchical.cpp
Lines 533 (patched)


s/roles/roles /


- Benjamin Mahler


On July 2, 2019, 7:12 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70995/
> ---
> 
> (Updated July 2, 2019, 7:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9870
> https://issues.apache.org/jira/browse/MESOS-9870
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch replaces the largest part of the logic around
> suppressing/unsuppressing framework roles in the 'updateFramework()'
> method with calling 'suppressRoles()'/'unsuppress()' methods, which
> are used for processing SUPPRESS/REVIVE calls.
> 
> This fixes bugs which are triggered by simulatneous updates of
> framework's roles and its suppressed roles (see MESOS-9870), namely:
>  - master crashes due to changing nonexistent role metrics
>  - master crash due to activating removed frameworkSorter
>  - spurious activation of a role added in suppressed state
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 26aad6778f12b99bb87c846788d6b6d60f743d8a 
> 
> 
> Diff: https://reviews.apache.org/r/70995/diff/1/
> 
> 
> Testing
> ---
> 
> make check + new tests in dependent patches
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70995: Fixed bugs in updating framework roles in the hierarchial allocator.

2019-07-03 Thread Benjamin Mahler 

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


Fix it, then Ship it!




Clean fix, thanks!

Looks to me like if we incrementally update `framework.roles`, then we have 
some clear code to pull out into `addSubscribedRole(Framework*, role)` and 
`removeSubscribedRole(Framework*, role)`, but we can do this later.


src/master/allocator/mesos/hierarchical.cpp
Lines 531 (patched)


Maybe just:

```
  CHECK_EQ(framework.suppressedRoles, suppressedRoles);
```



src/master/allocator/mesos/hierarchical.cpp
Line 1397 (original), 1358 (patched)


We should probably rename this to unsuppressRole (in a different change).



src/master/allocator/mesos/hierarchical.cpp
Lines 1361-1362 (patched)


This is double logging and its an inconsistency between suppressRoles and 
unsuppressRoles?

It's also saying "revived" but it's not clearing the filters? (which is 
part of revive)

Per the last review, let's just log that we unsuppressed roles here.


- Benjamin Mahler


On July 2, 2019, 7:12 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70995/
> ---
> 
> (Updated July 2, 2019, 7:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9870
> https://issues.apache.org/jira/browse/MESOS-9870
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch replaces the largest part of the logic around
> suppressing/unsuppressing framework roles in the 'updateFramework()'
> method with calling 'suppressRoles()'/'unsuppress()' methods, which
> are used for processing SUPPRESS/REVIVE calls.
> 
> This fixes bugs which are triggered by simulatneous updates of
> framework's roles and its suppressed roles (see MESOS-9870), namely:
>  - master crashes due to changing nonexistent role metrics
>  - master crash due to activating removed frameworkSorter
>  - spurious activation of a role added in suppressed state
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 26aad6778f12b99bb87c846788d6b6d60f743d8a 
> 
> 
> Diff: https://reviews.apache.org/r/70995/diff/1/
> 
> 
> Testing
> ---
> 
> make check + new tests in dependent patches
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70994: Moved to sepearte methods the (un)suppressing logic common with update.

2019-07-03 Thread Benjamin Mahler 

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


Fix it, then Ship it!




How about:

```
Extracted suppression logic in allocator for use in update framework.

This patch moves the logic of suppressing/unsuppressing a role set from
the inside of 'suppressOffers()'/'reviveOffers()' into separate methods.
Specifically, 'reviveOffers()' includes filter clearing logic that we
don't want when unsuppressing roles during framework update. For
'supppressOffers()', we need the empty set == all roles semantics, but
we don't want that in the suppression logic during framework update.

Longer term, the empty set == all roles semantics could be done in
the master and we won't need the extra function to provide empty set
== all roles logic in the allocator.

This is a prerequisite for using thes methods to fix
'updateFramework()' in a subsequent patch.
```

Note that I think we don't need the extra suppression function in the 
allocator, however we need to update the master to provide the empty set == all 
roles semantics (which is more of a protobuf API concern than something the 
allocator should know about).


src/master/allocator/mesos/hierarchical.cpp
Line 1342 (original), 1342 (patched)


Why not take `Framework*` in both of these?

...

Wow, in the process of looking into this, I see we don't store the 
FrameworkID inside the Framework struct.. we should change that, but for this 
patch taking FrameworkID is ok. Maybe some TODOs.



src/master/allocator/mesos/hierarchical.cpp
Lines 1363-1364 (original), 1375-1376 (patched)


Probably we should just log this in the suppressRoles function? Ideally, we 
only log those roles that transitioned from unsuppressed -> suppressed, but we 
currently log the no-ops for roles that are already unsuppressed.

A TODO is fine for now:

```
  // TODO(bmahler): This logs roles that were already suppressed,
  // only log roles that transitioned from unsuppressed -> suppressed.
  LOG(INFO) << "Suppressed offers for roles " << stringify(roles)
<< " of framework " << frameworkId;
```



src/master/allocator/mesos/hierarchical.cpp
Line 1391 (original), 1398 (patched)


To be consistent:

```
  // TODO(bmahler): This logs roles that were already unsuppressed,
  // only log roles that transitioned from suppressed -> unsuppressed.
  LOG(INFO) << "Unsuppressed offers for roles " << stringify(roles)
<< " of framework " << frameworkId;
```

reviveOffers can keep logging, since we also want to know when filters were 
cleared:

```
  LOG(INFO) << "Revived roles " << stringify(roles)
<< " of framework " << frameworkId;
```


- Benjamin Mahler


On July 2, 2019, 7:10 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70994/
> ---
> 
> (Updated July 2, 2019, 7:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9870
> https://issues.apache.org/jira/browse/MESOS-9870
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moves the logic of suppressing/unsuppressing a role set from
> the inside of 'suppressOffers()'/'reviveOffers()' into separate methods.
> 
> This is a prerequisite for using this methods to fix updateFramework()
> in the dependent patch.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 7e9765263f969a4499358579f1ee5bb1afb053da 
>   src/master/allocator/mesos/hierarchical.cpp 
> 26aad6778f12b99bb87c846788d6b6d60f743d8a 
> 
> 
> Diff: https://reviews.apache.org/r/70994/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70980: Removed deactivation of already inactive roles on framework update.

2019-07-03 Thread Benjamin Mahler 

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



Did you want to discard this?

- Benjamin Mahler


On June 29, 2019, 12:56 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70980/
> ---
> 
> (Updated June 29, 2019, 12:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9870
> https://issues.apache.org/jira/browse/MESOS-9870
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a refactoring of `updateFramework()` in the hierarchial
> allocator which makes the symmetry between activating and
> deactivating roles more visible.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 26aad6778f12b99bb87c846788d6b6d60f743d8a 
> 
> 
> Diff: https://reviews.apache.org/r/70980/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70906: Fixed pid checkpointing for `TestContainerizer`.

2019-07-03 Thread Joseph Wu 

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


Ship it!




Ship It!

- Joseph Wu


On June 21, 2019, 2:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70906/
> ---
> 
> (Updated June 21, 2019, 2:59 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order for a `MockExecutor` to be able to reregister after agent
> restart a persisted pid is required. This patch adds checkpointing of
> the pid.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer.cpp fab7e81339f3b7b3fec2a4f6cb7e3a52df2607ed 
> 
> 
> Diff: https://reviews.apache.org/r/70906/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71009: Fixed an compilation issue due to macro expansion.

2019-07-03 Thread Joseph Wu 

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


Ship it!




Ship It!

- Joseph Wu


On July 3, 2019, 10:59 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71009/
> ---
> 
> (Updated July 3, 2019, 10:59 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an compilation issue due to macro expansion.
> 
> 
> Diffs
> -
> 
>   src/tests/registrar_tests.cpp 1343b5e7c3309920fffa1fbf1eb9f23f77356e60 
> 
> 
> Diff: https://reviews.apache.org/r/71009/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70946: WIP: Added reviveOffers() and the new constructor to Java TestFramework.

2019-07-03 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On June 25, 2019, 9:30 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70946/
> ---
> 
> (Updated June 25, 2019, 9:30 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-9793 and MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Added reviveOffers() and the new constructor to Java TestFramework.
> 
> 
> Diffs
> -
> 
>   src/examples/java/TestFramework.java 
> c8b0ceacd1305f7ff67f2ef490e14513d2757f5a 
> 
> 
> Diff: https://reviews.apache.org/r/70946/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="ExamplesTest.JavaFramework" 
> --gtest_break_on_failure --gtest_repeat=10`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 71009: Fixed an compilation issue due to macro expansion.

2019-07-03 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Fixed an compilation issue due to macro expansion.


Diffs
-

  src/tests/registrar_tests.cpp 1343b5e7c3309920fffa1fbf1eb9f23f77356e60 


Diff: https://reviews.apache.org/r/71009/diff/1/


Testing
---

make check


Thanks,

Meng Zhu

Review Request 71008: Implemented transition from DRAINING to DRAINED in master.

2019-07-03 Thread Joseph Wu 

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


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


Repository: mesos


Description
---

This adds logic in the master to detect when a DRAINING agent can
be transitioned into a DRAINED state.  When this happens, the new
state is checkpointed into the registry and, if the agent is to be
marked "gone", the master will remove the agent.


Diffs
-

  src/master/master.hpp 23dafe746b6f9b3d70ad7220f54c4d49068b8af8 
  src/master/master.cpp 5247377c2e7e92b9843dd4c9d28f92ba679ad742 


Diff: https://reviews.apache.org/r/71008/diff/1/


Testing
---

TODO: Need to write some unit tests.  I'll want to rebase onto the agent 
changes so that there is more detectable stuff in the tests.


Thanks,

Joseph Wu

Re: Review Request 70985: Added `suppressOffers(roles)` to V0 Java bindings.

2019-07-03 Thread Andrei Sekretenko 

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

(Updated July 3, 2019, 5:06 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Made suppressing an empty collection of roles a no-op.


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


Repository: mesos


Description
---

Added `suppressOffers(roles)` to V0 Java bindings.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
f81a69a7f13f976a09f11409a0ea3f40fccea1b8 
  src/java/src/org/apache/mesos/MesosSchedulerDriver.java 
772a7383328519e63f0bf31908cb4c2129a77315 
  src/java/src/org/apache/mesos/SchedulerDriver.java 
155e4ac7b0e507c549a2d2de07ab95e215bf39e0 


Diff: https://reviews.apache.org/r/70985/diff/2/

Changes: https://reviews.apache.org/r/70985/diff/1-2/


Testing
---

Called this method from a modified example framework (WIP).


Thanks,

Andrei Sekretenko

Re: Review Request 70983: Added to the scheduler driver a method to suppress a subset of roles.

2019-07-03 Thread Andrei Sekretenko 

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

(Updated July 3, 2019, 5:05 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Made suppressing an empty vector of roles a no-op.


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


Repository: mesos


Description
---

This patch adds to the scheduler driver a 'suppressOffers(roles)'
method, which sends the SUPPRESS call for these roles and adds them to
the suppressed roles set.


Diffs (updated)
-

  include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4 
  src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516 


Diff: https://reviews.apache.org/r/70983/diff/2/

Changes: https://reviews.apache.org/r/70983/diff/1-2/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70947: Added `reviveOffers(roles)` to V0 Java bindings.

2019-07-03 Thread Andrei Sekretenko 

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

(Updated July 3, 2019, 5:03 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Made reviving an empty vector of roles a no-op.


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


Repository: mesos


Description
---

Added `reviveOffers(roles)` to V0 Java bindings.


Diffs (updated)
-

  src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp 
f81a69a7f13f976a09f11409a0ea3f40fccea1b8 
  src/java/src/org/apache/mesos/MesosSchedulerDriver.java 
772a7383328519e63f0bf31908cb4c2129a77315 
  src/java/src/org/apache/mesos/SchedulerDriver.java 
155e4ac7b0e507c549a2d2de07ab95e215bf39e0 


Diff: https://reviews.apache.org/r/70947/diff/3/

Changes: https://reviews.apache.org/r/70947/diff/2-3/


Testing
---

`./bin/mesos-tests.sh --gtest_filter="ExamplesTest.JavaFramework" 
--gtest_break_on_failure --gtest_repeat=10` with a patch from 
https://reviews.apache.org/r/70946/


Thanks,

Andrei Sekretenko

Re: Review Request 70946: WIP: Added reviveOffers() and the new constructor to Java TestFramework.

2019-07-03 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On June 25, 2019, 4:30 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70946/
> ---
> 
> (Updated June 25, 2019, 4:30 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-9793 and MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9793
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Added reviveOffers() and the new constructor to Java TestFramework.
> 
> 
> Diffs
> -
> 
>   src/examples/java/TestFramework.java 
> c8b0ceacd1305f7ff67f2ef490e14513d2757f5a 
> 
> 
> Diff: https://reviews.apache.org/r/70946/diff/1/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="ExamplesTest.JavaFramework" 
> --gtest_break_on_failure --gtest_repeat=10`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-07-03 Thread Benjamin Mahler 

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


Ship it!




Was the benchmark done with an optimized build?
Can you expand on the performance implications of this change in the 
description? (e.g. compare the min, 25th percentile, median, 75th percentile, 
max of at least 10 runs).


3rdparty/libprocess/include/process/pid.hpp
Lines 204 (patched)


Hm.. hostname or host?


- Benjamin Mahler


On June 26, 2019, 1:37 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 26, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9858
> https://issues.apache.org/jira/browse/MESOS-9858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/4/
> 
> 
> Testing
> ---
> 
> Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
> `--gtest_repeat=10`. There seems to be a slight degradation of performance on 
> average, but the benchmark runtime seems to be dominated by other factors, as 
> indicated by the large variance and presence of outliers on both sides.
> 
> 
> # Master
> 2519083.849973
> 2401163.451671
> 2322734.358626
> 2285763.292828
> 1883268.801707
> 2179436.004374
> 2071825.301714
> 2105588.897964
> 2067228.706673
> 
> 
> # Including UPID changes
> 2305142.571762
> 2194902.761484
> 2139113.533129
> 2024036.049603
> 1765327.993699
> 2172592.534808
> 2053217.096855
> 2103817.232509
> 1480080.596818
> 1808977.015807
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70991: Updated `Socket::connect()` API according to maintainer feedback.

2019-07-03 Thread Benjamin Mahler 

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



Overall approach looks good, thanks for the update!

Some comments below, but I feel comfortable at this point with other reviewers 
looking things over.


3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 26-28 (patched)


Stale comment? Connect requires the tls config, right?



3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 62-63 (patched)


Some guidance on what these are and how one should use them would be 
helpful, probably on the variables here (or if on the types above, perhaps a 
comment here pointing the reader to the above explanations)



3rdparty/libprocess/include/process/ssl/tls_config.hpp
Lines 82 (patched)


`LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME=libprocess` seems a little 
strange, can we call the "old" way `legacy` to signal that it's deprecated and 
we want to remove it?



3rdparty/libprocess/src/http.cpp
Line 1452 (original), 1454-1465 (patched)


How about:

```
  Future connected = [&]() {
switch (scheme) {
  case Scheme::HTTP:
return socket->connect(address);
#ifdef USE_SSL_SOCKET
  case Scheme::HTTPS:
return socket->connect(
address,
network::openssl::create_tls_client_config(peer_hostname));
#endif
}
UNREACHABLE(); // If needed to silence compiler.
  }();
```



3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp
Lines 518 (patched)


Ditto here w.r.t. EXIT



3rdparty/libprocess/src/posix/poll_socket.cpp
Lines 167 (patched)


EXIT will not produce a stack trace, can you use LOG(FATAL) here so that we 
can see who made the incorrect call?

Generally, we don't use EXIT for programming errors for this reason



3rdparty/libprocess/src/process.cpp
Lines 1453 (patched)


s/std:://



3rdparty/libprocess/src/tests/http_tests.cpp
Line 263 (original), 264-276 (patched)


Ditto here, can use the lambda approach?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 68 (patched)


no need for the break anymore?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 73 (patched)


Any comment on why we don't pass a hostname here or if we should? Do these 
tests even exercise the SSL socket? Will the branch be executed?



3rdparty/libprocess/src/tests/socket_tests.cpp
Lines 75 (patched)


ditto here, no need for the break?



3rdparty/libprocess/src/tests/ssl_client.cpp
Line 147 (original), 147-157 (patched)


Ditto here for assignment from result of a lambda?



3rdparty/libprocess/src/tests/ssl_client.cpp
Lines 155 (patched)


Ditto here on a comment about why we don't pass hostname here



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 559 (patched)


For all these spots passing None(), should we explain?

```
  const Future connect = client.connect(
  server_address.get(),
  // E.g. Don't care about hostname certificate validation?
  openssl::create_tls_client_config(None()));
```



3rdparty/libprocess/src/tests/ssl_tests.cpp
Lines 1008-1022 (patched)


Should we also verify the configure/verify error cases surfacing correctly 
as errors?


- Benjamin Mahler


On July 2, 2019, 5:50 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70991/
> ---
> 
> (Updated July 2, 2019, 5:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9878
> https://issues.apache.org/jira/browse/MESOS-9878
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reworked the API of `Socket::connect()` according to the following
> boundary conditions requested by libprocess maintainers:
> 
>  * It shall be possible to use custom connection options when
>connecting with a SSL socket.
>  * When libprocess is

Re: Review Request 70942: Added a test for 'reviveOffers(roles)'.

2019-07-03 Thread Andrei Sekretenko 


> On June 25, 2019, 7:44 p.m., Benjamin Mahler wrote:
> > src/tests/scheduler_driver_tests.cpp
> > Lines 514-516 (patched)
> > 
> >
> > This seems unnecessary?
> 
> Andrei Sekretenko wrote:
> Removed this `settle()`. 
> 
> It does nothing useful except for preventing a potential race between 
> `EXPECT_CALL(resourceOffers, ...)` and the driver calling `resourceOffers()` 
> (which is possible only if the offer filters / framework update are broken).
> And testing offer filters / framework update is not the purpose of this 
> test.

I was mistaken. There is a reason why we need to `settle()` here. It prevents 
reordering of updateFramework() and revive() in the allocator, which is 
possible if master processes REVIVE while UPDATE_FRAMEWORK is being authorized.


- Andrei


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


On July 3, 2019, 3:55 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70942/
> ---
> 
> (Updated July 3, 2019, 3:55 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9849
> https://issues.apache.org/jira/browse/MESOS-9849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for 'reviveOffers(roles)'.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_driver_tests.cpp 
> 80f3335b0054ed0e458c175700765293dfd6a929 
> 
> 
> Diff: https://reviews.apache.org/r/70942/diff/4/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh 
> --gtest_filter="MesosSchedulerDriverTest*ReviveSingleRole*" 
> --gtest_break_on_failure --gtest_repeat=1000`
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70942: Added a test for 'reviveOffers(roles)'.

2019-07-03 Thread Andrei Sekretenko 

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

(Updated July 3, 2019, 3:55 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Reverted `settle()` removal, fixed a comment to explain why it is really needed.


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


Repository: mesos


Description
---

Added a test for 'reviveOffers(roles)'.


Diffs (updated)
-

  src/tests/scheduler_driver_tests.cpp 80f3335b0054ed0e458c175700765293dfd6a929 


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

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


Testing
---

`./bin/mesos-tests.sh 
--gtest_filter="MesosSchedulerDriverTest*ReviveSingleRole*" 
--gtest_break_on_failure --gtest_repeat=1000`


Thanks,

Andrei Sekretenko

Re: Review Request 70941: Added to the scheduler driver a method to revive a subset of roles.

2019-07-03 Thread Andrei Sekretenko 

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

(Updated July 3, 2019, 3:52 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Made reviving an empty vector of roles a no-op.


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


Repository: mesos


Description
---

This patch adds to the scheduler driver a 'reviveOffers(roles)' method,
which sends the REVIVE call for these roles and removes them from the
suppressed roles set.


Diffs (updated)
-

  include/mesos/scheduler.hpp 0a09d5505ed2f9f65897ecce44cffa75db624ef4 
  src/sched/sched.cpp 6b02ac071c9773411954636730c953cca0044516 


Diff: https://reviews.apache.org/r/70941/diff/3/

Changes: https://reviews.apache.org/r/70941/diff/2-3/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70883: Added optional 'peer_hostname' argument to Socket::connect().

2019-07-03 Thread Benjamin Mahler 

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



Can you discard this patch in favor of the agreed upon interface?

This patch looks pretty small outside of the interface changes, so it should be 
easy to re-work into the new approach? We don't want to commit a confusing 
interface only to immediately rework it, so let's just go with the better 
interface.

- Benjamin Mahler


On July 2, 2019, 5:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70883/
> ---
> 
> (Updated July 2, 2019, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Socket::connect() function now takes an optional string
> as an additional argument. This is to prepare support for proper
> TLS hostname validation.
> 
> With TCP, a connection is always made to a specific IP address,
> with the hostname just serving as an artifact to help humans
> remember that address.
> 
> With TLS, the roles are switched: A connection is made to a
> specific hostname (which is recorded in a TLS certificate),
> with the IP address just being a network-layer artifact to
> help packets route to that hostname.
> 
> Therefore, a connecting TLS socket must be aware of the
> hostname it is supposed to connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 029605eaff72e80206cb7dfd64c2f898084155e0 
>   3rdparty/libprocess/include/process/socket.hpp 
> 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
>   3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
>   3rdparty/libprocess/src/poll_socket.hpp 
> 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
> 6ef5a86566af3439cfe0b06ab3576076623f7be0 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 7e2229a9ed815727500bd457356e5531607fa6cf 
>   3rdparty/libprocess/src/posix/poll_socket.cpp 
> 74acb6942682a9d9626df81b303eba0a1c24ecf7 
>   3rdparty/libprocess/src/process.cpp 
> 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
>   3rdparty/libprocess/src/windows/poll_socket.cpp 
> 565b0088dc2b270193e615655f57f48419eb2c12 
> 
> 
> Diff: https://reviews.apache.org/r/70883/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 71004: Added a test `ROOT_DOCKER_AllocationRoleEnvironmentVariable`.

2019-07-03 Thread Qian Zhang 

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

Added a test `ROOT_DOCKER_AllocationRoleEnvironmentVariable`.


Diffs
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
a6217581e20168c5114f733323e927a83ac59fd0 


Diff: https://reviews.apache.org/r/71004/diff/1/


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 71003: Added a test `DefaultExecutorTest.AllocationRoleEnvironmentVariable`.

2019-07-03 Thread Qian Zhang 

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

Added a test `DefaultExecutorTest.AllocationRoleEnvironmentVariable`.


Diffs
-

  src/tests/default_executor_tests.cpp 1c3b48848cf387fe9fb109abfaffa5be04e2a3e1 


Diff: https://reviews.apache.org/r/71003/diff/1/


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 71002: Added a test `CommandExecutorTest.AllocationRoleEnvironmentVariable`.

2019-07-03 Thread Qian Zhang 

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

Added a test `CommandExecutorTest.AllocationRoleEnvironmentVariable`.


Diffs
-

  src/tests/command_executor_tests.cpp 02ae250a599043f30ef5879ce528815e0ded5d3d 


Diff: https://reviews.apache.org/r/71002/diff/1/


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 71005: Used euid to determine subprocess' user when launching tasks.

2019-07-03 Thread fei long 

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

Review request for mesos and Jie Yu.


Bugs: mesos-9876
https://issues.apache.org/jira/browse/mesos-9876


Repository: mesos


Description
---

Used euid to determine subprocess' user when launching tasks.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
b29ec556399a40aa662987a11a2b64e6a16de889 


Diff: https://reviews.apache.org/r/71005/diff/1/


Testing
---

All testing cases passed.


Thanks,

fei long