Re: Review Request 56378: Added test case for suppress and revive with multi role framework.

[Jay Guo]

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



Maybe add a test to check master suppress/revive validation code path too?

- Jay Guo


On Feb. 7, 2017, 6:10 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56378/
> ---
> 
> (Updated Feb. 7, 2017, 6:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for suppress and revive with multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56378/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  
> --gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffersWithMultiRole"
>  --gtest_repeat=100
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 55773: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 8, 2017, 7:55 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/stout/configure.ac cac14577caa9085728f89af9f7511c45ee37f78d 

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


Testing
---

* tested on various Linux configurations internal CI
* tested on OS X Sierra with the bundled compiler+stdlib and versions close to 
their respective HEADs


Thanks,

Benjamin Bannier

Re: Review Request 55772: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 8, 2017, 7:55 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/configure.ac 48e3b14e3a70e8b0dc416c35997e8784d3409772 

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


Testing
---

Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
their HEADs with

$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD

$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
from HEAD


Thanks,

Benjamin Bannier

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 8, 2017, 7:54 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 

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


Testing
---

Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
their HEADs with

$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD

$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
from HEAD


Thanks,

Benjamin Bannier

Re: Review Request 56364: Windows: Stout: Rewrite Job Object wrappers.

[Alex Clemmer]

> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, lines 718-721
> > 
> >
> > This is a good default.  But we need a way to toggle this behavior, 
> > such that the agent's death does not kill child jobs.
> > 
> > i.e. A Windows version of ChildHook::SETSID

I asked Andy to follow up this patch with a different changeset that decouples 
the life of the executor from the life of the agent. Since the agent already 
kills all executors when it dies, I think it makes sense to have one set of 
patches just adding Mesos Containers support to Windows, and one fixing the 
semantics of a dying Agent.

Thoughts?


- Alex


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


On Feb. 7, 2017, 2:31 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56364/
> ---
> 
> (Updated Feb. 7, 2017, 2:31 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::create_job` now returns a `Try` instead of a raw
> `HANDLE`, forcing ownership of the job object handle onto the caller
> of the function. `create_job` requires a `std::string name` for the
> job object, which is mapped from a PID using `os::name_job`.
> 
> The assignment of a process to the job object is now done via
> `Try os::assign_job(SharedHandle, pid_t)`.
> 
> The equivalent of killing a process tree with job object semantics
> is simply to terminate the job object. This is done via
> `os::kill_job(SharedHandle)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> b5172fca96c4151f4b1ebb6d343022558f45fc34 
> 
> Diff: https://reviews.apache.org/r/56364/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56347: Stout: Explicitly delete `SharedHandle` default constructor.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Feb. 6, 2017, 10:56 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56347/
> ---
> 
> (Updated Feb. 6, 2017, 10:56 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Explicitly delete `SharedHandle` default constructor.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> 3bc973b88ea39948780df2ecc7c3692f7e07e1a9 
> 
> Diff: https://reviews.apache.org/r/56347/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56327: Updated Suppress and Revive proto to support per role.

[Jay Guo]

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


Ship it!




Ship It!

- Jay Guo


On Feb. 7, 2017, 10:08 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56327/
> ---
> 
> (Updated Feb. 7, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Suppress and Revive proto to support per role.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 5f4635d523286754a61aa99e18e79d6c1db9463f 
>   include/mesos/v1/scheduler/scheduler.proto 
> 096c76dfffe03c0e2d6abe84d438c396cc1b0be9 
> 
> Diff: https://reviews.apache.org/r/56327/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 55888: Test to ensure non-authorized users cannot launch tasks on agents.

[Anindya Sinha]

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

(Updated Feb. 8, 2017, 3:54 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.


Changes
---

Used `os::user()` instead of `os::getenv("USER")` to get current user.


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


Repository: mesos


Description
---

Test to ensure non-authorized users cannot launch tasks on agents.


Diffs (updated)
-

  src/tests/slave_tests.cpp 7f357a653335b559b5c78d4618926715dc4a63c7 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 56392: Tightened test expecations in reservation-related tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55461, 56391, 55462, 56392]

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 Feb. 7, 2017, 4:43 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56392/
> ---
> 
> (Updated Feb. 7, 2017, 4:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes expected error strings in reservation
> validation-related tests explicit. This is to make sure that the
> observed errors match the expected ones.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2 
> 
> Diff: https://reviews.apache.org/r/56392/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 56427: Added another sorter test case.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

When two clients have the same share, the sorter uses the total number
of allocations made to each client as a tiebreaker.


Diffs
-

  src/tests/sorter_tests.cpp a57a4fa4dbf7a9184e29b5dca4f636fe65a3 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56424: Removed a stale TODO in the agent code.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

This TODO was written before the code below it handled the
optionality of the state `info` fields.


Diffs
-

  src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Till Toenshoff]

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




configure.ac 


Lets keep this one.


- Till Toenshoff


On Feb. 7, 2017, 7:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> ---
> 
> (Updated Feb. 7, 2017, 7:49 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
>   configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef 
>   src/tests/default_executor_tests.cpp 
> 1afab8292c7fc15bd16bee4bcd93ce290a233c9a 
> 
> Diff: https://reviews.apache.org/r/55771/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
> their HEADs with
> 
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD
> 
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
> from HEAD
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 56423: Added CHECKs to the agent to ensure allocation info is set.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

The agent has an invariant that all tasks and executors have an
allocation info. This adds CHECKs to ensure this is the case.


Diffs
-

  src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 56422: Inject resource allocation info in agent for received tasks.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Per MESOS-7077, the agent needs to inject allocation info into
tasks and executors coming from an old master to maintain the
invariant that all tasks and executors on the agent have an
allocation info.


Diffs
-

  src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
  src/tests/slave_tests.cpp 7f357a653335b559b5c78d4618926715dc4a63c7 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Till Toenshoff]

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



Rebase please.

- Till Toenshoff


On Feb. 7, 2017, 7:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> ---
> 
> (Updated Feb. 7, 2017, 7:49 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
>   configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef 
>   src/tests/default_executor_tests.cpp 
> 1afab8292c7fc15bd16bee4bcd93ce290a233c9a 
> 
> Diff: https://reviews.apache.org/r/55771/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
> their HEADs with
> 
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD
> 
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
> from HEAD
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 56421: Added TODOs to make the agent code safer.

[Benjamin Mahler]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

The executor and tasks fields in the agent's structs are publicly
visible, but yet for correctness it is required that callers made
modifications through the member functions which carry additional
logic (e.g. tracking allocated resources). The TODO here is to
make these private and to provide public views that are const.


Diffs
-

  src/slave/slave.hpp 5049eb783b8ad7b9599f20c3701f7d3d654b4491 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 56364: Windows: Stout: Rewrite Job Object wrappers.

[Joseph Wu]

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



I need to check how this is used in the rest of the review chain, but...

Giving the ownership of the HANDLE to the caller may require much larger 
changes in the codebase.  You may notice that we simply leak some pid's in some 
parts of the codebase.  So we have to make sure we aren't leaking these shared 
objects.


3rdparty/stout/include/stout/windows/os.hpp (lines 710 - 713)


This is a good default.  But we need a way to toggle this behavior, such 
that the agent's death does not kill child jobs.

i.e. A Windows version of ChildHook::SETSID



3rdparty/stout/include/stout/windows/os.hpp (line 721)


I believe using `BOOL` as a boolean results in a warning.  We often just 
compare it to `FALSE`.



3rdparty/stout/include/stout/windows/os.hpp (lines 730 - 734)


There's no `name` argument in this function...


- Joseph Wu


On Feb. 6, 2017, 6:31 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56364/
> ---
> 
> (Updated Feb. 6, 2017, 6:31 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `os::create_job` now returns a `Try` instead of a raw
> `HANDLE`, forcing ownership of the job object handle onto the caller
> of the function. `create_job` requires a `std::string name` for the
> job object, which is mapped from a PID using `os::name_job`.
> 
> The assignment of a process to the job object is now done via
> `Try os::assign_job(SharedHandle, pid_t)`.
> 
> The equivalent of killing a process tree with job object semantics
> is simply to terminate the job object. This is done via
> `os::kill_job(SharedHandle)`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> b5172fca96c4151f4b1ebb6d343022558f45fc34 
> 
> Diff: https://reviews.apache.org/r/56364/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56362: Windows: Create the `WindowsLauncher`.

[Joseph Wu]

> On Feb. 7, 2017, 4 a.m., Ilya Pronin wrote:
> > src/slave/containerizer/mesos/windows_launcher.cpp, line 105
> > 
> >
> > Systemd on Windows? ;)
> 
> Andrew Schwartzmeyer wrote:
> This iteration is the copy of the existing implementation. It was 
> refactored into the real `WindowsLauncher` in 
> [#56365](https://reviews.apache.org/r/56365/).

You should consider squashing #56365 into this review.  You end up modifying 
the bulk of this copied code, so the diff-of-a-diff ends up distracting from 
the review.


- Joseph


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


On Feb. 7, 2017, 10:54 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56362/
> ---
> 
> (Updated Feb. 7, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of deriving `WindowsLauncher` from `PosixLauncher`,
> this commit implements a separate `WindowsLauncher` derived
> from `Launcher` parallel to the `PosixLauncher`.
> 
> The purpose of this is to setup for refactoring the `WindowsLauncher`
> into a `Launcher` that uses Windows' "Job Objects,"
> which are similar to Linux's cgroups, and enable us to reason
> about a group of processes. It is necessary to do this in the
> `WindowsLaucher` because the current implementation uses the POSIX
> idea of a process hierarchy to list and kill all processes
> associated with a task. We have found this model to not work on
> Windows, and the solution is to use Job Objects.
> 
> Because this is preparation work, it is a straight copy of the
> current "working" implementation. That is, the `PosixLauncher`
> has been copied to create the `WindowsLauncher`, instead of
> the latter deriving the former.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/launcher.hpp 
> 79f6eea0ee8e564e90b36208672df150dbc5d540 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56362: Windows: Create the `WindowsLauncher`.

[Joseph Wu]

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




src/slave/containerizer/mesos/windows_launcher.hpp (lines 48 - 55)


Definitely update this comment :)



src/slave/containerizer/mesos/windows_launcher.cpp (lines 92 - 98)


Unless there's a good reason, you should just return an Error here if 
either of these arguments are `Some`.



src/slave/containerizer/mesos/windows_launcher.cpp (lines 105 - 106)


We're definitely not on systemd... on Windows :D



src/slave/containerizer/mesos/windows_launcher.cpp (lines 109 - 127)


Why are you removing the CREATE_JOB parent hook 
(https://reviews.apache.org/r/56368/ ) instead of using it here?

We may want to use JobObjects for other subprocesses in general, rather 
than just the launcher.  In which case, having a CREATE_JOB helper would be 
useful.



src/slave/containerizer/mesos/windows_launcher.cpp (line 139)


This is a no-op on Windows.  The intended behavior is to prevent the child 
process from exiting if the parent exits.

On Windows, we may eventually want to introduce the opposite type of hook 
(having the child exit if the parent exits).



src/slave/containerizer/mesos/windows_launcher.cpp (lines 155 - 156)


The reason for this forward declaration is that `_destroy` is not declared 
in the WindowsLauncher header.  

You should just declare it.



src/slave/containerizer/mesos/windows_launcher.cpp (lines 203 - 207)


One newline here.


- Joseph Wu


On Feb. 7, 2017, 10:54 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56362/
> ---
> 
> (Updated Feb. 7, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of deriving `WindowsLauncher` from `PosixLauncher`,
> this commit implements a separate `WindowsLauncher` derived
> from `Launcher` parallel to the `PosixLauncher`.
> 
> The purpose of this is to setup for refactoring the `WindowsLauncher`
> into a `Launcher` that uses Windows' "Job Objects,"
> which are similar to Linux's cgroups, and enable us to reason
> about a group of processes. It is necessary to do this in the
> `WindowsLaucher` because the current implementation uses the POSIX
> idea of a process hierarchy to list and kill all processes
> associated with a task. We have found this model to not work on
> Windows, and the solution is to use Job Objects.
> 
> Because this is preparation work, it is a straight copy of the
> current "working" implementation. That is, the `PosixLauncher`
> has been copied to create the `WindowsLauncher`, instead of
> the latter deriving the former.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/launcher.hpp 
> 79f6eea0ee8e564e90b36208672df150dbc5d540 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 5:40 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Updated code to allow future tests of custom executors to disable the invalid 
environment variables, if needed.


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


Repository: mesos


Description
---

This subtly modifies all tests using the `docker/runtime` isolator
to fail if environment variables from inside the DockerArchive
are passed into the Mesos executor's environment.  This applies
for all executors (default, command, and docker), but mainly
affects the command executor.

The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
either causes linking problems or will force libprocess to exit.


Diffs (updated)
-

  src/tests/containerizer/docker_archive.hpp 
b36dbdba7acf0587e18aced3581f68a1269b04d2 

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


Testing
---

This causes some tests to fail, particularly many of the 
`DockerRuntimeIsolatorTest`:

```
HealthCheckTest.ROOT_HealthyTaskWithContainerImage
ProvisionerDockerPullerTest.ROOT_LocalPullerSimpleCommand
CgroupsIsolatorTest.ROOT_CGROUPS_PERF_NET_CLS_UserCgroup
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutor
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutorWithHostVolumes
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutorPersistentVolume
LinuxFilesystemIsolatorMesosTest.ROOT_RecoverOrphanedPersistentVolume
LinuxFilesystemIsolatorMesosTest.ROOT_SandboxEnvironmentVariable
DockerRuntimeIsolatorTest.ROOT_DockerDefaultCmdLocalPuller
DockerRuntimeIsolatorTest.ROOT_DockerDefaultEntryptLocalPuller
DockerRuntimeIsolatorTest.ROOT_NestedDockerDefaultCmdLocalPuller
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/11 
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/13 
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/15
```


Thanks,

Joseph Wu

Re: Review Request 56212: Added support for general checks to command executor.

[Vinod Kone]

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




src/launcher/executor.cpp (line 327)


can we also add a REASON_TASK_HEALTHCHECK_STATUS_UPDATED for consistency?



src/launcher/executor.cpp (line 453)


there is a lot of code duplication in the call sites of checker and health 
checker and their code itself. I know you had a TODO, but can you ensure there 
is a ticket for tracking the de-duplication of code? we should tackle that 
sooner than latter, hopefully even before 1.3 is cut. otherwise it will be a 
nightmare to maintain both.


- Vinod Kone


On Feb. 2, 2017, 9:57 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> ---
> 
> (Updated Feb. 2, 2017, 9:57 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/56212/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56211: Renamed health checker in command executor for clarity.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 2, 2017, 9:57 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56211/
> ---
> 
> (Updated Feb. 2, 2017, 9:57 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/56211/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56210: Reused previous task status to generate a new one in command executor.

[Vinod Kone]

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




src/launcher/executor.cpp (lines 714 - 753)


I think these helpers are bit confusing. For example, it's not clear to me 
when I should use `update()` vs `bootstrappedUpdate()` when I write new code in 
the future that generates a status update (say TASK_STARTING). The comment on 
top of `update()` seems too specific to health checks for the generic function 
names that you picked here.

Is `update` supposed to be used when `TaskState` changes whereas the 
`bootstrappedUpdate` should be used when it doesn't? I hope not because even if 
TaskState changes you would want to preserve health and check status. Right now 
it kinda works because TASK_RUNNING is the only state that can have different 
health / check statuses, whereas TASK_KILLING or TASK_FINISHED or TASK_FINISHED 
don't need that info.

Alternatively, can we just merge these 2 into one helper that takes a bunch 
of optional fields that can overwrite fields in `lastTaskStatus`? Would that be 
more intuitive?



src/launcher/executor.cpp (line 720)


looks like this review doesn't use `reason` argument, so I wouldn't add it 
in this review. lets add it in the review that needs it.



src/launcher/executor.cpp (line 752)


ditto. no `reason` argument in this review.



src/launcher/executor.cpp (line 775)


s/sendTaskStatusUpdate/forward/


- Vinod Kone


On Feb. 2, 2017, 9:57 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56210/
> ---
> 
> (Updated Feb. 2, 2017, 9:57 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a new task status update is generated in the executor, we have
> to make sure specific data is duplicated from the previous update
> to, e.g., avoid shadowing of those data during reconciliation. For
> instance, consider a check status being sent; in this status update
> we must include the latest known health information.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/56210/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54784: Adds tests for authorization when attaching to a container input.

[Greg Mann]

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


Fix it, then Ship it!




Could you add the relevant JIRA, MESOS-6886? Also, the commit message should be 
in the past tense.


src/tests/api_tests.cpp (line 4313)


Could you include a comment above the test saying explicitly what we are 
testing here?



src/tests/api_tests.cpp (lines 4370 - 4371)


Is this comment still accurate?


- Greg Mann


On Dec. 15, 2016, 3:34 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54784/
> ---
> 
> (Updated Dec. 15, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds tests for authorization when attaching to a container input.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 
> 
> Diff: https://reviews.apache.org/r/54784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 56410: Increased granularity of environment handling in command executor.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 4:58 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Added a TODO for Windows.


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


Repository: mesos


Description
---

This adds a new flag to the command executor `--task_environment`
which allows isolators to specify additionaly environment the task
should be given.  This flag is subject to the same priority as other
environment.  User-specified environment is first, isolator's second,
and agent environment last (i.e. `os::environment` from the executor's
context).

This also changes how the task itself receives these environment
variables.  The task is now given the environment explicitly, rather
than inheriting the executor's environment.  This will help prevent
isolators from accidently breaking the command executor or health
checks with task-specific environment variables.


Diffs (updated)
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/launcher/posix/executor.hpp d057ff6211a77e681ee80599d2ec5851a2fa6785 
  src/launcher/posix/executor.cpp 4bc0b0f1a09bf1579ff9612f812f09a3061883ce 

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


Testing
---

Testing later in the chain.


Thanks,

Joseph Wu

Re: Review Request 56350: Const-ified several methods in the sorter interface.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 7, 2017, 2:58 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56350/
> ---
> 
> (Updated Feb. 7, 2017, 2:58 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Const-ified several methods in the sorter interface.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 5bed53a1641fee0c09862f77b394a0e9ec131990 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
>   src/master/allocator/sorter/sorter.hpp 
> 737b13f66f989ac23c9a2fb6467a51f84339c773 
> 
> Diff: https://reviews.apache.org/r/56350/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Note that this changes the sorter interface. I can add a note to the 
> `CHANGELOG` if warranted.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56351: Renamed a private member function in DRFSorter.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 6, 2017, 3:14 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56351/
> ---
> 
> (Updated Feb. 6, 2017, 3:14 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `update(const string&)` to `updateShare(const string&)`. The
> sorter interface already includes two different member functions named
> `update` that do different things, so being a bit more explicit seems
> helpful.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 5bed53a1641fee0c09862f77b394a0e9ec131990 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
> 
> Diff: https://reviews.apache.org/r/56351/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56349: Clarified comments in sorter.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 6, 2017, 3:13 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56349/
> ---
> 
> (Updated Feb. 6, 2017, 3:13 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified comments in sorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 5bed53a1641fee0c09862f77b394a0e9ec131990 
> 
> Diff: https://reviews.apache.org/r/56349/diff/
> 
> 
> Testing
> ---
> 
> `make check` (no functional changes).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 4:47 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Added the list of tests that break due to this review.


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


Repository: mesos


Description
---

This subtly modifies all tests using the `docker/runtime` isolator
to fail if environment variables from inside the DockerArchive
are passed into the Mesos executor's environment.  This applies
for all executors (default, command, and docker), but mainly
affects the command executor.

The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
either causes linking problems or will force libprocess to exit.


Diffs
-

  src/tests/containerizer/docker_archive.hpp 
b36dbdba7acf0587e18aced3581f68a1269b04d2 

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


Testing (updated)
---

This causes some tests to fail, particularly many of the 
`DockerRuntimeIsolatorTest`:

```
HealthCheckTest.ROOT_HealthyTaskWithContainerImage
ProvisionerDockerPullerTest.ROOT_LocalPullerSimpleCommand
CgroupsIsolatorTest.ROOT_CGROUPS_PERF_NET_CLS_UserCgroup
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutor
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutorWithHostVolumes
LinuxFilesystemIsolatorMesosTest.ROOT_ChangeRootFilesystemCommandExecutorPersistentVolume
LinuxFilesystemIsolatorMesosTest.ROOT_RecoverOrphanedPersistentVolume
LinuxFilesystemIsolatorMesosTest.ROOT_SandboxEnvironmentVariable
DockerRuntimeIsolatorTest.ROOT_DockerDefaultCmdLocalPuller
DockerRuntimeIsolatorTest.ROOT_DockerDefaultEntryptLocalPuller
DockerRuntimeIsolatorTest.ROOT_NestedDockerDefaultCmdLocalPuller
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/11 
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/13 
TestParam/LinuxCapabilitiesIsolatorTest.ROOT_Ping/15
```


Thanks,

Joseph Wu

Re: Review Request 56410: Increased granularity of environment handling in command executor.

[Joseph Wu]

> On Feb. 7, 2017, 3:56 p.m., Jie Yu wrote:
> > Ship It!

Adding a TODO for the Windows portion before I commit (as we discussed offline).


- Joseph


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


On Feb. 7, 2017, 2:18 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56410/
> ---
> 
> (Updated Feb. 7, 2017, 2:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new flag to the command executor `--task_environment`
> which allows isolators to specify additionaly environment the task
> should be given.  This flag is subject to the same priority as other
> environment.  User-specified environment is first, isolator's second,
> and agent environment last (i.e. `os::environment` from the executor's
> context).
> 
> This also changes how the task itself receives these environment
> variables.  The task is now given the environment explicitly, rather
> than inheriting the executor's environment.  This will help prevent
> isolators from accidently breaking the command executor or health
> checks with task-specific environment variables.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/launcher/posix/executor.hpp d057ff6211a77e681ee80599d2ec5851a2fa6785 
>   src/launcher/posix/executor.cpp 4bc0b0f1a09bf1579ff9612f812f09a3061883ce 
> 
> Diff: https://reviews.apache.org/r/56410/diff/
> 
> 
> Testing
> ---
> 
> Testing later in the chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56247: Switched to implicit roles in master quota tests.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 2, 2017, 2:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56247/
> ---
> 
> (Updated Feb. 2, 2017, 2:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using an explicit role list is deprecated; this change will also make it
> easier to write test cases for hierarchical roles.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 
> 
> Diff: https://reviews.apache.org/r/56247/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Joseph Wu]

> On Feb. 7, 2017, 3:54 p.m., Jie Yu wrote:
> > src/tests/containerizer/docker_archive.hpp, line 55
> > 
> >
> > Let's add another parameter here:
> > ```
> > static Future create(
> >   const std::string& directory,
> >   const std::string& name,
> >   const std::string& entrypoint = "null",
> >   const std::string& cmd = "null",
> >   const map& environment = {
> > {"LD_LIBRARY_PATH", "invalid"},
> > {"LIBPROCESS_IP", "invalid"},
> > {"LIBPROCESS_PORT", "invalid"}});
> > ```
> > 
> > (not sure if the above compile or not :)

To reduce the amount of parsing/transformation that needs to be done, I'm going 
to add a vector instead, as the image takes an array of environment rather than 
a map:
```
  const std::vector& environment = {
{"LD_LIBRARY_PATH=invalid"},
{"LIBPROCESS_IP=invalid"},
{"LIBPROCESS_PORT=invalid"}}
```


- Joseph


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


On Feb. 6, 2017, 2:04 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56339/
> ---
> 
> (Updated Feb. 6, 2017, 2:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This subtly modifies all tests using the `docker/runtime` isolator
> to fail if environment variables from inside the DockerArchive
> are passed into the Mesos executor's environment.  This applies
> for all executors (default, command, and docker), but mainly
> affects the command executor.
> 
> The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
> and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
> either causes linking problems or will force libprocess to exit.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_archive.hpp 
> b36dbdba7acf0587e18aced3581f68a1269b04d2 
> 
> Diff: https://reviews.apache.org/r/56339/diff/
> 
> 
> Testing
> ---
> 
> This causes some tests to fail, particularly many of the 
> `DockerRuntimeIsolatorTest` in `runtime_isolator_tests.cpp`.
> 
> A variety of other tests which use `DockerArchive` will also fail.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 54783: Added tests for authorization of launching container sessions.

[Greg Mann]

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


Fix it, then Ship it!




Could you add the relevant JIRA, MESOS-6886?


src/tests/api_tests.cpp (line 3864)


Could you include a comment before this test case which says explicitly 
what we're testing here?



src/tests/api_tests.cpp (lines 3885 - 3886)


Indent just two spaces:
```
  mesos::ACL::LaunchNestedContainerSessionUnderParentWithUser* acl =
flags.acls.get()
  .add_launch_nested_container_sessions_under_parent_with_user();
```



src/tests/api_tests.cpp (lines 3922 - 3923)


Update this comment?


- Greg Mann


On Dec. 15, 2016, 3:33 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54783/
> ---
> 
> (Updated Dec. 15, 2016, 3:33 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a unit test which checks authorization when launching
> a nested container session.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 
> 
> Diff: https://reviews.apache.org/r/54783/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Alexander Rukletsov]

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.hpp, line 137
> > 
> >
> > we don't typically do `const` for POD types.
> > 
> > s/_agentSpawnsCommandContainer/
> 
> Gastón Kleiman wrote:
> Looks lke the regex was truncated ;-).
> 
> Vinod Kone wrote:
> How about "localCommandCheck" (vs "remoteCommandCheck")?
> 
> Alexander Rukletsov wrote:
> We are not consistent about `const` that's true, but I think we are 
> starting to use it more and more often. Unfortunately, I don't have extensive 
> data at hand, here are some links:
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/slave.hpp#L970
> 
> https://github.com/apache/mesos/blob/30f7f3e1965885406d6d3e49348bee7e7d0df9d5/src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp#L55
> 
> Vinod Kone wrote:
> Making a POD member variable const is different from making it a const in 
> function argument. We definitely do the former but not the latter IIRC.

Agree.


- Alexander


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56410: Increased granularity of environment handling in command executor.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 7, 2017, 10:18 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56410/
> ---
> 
> (Updated Feb. 7, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new flag to the command executor `--task_environment`
> which allows isolators to specify additionaly environment the task
> should be given.  This flag is subject to the same priority as other
> environment.  User-specified environment is first, isolator's second,
> and agent environment last (i.e. `os::environment` from the executor's
> context).
> 
> This also changes how the task itself receives these environment
> variables.  The task is now given the environment explicitly, rather
> than inheriting the executor's environment.  This will help prevent
> isolators from accidently breaking the command executor or health
> checks with task-specific environment variables.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/launcher/posix/executor.hpp d057ff6211a77e681ee80599d2ec5851a2fa6785 
>   src/launcher/posix/executor.cpp 4bc0b0f1a09bf1579ff9612f812f09a3061883ce 
> 
> Diff: https://reviews.apache.org/r/56410/diff/
> 
> 
> Testing
> ---
> 
> Testing later in the chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/docker_archive.hpp (line 55)


Let's add another parameter here:
```
static Future create(
  const std::string& directory,
  const std::string& name,
  const std::string& entrypoint = "null",
  const std::string& cmd = "null",
  const map& environment = {
{"LD_LIBRARY_PATH", "invalid"},
{"LIBPROCESS_IP", "invalid"},
{"LIBPROCESS_PORT", "invalid"}});
```

(not sure if the above compile or not :)


- Jie Yu


On Feb. 6, 2017, 10:04 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56339/
> ---
> 
> (Updated Feb. 6, 2017, 10:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This subtly modifies all tests using the `docker/runtime` isolator
> to fail if environment variables from inside the DockerArchive
> are passed into the Mesos executor's environment.  This applies
> for all executors (default, command, and docker), but mainly
> affects the command executor.
> 
> The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
> and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
> either causes linking problems or will force libprocess to exit.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_archive.hpp 
> b36dbdba7acf0587e18aced3581f68a1269b04d2 
> 
> Diff: https://reviews.apache.org/r/56339/diff/
> 
> 
> Testing
> ---
> 
> This causes some tests to fail, particularly many of the 
> `DockerRuntimeIsolatorTest` in `runtime_isolator_tests.cpp`.
> 
> A variety of other tests which use `DockerArchive` will also fail.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.
> 
> Vinod Kone wrote:
> The message returned by `http::connect` should be good enough? Do we use 
> this pattern elsewhere esp. with connect? Most uses of repair I have seen in 
> the code base, transform the future not really to add extra logging 
> information.
> 
> Alexander Rukletsov wrote:
> We use `.repair` for adjusting the message or changing the error type, 
> which is technically the same. Here are some examples from the code:
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/3rdparty/libprocess/src/http.cpp#L1766-L1769
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/master/http.cpp#L4695-L4697
> 
> https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/slave/containerizer/mesos/io/switchboard.cpp#L1632-L1638
> 
> Now, the question is whether connection failure message is good enough? 
> Gastón, could you trigger a failure path and check what error message will be 
> returned?

This is how it looks like with the repair:

```
W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 times 
consecutively: COMMAND health check failed: Unable to establish connection with 
the agent: Failed to connect to 192.99.40.208:31338: Connection refused
```

Without the repair it would look like this:

```
W0207 10:42:19.659122  9361 health_checker.cpp:314] Health check failed 1 times 
consecutively: COMMAND health check failed: Failed to connect to 
192.99.40.208:31338: Connection refused
```


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 618-624
> > 
> >
> > 1) You have no interest in both `future` and `status`. Omit the names 
> > for clarity, please.
> > 
> > 2) Why do we need to call `waitForNestedContainer` here instead of 
> > simply returning `failure`?

Added a comment clarifying this.


> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 653
> > 
> >
> > Don't you need to check that `wait_nested_container().exit_status()` 
> > exists? If `exit_status` is not set in the proto, don't you have to check 
> > this explicitly and return `None`?

Very good catch, thanks!


> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 585-588
> > 
> >
> > Mention `taskId`?

Updated all the comments I could find.


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated Feb. 7, 2017, 11:37 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Added a comment in the new test.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

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


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > 
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> > 
> > This looks like a utility function not really related to the health 
> > checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?
> 
> Gastón Kleiman wrote:
> I renamed the function. Vinod suggested to make it `inline` what's wrong 
> with that?
> 
> This function as-is is specific to how we perform requests in this file 
> (`keep_alive = false`, protobuf serialization). Since it is used only in two 
> places, I'm tempted to removed it and put the code inline in the 
> corresponding methods.

I removed these functions.


- Gastón


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


On Feb. 7, 2017, 11:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 7, 2017, 11:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56409: Enabled flag parsing of the 'Environment' protobuf.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 7, 2017, 10:16 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56409/
> ---
> 
> (Updated Feb. 7, 2017, 10:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds 'mesos::Environment' to the template specializations
> required for stout flags to parse the protobuf directly.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 632a7e53b1604a6234ed231eaedf1efb2eafde5f 
>   src/common/parse.hpp 1dca9ba4ed4ff2163193ab6432b5b017d2be787e 
>   src/common/type_utils.cpp 516d3094ef50aa368ec61c2c784f38b393c7e24a 
> 
> Diff: https://reviews.apache.org/r/56409/diff/
> 
> 
> Testing
> ---
> 
> Testing later in the chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated Feb. 7, 2017, 11:32 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Addressed Alex's comments.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 

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


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 56341: Updated AppC & Docker runtime isolators' handling of Environment.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 7, 2017, 10:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> ---
> 
> (Updated Feb. 7, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit builds upon the command executor's new `--task_environment`
> flag, which allows isolators to specify environment variables meant
> for the task, without affecting the executor's environment.
> 
> This is important as the command executor is both an executor and
> a task.  Some environment variables from isolators are intended
> for the executor, while others are intended for the task (such as
> from the two runtime isolators).
> 
> For example, a container image may provide an environment variable
> like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
> executor expects to find libraries in the host's environment.  If the
> image's environment end up in the command executor at launch time,
> the command executor may simply fail to launch.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> 9bc3fd8309435846c17944e74f611212069dbd76 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2d816e512c95ed2922c9578ba796908c5ce23da4 
> 
> Diff: https://reviews.apache.org/r/56341/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*"
> 
> All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
> chain) are now passing.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56378: Added test case for suppress and revive with multi role framework.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56327, 56328, 56330, 56371, 56373, 56374, 56376, 56378]

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 Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56378/
> ---
> 
> (Updated Feb. 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for suppress and revive with multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56378/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  
> --gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffersWithMultiRole"
>  --gtest_repeat=100
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 6, 2017, 9:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> ---
> 
> (Updated Feb. 6, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
> https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 56d6791baa64189523df668749f4a7ab67d6b363 
> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 56350: Const-ified several methods in the sorter interface.

[Neil Conway]

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

(Updated Feb. 7, 2017, 10:58 p.m.)


Review request for mesos and Michael Park.


Changes
---

Tweak comments.


Repository: mesos


Description
---

Const-ified several methods in the sorter interface.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
5bed53a1641fee0c09862f77b394a0e9ec131990 
  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
  src/master/allocator/sorter/sorter.hpp 
737b13f66f989ac23c9a2fb6467a51f84339c773 

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


Testing
---

`make check`

Note that this changes the sorter interface. I can add a note to the 
`CHANGELOG` if warranted.


Thanks,

Neil Conway

Re: Review Request 56350: Const-ified several methods in the sorter interface.

[Neil Conway]

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

(Updated Feb. 7, 2017, 10:39 p.m.)


Review request for mesos and Michael Park.


Changes
---

Fix typo in comment.


Repository: mesos


Description
---

Const-ified several methods in the sorter interface.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
5bed53a1641fee0c09862f77b394a0e9ec131990 
  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
  src/master/allocator/sorter/sorter.hpp 
737b13f66f989ac23c9a2fb6467a51f84339c773 

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


Testing
---

`make check`

Note that this changes the sorter interface. I can add a note to the 
`CHANGELOG` if warranted.


Thanks,

Neil Conway

Re: Review Request 56408: Fixed a regression in the execute CLI command.

[Anand Mazumdar]

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


Ship it!




Can you update the review description/testing done accordingly after the recent 
update?

- Anand Mazumdar


On Feb. 7, 2017, 10:28 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56408/
> ---
> 
> (Updated Feb. 7, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Gastón Kleiman.
> 
> 
> Bugs: MESOS-7075
> https://issues.apache.org/jira/browse/MESOS-7075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of `Resource.allocation_info` to support MULTI_ROLE
> frameworks, the execute helper has pulled in the updated `Resources`
> library and needs to be updated to be aware of allocation info.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 50969145ae02f90d324078d94a8ab09d2191cce0 
> 
> Diff: https://reviews.apache.org/r/56408/diff/
> 
> 
> Testing
> ---
> 
> Manually tested against a new master.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56408: Fixed a regression in the execute CLI command.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Feb. 7, 2017, 10:28 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56408/
> ---
> 
> (Updated Feb. 7, 2017, 10:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Gastón Kleiman.
> 
> 
> Bugs: MESOS-7075
> https://issues.apache.org/jira/browse/MESOS-7075
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of `Resource.allocation_info` to support MULTI_ROLE
> frameworks, the execute helper has pulled in the updated `Resources`
> library and needs to be updated to be aware of allocation info.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 50969145ae02f90d324078d94a8ab09d2191cce0 
> 
> Diff: https://reviews.apache.org/r/56408/diff/
> 
> 
> Testing
> ---
> 
> Manually tested against a new master.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56408: Fixed a regression in the execute CLI command.

[Benjamin Mahler]

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

(Updated Feb. 7, 2017, 10:28 p.m.)


Review request for mesos, Anand Mazumdar and Gastón Kleiman.


Changes
---

Updated to work against both old and new masters.


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


Repository: mesos


Description
---

With the addition of `Resource.allocation_info` to support MULTI_ROLE
frameworks, the execute helper has pulled in the updated `Resources`
library and needs to be updated to be aware of allocation info.


Diffs (updated)
-

  src/cli/execute.cpp 50969145ae02f90d324078d94a8ab09d2191cce0 

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


Testing
---

Manually tested against a new master.


Thanks,

Benjamin Mahler

Re: Review Request 56341: Updated AppC & Docker runtime isolators' handling of Environment.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 2:26 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Oops, forgot to update the summary/description.


Summary (updated)
-

Updated AppC & Docker runtime isolators' handling of Environment.


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


Repository: mesos


Description (updated)
---

This commit builds upon the command executor's new `--task_environment`
flag, which allows isolators to specify environment variables meant
for the task, without affecting the executor's environment.

This is important as the command executor is both an executor and
a task.  Some environment variables from isolators are intended
for the executor, while others are intended for the task (such as
from the two runtime isolators).

For example, a container image may provide an environment variable
like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
executor expects to find libraries in the host's environment.  If the
image's environment end up in the command executor at launch time,
the command executor may simply fail to launch.


Diffs
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
9bc3fd8309435846c17944e74f611212069dbd76 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
2d816e512c95ed2922c9578ba796908c5ce23da4 

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


Testing
---

make check

sudo src/mesos-tests --gtest_filter="*ROOT*"

All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
chain) are now passing.


Thanks,

Joseph Wu

Review Request 56410: Increased granularity of environment handling in command executor.

[Joseph Wu]

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

Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This adds a new flag to the command executor `--task_environment`
which allows isolators to specify additionaly environment the task
should be given.  This flag is subject to the same priority as other
environment.  User-specified environment is first, isolator's second,
and agent environment last (i.e. `os::environment` from the executor's
context).

This also changes how the task itself receives these environment
variables.  The task is now given the environment explicitly, rather
than inheriting the executor's environment.  This will help prevent
isolators from accidently breaking the command executor or health
checks with task-specific environment variables.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/launcher/posix/executor.hpp d057ff6211a77e681ee80599d2ec5851a2fa6785 
  src/launcher/posix/executor.cpp 4bc0b0f1a09bf1579ff9612f812f09a3061883ce 

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


Testing
---

Testing later in the chain.


Thanks,

Joseph Wu

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

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

(Updated Feb. 7, 2017, 2:17 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Moved the command executor changes into another review, to keep those logically 
separate.  See https://reviews.apache.org/r/56410/


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


Repository: mesos


Description
---

This commit adds more special-casing in the `docker/runtime` isolator
for the command executor.  The command executor will generally break
when the `docker/runtime` isolator provides environment variables
directly to the executor.  This is because the environment variables
are provided in the context of the container image, rather than the
host.

For example, a container image may provide an environment variable
like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
executor expects to find libraries in the host's environment.  If the
image's environment end up in the command executor at launch time, 
the command executor may simply fail to launch.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
9bc3fd8309435846c17944e74f611212069dbd76 
  src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
2d816e512c95ed2922c9578ba796908c5ce23da4 

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


Testing
---

make check

sudo src/mesos-tests --gtest_filter="*ROOT*"

All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
chain) are now passing.


Thanks,

Joseph Wu

Review Request 56409: Enabled flag parsing of the 'Environment' protobuf.

[Joseph Wu]

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

Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This adds 'mesos::Environment' to the template specializations
required for stout flags to parse the protobuf directly.


Diffs
-

  include/mesos/type_utils.hpp 632a7e53b1604a6234ed231eaedf1efb2eafde5f 
  src/common/parse.hpp 1dca9ba4ed4ff2163193ab6432b5b017d2be787e 
  src/common/type_utils.cpp 516d3094ef50aa368ec61c2c784f38b393c7e24a 

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


Testing
---

Testing later in the chain.


Thanks,

Joseph Wu

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Joseph Wu]

> On Feb. 7, 2017, 11:17 a.m., Jie Yu wrote:
> > src/launcher/executor.cpp, line 939
> > 
> >
> > I'd do that in `launch`.
> > 
> > I'll also add a TODO. Ideally, we should use execvpe, rather than 
> > relying on inheritence.
> > 
> > Are you sure it won't affect command health check if you do that in 
> > this way?

Health checks will fail too (particularly the type that spawns a 
`mesos-tcp-connect` helper), unless we do the execvpe solution.  So I should 
update this code path in its entirety.

Side note: in CMake, we plan to make `mesos-tcp-connect` a static binary.  So 
it would not be affected by the environment.


- Joseph


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


On Feb. 6, 2017, 5:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> ---
> 
> (Updated Feb. 6, 2017, 5:19 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds more special-casing in the `docker/runtime` isolator
> for the command executor.  The command executor will generally break
> when the `docker/runtime` isolator provides environment variables
> directly to the executor.  This is because the environment variables
> are provided in the context of the container image, rather than the
> host.
> 
> For example, a container image may provide an environment variable
> like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
> executor expects to find libraries in the host's environment.  If the
> image's environment end up in the command executor at launch time, 
> the command executor may simply fail to launch.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2d816e512c95ed2922c9578ba796908c5ce23da4 
> 
> Diff: https://reviews.apache.org/r/56341/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*"
> 
> All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
> chain) are now passing.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 56408: Fixed a regression in the execute CLI command.

[Benjamin Mahler]

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

Review request for mesos, Anand Mazumdar and Gastón Kleiman.


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


Repository: mesos


Description
---

With the addition of `Resource.allocation_info` to support MULTI_ROLE
frameworks, the execute helper has pulled in the updated `Resources`
library and needs to be updated to be aware of allocation info.


Diffs
-

  src/cli/execute.cpp 50969145ae02f90d324078d94a8ab09d2191cce0 

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


Testing
---

Manually tested against a new master.


Thanks,

Benjamin Mahler

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Ilya Pronin]

> On Feb. 7, 2017, 6:57 p.m., Gilbert Song wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, lines 241-242
> > 
> >
> > Maybe you should fix the comment.

Yes, the comment should be changed.


> On Feb. 7, 2017, 6:57 p.m., Gilbert Song wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, lines 268-271
> > 
> >
> > Since you set `cached` as false and manually remove one layer from the 
> > store. This test only verifies that a missing layer will be pulled if 
> > `cached` is set as false.
> > 
> > I do not see how it verifies the local puller will skip pullin the 
> > existing layers.

I agree that this test does not explicitly verify that `LocalPuller` skips the 
layer. My first intention was to write 2 separate tests: for `LocalPuller` and 
for `Store`. But such tests assumed a lot of details about staging directory 
layout. So Jie suggested this test that is simpler and less fragile and 
exercises the skipping layers code.


- Ilya


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


On Feb. 6, 2017, 8:40 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 6, 2017, 8:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that that local Docker puller and store will skip 
> layers that are already pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56174: Added skipping already stored layers to local Docker puller.

[Ilya Pronin]

> On Feb. 5, 2017, 1:28 a.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_docker_tests.cpp, lines 262-270
> > 
> >
> > Instead of testing local puller directly, I'd suggest we test using 
> > 'Store'. This test assumes too many impl details which will be very fragile 
> > as code evolves. We try to avoid that if possible.
> > 
> > My suggestion is that we pull the image once, making sure store has it. 
> > And then, delete one layer, and then pull again. Make sure the second pull 
> > is successful.

Done in [56284](https://reviews.apache.org/r/56284/).


- Ilya


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


On Feb. 3, 2017, 6:07 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56174/
> ---
> 
> (Updated Feb. 3, 2017, 6:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a layer is already in the store, there's no need to extract it. 
> `RegistryPuller` already does this and `Store` is ready for such puller 
> behaviour.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> ee391af898886bff9e5b911697f725c5ea53ebd8 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> af9987f88205d00d091f35fa734d5667506aaffd 
> 
> Diff: https://reviews.apache.org/r/56174/diff/
> 
> 
> Testing
> ---
> 
> Added a test verifying that local puller will skip layes that are already in 
> the store. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 55710: Added agent capabilities to /state endpoint of master.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55710]

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 Feb. 7, 2017, 9:59 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55710/
> ---
> 
> (Updated Feb. 7, 2017, 9:59 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-6902
> https://issues.apache.org/jira/browse/MESOS-6902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master should be able to reflect agent capabilities via  `/state`(v0)
> and `getState`(v1) endpoints.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 
>   include/mesos/v1/master/master.proto 
> cfdca7400d98233c6320eebd8784e4f51c43ebbd 
>   src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a 
>   src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
>   src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 
> 
> Diff: https://reviews.apache.org/r/55710/diff/
> 
> 
> Testing
> ---
> 
> make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
> [   OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
> [--] 1 test from MasterTest (94 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (122 ms total)
> 
> make check on Ubuntu 14.04
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Joseph Wu]

> On Feb. 7, 2017, 10:06 a.m., Jie Yu wrote:
> > src/tests/containerizer/docker_archive.hpp, lines 101-105
> > 
> >
> > I think this only applies to 'Task'. If the image is used by the 
> > executor, then this will certainly break them?

Yeah, the intent of the extra environment variables is to break executors (as 
stated by the comment).

For tasks (usually shell commands), these environment variables have little or 
no effect.


- Joseph


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


On Feb. 6, 2017, 2:04 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56339/
> ---
> 
> (Updated Feb. 6, 2017, 2:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This subtly modifies all tests using the `docker/runtime` isolator
> to fail if environment variables from inside the DockerArchive
> are passed into the Mesos executor's environment.  This applies
> for all executors (default, command, and docker), but mainly
> affects the command executor.
> 
> The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
> and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
> either causes linking problems or will force libprocess to exit.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_archive.hpp 
> b36dbdba7acf0587e18aced3581f68a1269b04d2 
> 
> Diff: https://reviews.apache.org/r/56339/diff/
> 
> 
> Testing
> ---
> 
> This causes some tests to fail, particularly many of the 
> `DockerRuntimeIsolatorTest` in `runtime_isolator_tests.cpp`.
> 
> A variety of other tests which use `DockerArchive` will also fail.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

> On Feb. 7, 2017, 6:32 p.m., Till Toenshoff wrote:
> > configure.ac, line 1892
> > 
> >
> > Sorry for not noticing ealier but I believe the idiomatic autotools way 
> > here is to use a pair of `AC_MSG_CHECKING([string])`, 
> > `AC_MSG_RESULT([string])`. We could then have the failure message follow 
> > that result; something along these lines, I guess
> > 
> > ```
> > AC_MSG_CHECKING([C++ standard library has undefined behaviour with 
> > selected optimization level])
> > AC_LANG_PUSH([C++])
> > AC_RUN_IFELSE([
> >   AC_LANG_SOURCE([[
> >   [...]
> >   ]])],
> >   [message='no'], [message='yes']
> > )
> > AC_MSG_RESULT([$message])
> > 
> > if test "x$message" = "xyes"; then
> >   AC_MSG_ERROR([Mesos cannot be built with optimizations against this 
> > version of libcxx (MESOS-5745).
> >   Consider building without optimizations, or changing 
> > the used C++ standard library.])
> > fi
> > ```
> > 
> > ...which would look like this;
> > 
> > ```
> > Checking C++ standard library has undefined behaviour with selected 
> > optimization level... no
> > ```

Very good point. I updated the code to use `AC_MSG_CHECKING`/`AC_MSG_RESULT`, 
but did not introduce additional branching (via raw shell). Fixed here and in 
following patches.


- Benjamin


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


On Feb. 7, 2017, 8:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> ---
> 
> (Updated Feb. 7, 2017, 8:49 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
>   configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 
>   src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
>   src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef 
>   src/tests/default_executor_tests.cpp 
> 1afab8292c7fc15bd16bee4bcd93ce290a233c9a 
> 
> Diff: https://reviews.apache.org/r/55771/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
> their HEADs with
> 
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD
> 
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
> from HEAD
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55773: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 8:49 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Used idiomatic way to output progress.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/stout/configure.ac cac14577caa9085728f89af9f7511c45ee37f78d 

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


Testing
---

* tested on various Linux configurations internal CI
* tested on OS X Sierra with the bundled compiler+stdlib and versions close to 
their respective HEADs


Thanks,

Benjamin Bannier

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 8:49 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Used idiomatic way to output progress.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  CHANGELOG 1478ad4dd3b20fc768fd679f81d803acf0995ad2 
  configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 
  src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668 
  src/oci/spec.cpp 0a88edb404836205f11e61ba3edb771df8b618ef 
  src/tests/default_executor_tests.cpp 1afab8292c7fc15bd16bee4bcd93ce290a233c9a 

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


Testing
---

Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
their HEADs with

$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD

$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
from HEAD


Thanks,

Benjamin Bannier

Re: Review Request 55772: Rejected optimizing if we are working with a libcxx with UB.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 8:49 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Used idiomatic way to output progress.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/configure.ac 48e3b14e3a70e8b0dc416c35997e8784d3409772 

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


Testing
---

Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
their HEADs with

$ autoreconf -fi && ./configure CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD

$ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
$ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
from HEAD


Thanks,

Benjamin Bannier

Re: Review Request 56341: Changed docker/runtime isolator's handling of Environment.

[Jie Yu]

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




src/launcher/executor.cpp (line 860)


Similar to capabilities, can we add a parse function for Environment and 
use `Option` here.



src/launcher/executor.cpp (line 928)


Let's avoid the `mesos::` prefix here by using a `using` clause at the 
begining of the file.



src/launcher/executor.cpp (lines 937 - 940)


Let's just pass `environment` to `CommandExecutor` below.



src/launcher/executor.cpp (line 939)


I'd do that in `launch`.

I'll also add a TODO. Ideally, we should use execvpe, rather than relying 
on inheritence.

Are you sure it won't affect command health check if you do that in this 
way?



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


Let's update AppcRuntimeIsolator as well.


- Jie Yu


On Feb. 7, 2017, 1:19 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56341/
> ---
> 
> (Updated Feb. 7, 2017, 1:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds more special-casing in the `docker/runtime` isolator
> for the command executor.  The command executor will generally break
> when the `docker/runtime` isolator provides environment variables
> directly to the executor.  This is because the environment variables
> are provided in the context of the container image, rather than the
> host.
> 
> For example, a container image may provide an environment variable
> like `LD_LIBRARY_PATH=/image/specific/location`, whereas the default
> executor expects to find libraries in the host's environment.  If the
> image's environment end up in the command executor at launch time, 
> the command executor may simply fail to launch.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 2d816e512c95ed2922c9578ba796908c5ce23da4 
> 
> Diff: https://reviews.apache.org/r/56341/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo src/mesos-tests --gtest_filter="*ROOT*"
> 
> All the tests broken by https://reviews.apache.org/r/56339/ (earlier in the 
> chain) are now passing.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56284: Added ProvisionerDockerLocalStoreTest.MissingLayer test.

[Gilbert Song]

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




src/tests/containerizer/provisioner_docker_tests.cpp (lines 241 - 242)


Maybe you should fix the comment.



src/tests/containerizer/provisioner_docker_tests.cpp (lines 268 - 271)


Since you set `cached` as false and manually remove one layer from the 
store. This test only verifies that a missing layer will be pulled if `cached` 
is set as false.

I do not see how it verifies the local puller will skip pullin the existing 
layers.


- Gilbert Song


On Feb. 6, 2017, 12:40 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 6, 2017, 12:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new test verifies that that local Docker puller and store will skip 
> layers that are already pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56368: Libprocess: Windows: Remove `CREATE_JOB` parent hook.

[Andrew Schwartzmeyer]

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

(Updated Feb. 7, 2017, 6:56 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Remove declaration of `CREATE_JOB` too.


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


Repository: mesos


Description (updated)
---

Libprocess: Windows: Remove `CREATE_JOB` parent hook.


Diffs (updated)
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
7117a2730ae8666d0b55cbe05162a0b4e843193c 
  3rdparty/libprocess/src/subprocess.cpp 
bc1b99b8f84332f42623c35fc2b8d5186ba8af70 

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


Testing
---

This is the last patch in a series. Applied all together, they were tested with 
`make && make check` on Linux, and `support\windows-build.bat` on Windows. All 
tests pass (minus flaky ones which sometimes fail but that's unrelated).


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56368: Libprocess: Windows: Remove `CREATE_JOB` parent hook.

[Andrew Schwartzmeyer]

> On Feb. 7, 2017, 11:37 a.m., Ilya Pronin wrote:
> > The declaration of `ParentHook::CREATE_JOB()` in `subprocess_base.hpp` 
> > should also be removed.

Good catch, thanks! This code had been added between my work and when I had to 
rebase it, so I missed this part.


- Andrew


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


On Feb. 7, 2017, 2:31 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56368/
> ---
> 
> (Updated Feb. 7, 2017, 2:31 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Windows: Remove `CREATE_JOB` parent hook.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> bc1b99b8f84332f42623c35fc2b8d5186ba8af70 
> 
> Diff: https://reviews.apache.org/r/56368/diff/
> 
> 
> Testing
> ---
> 
> This is the last patch in a series. Applied all together, they were tested 
> with `make && make check` on Linux, and `support\windows-build.bat` on 
> Windows. All tests pass (minus flaky ones which sometimes fail but that's 
> unrelated).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56362: Windows: Create the `WindowsLauncher`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 7, 2017, 6:54 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Add `#ifdef __linux__` guard around `systemd` back.


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


Repository: mesos


Description (updated)
---

Instead of deriving `WindowsLauncher` from `PosixLauncher`,
this commit implements a separate `WindowsLauncher` derived
from `Launcher` parallel to the `PosixLauncher`.

The purpose of this is to setup for refactoring the `WindowsLauncher`
into a `Launcher` that uses Windows' "Job Objects,"
which are similar to Linux's cgroups, and enable us to reason
about a group of processes. It is necessary to do this in the
`WindowsLaucher` because the current implementation uses the POSIX
idea of a process hierarchy to list and kill all processes
associated with a task. We have found this model to not work on
Windows, and the solution is to use Job Objects.

Because this is preparation work, it is a straight copy of the
current "working" implementation. That is, the `PosixLauncher`
has been copied to create the `WindowsLauncher`, instead of
the latter deriving the former.


Diffs (updated)
-

  src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/launcher.hpp 
79f6eea0ee8e564e90b36208672df150dbc5d540 
  src/slave/containerizer/mesos/launcher.cpp 
5114c130efbfb252dde1e85c081f5174e66f57af 
  src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
  src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56362: Windows: Create the `WindowsLauncher`.

[Andrew Schwartzmeyer]

> On Feb. 7, 2017, noon, Ilya Pronin wrote:
> > src/slave/containerizer/mesos/windows_launcher.cpp, line 105
> > 
> >
> > Systemd on Windows? ;)

This iteration is the copy of the existing implementation. It was refactored 
into the real `WindowsLauncher` in 
[#56365](https://reviews.apache.org/r/56365/).


> On Feb. 7, 2017, noon, Ilya Pronin wrote:
> > src/slave/containerizer/mesos/launcher.cpp, line 110
> > 
> >
> > I think it would be better to leave `__linux__` check in place. There's 
> > no systemd on macOS or FreeBSD.

Sure thing!


- Andrew


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


On Feb. 7, 2017, 2:30 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56362/
> ---
> 
> (Updated Feb. 7, 2017, 2:30 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of deriving `WindowsLauncher` from `PosixLauncher`,
> this commit implements a separate `WindowsLauncher` derived
> from `Launcher` parallel to the `PosixLauncher`.
> 
> The purpose of this is to setup for refactoring the `WindowsLauncher`
> into a `Launcher` that uses Windows' "Job Objects,"
> which are similar to Linux's cgroups, and enable us to reason
> about a group of processes. It is necessary to do this in the
> `WindowsLaucher` because the current implementation uses the POSIX
> idea of a process hierarchy to list and kill all processes
> associated with a task. We have found this model to not work on
> Windows, and the solution is to use Job Objects.
> 
> Because this is preparation work, it is a straight copy of the
> current "working" implementation. That is, the `PosixLauncher`
> has been copied to create the `WindowsLauncher`, instead of
> the latter deriving the former.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/launcher.hpp 
> 79f6eea0ee8e564e90b36208672df150dbc5d540 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56340: Disabled ability to launch default executor with ContainerInfo.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 7, 2017, 1:18 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56340/
> ---
> 
> (Updated Feb. 7, 2017, 1:18 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The default executor used to launch TaskGroups is generated by the
> agent.  The agent generates the executor's `CommandInfo`, hence why
> the user may not specify the `CommandInfo` in the `LAUNCH_TASK_GROUP`
> call.
> 
> This commit adds similar restrictions for `ContainerInfo` plus the
> default executor.  The `CommandInfo` constructed by the agent expects
> to be run in the same environment as the agent process.  This commit
> prevents the user from specifying a `DockerInfo` or a container image
> along with the default executor.
> 
> If the user explicitly wants to use the default executor, they could
> always package the default executor's binary and libraries into a
> container and launch it like any other custom executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 53885cbc63ac6658a749da5e05bb2301392f84dd 
>   include/mesos/v1/mesos.proto c4ca6dea787cfe4661c9f0d9afb770bceb1c2639 
>   src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd 
>   src/tests/master_validation_tests.cpp 
> 1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2 
> 
> Diff: https://reviews.apache.org/r/56340/diff/
> 
> 
> Testing
> ---
> 
> src/mesos-tests --gtest_filter="ExecutorValidationTest.ExecutorType"
> 
> NOTE: The change to TaskGroupValidationTest.ExecutorUsesDockerContainerInfo 
> is because the tested case now fails during "general" validation of the 
> ExecutorInfo.  We still need the checks around `Option 
> validateExecutor(TaskGroupInfo, ExecutorInfo, Framework*, Slave*, Resources)` 
> because those checks are valid for custom executors.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56339: Changed test DockerArchive to include environment variables.

[Jie Yu]

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




src/tests/containerizer/docker_archive.hpp (lines 101 - 105)


I think this only applies to 'Task'. If the image is used by the executor, 
then this will certainly break them?


- Jie Yu


On Feb. 6, 2017, 10:04 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56339/
> ---
> 
> (Updated Feb. 6, 2017, 10:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-7027
> https://issues.apache.org/jira/browse/MESOS-7027
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This subtly modifies all tests using the `docker/runtime` isolator
> to fail if environment variables from inside the DockerArchive
> are passed into the Mesos executor's environment.  This applies
> for all executors (default, command, and docker), but mainly
> affects the command executor.
> 
> The environment variables are `LD_LIBRARY_PATH`, `LIBPROCESS_IP`,
> and `LIBPROCESS_PORT`; all of which are set to `invalid`.  This
> either causes linking problems or will force libprocess to exit.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_archive.hpp 
> b36dbdba7acf0587e18aced3581f68a1269b04d2 
> 
> Diff: https://reviews.apache.org/r/56339/diff/
> 
> 
> Testing
> ---
> 
> This causes some tests to fail, particularly many of the 
> `DockerRuntimeIsolatorTest` in `runtime_isolator_tests.cpp`.
> 
> A variety of other tests which use `DockerArchive` will also fail.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 55771: Rejected optimizing if we are working with a libcxx with UB.

[Till Toenshoff]

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




configure.ac (line 1891)


Sorry for not noticing ealier but I believe the idiomatic autotools way 
here is to use a pair of `AC_MSG_CHECKING([string])`, 
`AC_MSG_RESULT([string])`. We could then have the failure message follow that 
result; something along these lines, I guess

```
AC_MSG_CHECKING([C++ standard library has undefined behaviour with selected 
optimization level])
AC_LANG_PUSH([C++])
AC_RUN_IFELSE([
  AC_LANG_SOURCE([[
  [...]
  ]])],
  [message='no'], [message='yes']
)
AC_MSG_RESULT([$message])

if test "x$message" = "xyes"; then
  AC_MSG_ERROR([Mesos cannot be built with optimizations against this 
version of libcxx (MESOS-5745).
  Consider building without optimizations, or changing the 
used C++ standard library.])
fi
```

...which would look like this;

```
Checking C++ standard library has undefined behaviour with selected 
optimization level... no
```


- Till Toenshoff


On Jan. 23, 2017, 11:33 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55771/
> ---
> 
> (Updated Jan. 23, 2017, 11:33 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6606
> https://issues.apache.org/jira/browse/MESOS-6606
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   configure.ac 60ffa0e6a0dc590af929ab8a011b6d78f82a48e0 
> 
> Diff: https://reviews.apache.org/r/55771/diff/
> 
> 
> Testing
> ---
> 
> Tested on OS X Sierra with both bundled clang+stdlib and versions close to 
> their HEADs with
> 
> $ autoreconf -fi && ./configure CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure CXX=clang++  # versions from HEAD
> 
> $ autoreconf -fi && ./configure --enable-optimize CXX=/usr/bin/clang++
> $ autoreconf -fi && ./configure --enable-optimize CXX=clang++  # versions 
> from HEAD
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56360: Fixed consistency in error messages.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [54649, 54650, 56375, 56360]

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 Feb. 7, 2017, 7:34 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56360/
> ---
> 
> (Updated Feb. 7, 2017, 7:34 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed consistency in error messages.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd 
> 
> Diff: https://reviews.apache.org/r/56360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 55462: Validate resource reservation against allocated role.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 5:43 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Changes
---

Used `AllocationInfo` now in master.


Summary (updated)
-

Validate resource reservation against allocated role.


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


Repository: mesos


Description (updated)
---

This change introduces validation of the 'AllocationInfo' of resources
used in reservations.


Diffs (updated)
-

  src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd 

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


Testing
---

N/A yet.


Thanks,

Benjamin Bannier

Review Request 56392: Tightened test expecations in reservation-related tests.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Repository: mesos


Description
---

This change makes expected error strings in reservation
validation-related tests explicit. This is to make sure that the
observed errors match the expected ones.


Diffs
-

  src/tests/master_validation_tests.cpp 
1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2 

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


Testing
---

make check (OS X)


Thanks,

Benjamin Bannier

Review Request 56391: Set AllocationInfo in some reservation-related tests.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


Repository: mesos


Description
---

In a later change we will take a 'Resource''s 'AllocationInfo' into
account. Update a number of tests related to resource validation to
set 'AllocationInfo' so continue to trigger the originally expected
errors.


Diffs
-

  src/tests/master_validation_tests.cpp 
1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2 

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


Testing
---

make check (OS X)


Thanks,

Benjamin Bannier

Re: Review Request 55461: Made resource reservation validation multi-role aware.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 5:41 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.


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


Repository: mesos


Description
---

This updates the resource reservation validation for frameworks which
can have multiple roles. During a deprecation period 'FrameworkInfo'
will have fields for both 'role' and 'roles', however the validation
function works with just an optional set of roles. Here an empty set
captures the previous semantics of either having an empty 'role' field
or 'role' set as '*'. This forces the callers to properly construct a
set of framework roles from the available information. An optional set
is used in order to accommodate callers which have no information
about the framework's roles, and ultimately disables validation taking
that information into account.


Diffs (updated)
-

  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/master/validation.hpp fc1eaeebd0643b897f4e14aabc76e7d94f0de263 
  src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd 
  src/tests/master_validation_tests.cpp 
1f833aa8d6cacbd7bf502fb66c556e4e3d4f79e2 

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


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 55901: Added support for command health checks to the default executor.

[Alexander Rukletsov]

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




src/checks/health_checker.cpp (line 512)


You can use `->` for options instead of `.get()`.



src/checks/health_checker.cpp (lines 585 - 588)


Mention `taskId`?



src/checks/health_checker.cpp (line 594)


How about "WAIT_NESTED_CONTAINER API call..."



src/checks/health_checker.cpp (lines 618 - 624)


1) You have no interest in both `future` and `status`. Omit the names for 
clarity, please.

2) Why do we need to call `waitForNestedContainer` here instead of simply 
returning `failure`?



src/checks/health_checker.cpp (lines 640 - 641)


Is this formatting correct? Is it what clang-format suggests?



src/checks/health_checker.cpp (line 646)


"... while waiting..."?



src/checks/health_checker.cpp (line 653)


Don't you need to check that `wait_nested_container().exit_status()` 
exists? If `exit_status` is not set in the proto, don't you have to check this 
explicitly and return `None`?


- Alexander Rukletsov


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56370: Added a test to ensure multi-role framework receiving offers.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56370]

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 Feb. 7, 2017, 7:29 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56370/
> ---
> 
> (Updated Feb. 7, 2017, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-7062
> https://issues.apache.org/jira/browse/MESOS-7062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that multi-role framework should receive offers for
> each of its roles.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 
> 
> Diff: https://reviews.apache.org/r/56370/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> make check GTEST_FILTER="MasterTest.MultiRoleFrameworkReceivesOffers"
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 51942: Removed scalars() API.

[Guangya Liu]

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

(Updated 二月 7, 2017, 3:14 p.m.)


Review request for mesos, Benjamin Mahler, Neil Conway, and Jiang Yan Xu.


Repository: mesos


Description
---

This API is not used anymore because of performance issues and
replaced by `createStrippedScalarQuantity`.


Diffs (updated)
-

  include/mesos/resources.hpp 73e44be8aa03cfd167ff47bbff5d708fb545c01b 
  include/mesos/v1/resources.hpp 19132298cf9f51a60f700ed5ac8c3fbe93e8c40f 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 56362: Windows: Create the `WindowsLauncher`.

[Ilya Pronin]

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




src/slave/containerizer/mesos/launcher.cpp 


I think it would be better to leave `__linux__` check in place. There's no 
systemd on macOS or FreeBSD.



src/slave/containerizer/mesos/windows_launcher.cpp (line 105)


Systemd on Windows? ;)


- Ilya Pronin


On Feb. 7, 2017, 2:30 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56362/
> ---
> 
> (Updated Feb. 7, 2017, 2:30 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of deriving `WindowsLauncher` from `PosixLauncher`,
> this commit implements a separate `WindowsLauncher` derived
> from `Launcher` parallel to the `PosixLauncher`.
> 
> The purpose of this is to setup for refactoring the `WindowsLauncher`
> into a `Launcher` that uses Windows' "Job Objects,"
> which are similar to Linux's cgroups, and enable us to reason
> about a group of processes. It is necessary to do this in the
> `WindowsLaucher` because the current implementation uses the POSIX
> idea of a process hierarchy to list and kill all processes
> associated with a task. We have found this model to not work on
> Windows, and the solution is to use Job Objects.
> 
> Because this is preparation work, it is a straight copy of the
> current "working" implementation. That is, the `PosixLauncher`
> has been copied to create the `WindowsLauncher`, instead of
> the latter deriving the former.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/launcher.hpp 
> 79f6eea0ee8e564e90b36208672df150dbc5d540 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56368: Libprocess: Windows: Remove `CREATE_JOB` parent hook.

[Ilya Pronin]

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



The declaration of `ParentHook::CREATE_JOB()` in `subprocess_base.hpp` should 
also be removed.

- Ilya Pronin


On Feb. 7, 2017, 2:31 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56368/
> ---
> 
> (Updated Feb. 7, 2017, 2:31 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Windows: Remove `CREATE_JOB` parent hook.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> bc1b99b8f84332f42623c35fc2b8d5186ba8af70 
> 
> Diff: https://reviews.apache.org/r/56368/diff/
> 
> 
> Testing
> ---
> 
> This is the last patch in a series. Applied all together, they were tested 
> with `make && make check` on Linux, and `support\windows-build.bat` on 
> Windows. All tests pass (minus flaky ones which sometimes fail but that's 
> unrelated).
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Benjamin Bannier]

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

(Updated Feb. 7, 2017, 11:26 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.


Changes
---

Add link to `value`'s deprecation ticket.


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


Repository: mesos


Description
---

This updates the local authorizer so that MULTI_ROLE frameworks can be
authorized.

For non-MULTI_ROLE frameworks we continue to support use of the
deprecated 'value' field in the authorization request's 'Object';
however for MULTI_ROLE frameworks the 'value' field will not be set,
and authorizers still relying on it should be updated to instead use
the object's 'framework_info' field to extract roles to authorize
against from.


Diffs (updated)
-

  src/authorizer/local/authorizer.cpp b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 

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


Testing
---

Tested on various configurations in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Benjamin Bannier]

> On Feb. 7, 2017, 10:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > 
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?

There is only a weak relation between this framework capability and the 
capabilities of authorizers. My thinking here was roughly:

* The way authorization works ("send at most a single authorization request per 
authorizable action") all information required for authorization needs to be 
packed into a single authorization request.
* There are authorizers relying only on the `value` field to determine the role 
of a framework; this approach is deprecated.
* In the current authorization approach there is no meaningful value for 
`value` for multirole frameworks.
* Multirole frameworks are a new feature so not updating deprecated features to 
work with them does not break backwards compatibility.

With this change we

* maintain the current, single-request authorization approach,
* avoid breaking authorizers assuming frameworks can only have a single role,
* avoid breaking authorizers using `value` and assuming a single-role world, and
* enable authorizers using `framework_info` to authorize multirole frameworks.

This is very much in line with how e.g., `FrameworkInfo` has both a 
(deprecated) `role` and a `roles` field, and the correct action is taken 
depending on framework capabilities.

Would we instead e.g., send an authorization request for each role, we wouldn't 
just introduce new code to use a deprecated field (`value`), but we would also 
introduce a lot of potential overhead, all while ignoring the existence of 
`framework_info` which already now bundles all this information and can be sent 
as part of an authorization request.


> On Feb. 7, 2017, 10:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2177
> > 
> >
> > I find it helpful to mention when the deprecation cycle started 
> > (release date or version), so we can compute when it ends, based on our 
> > current policy.

`Object` became a variant over different authorizable objects with 1.0.0 which 
I believe also started `value`'s deprecation cycle. In some places in the code 
1.2 is explicitly called out to remove `value`, but there seem to be some 
confusion around API stability guarantees, fixed time frames and experimental 
features.

I created MESOS-7073 to track removal of `value`; I also mention it in this 
`TODO` now.


- Benjamin


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


On Feb. 7, 2017, 11:26 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 11:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56330: Enabled suppress offer per role.

[Guangya Liu]

> On 二月 6, 2017, 9:15 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > 
> >
> > Hm.. it doesn't look like any of the comments in this header make use 
> > of @param.

Do you mean we do not need `@param`? But there are actually many places using 
such format as 
https://github.com/apache/mesos/blob/master/include/mesos/allocator/allocator.hpp#L76-L86
 , comments?


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56330/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled suppress offer per role.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 71a40537b673e44ecdd5327d9a9f083faa7fc13a 
>   src/master/allocator/mesos/allocator.hpp 
> e3c86181677302dbfc3b213715196122f96e312c 
>   src/master/allocator/mesos/hierarchical.hpp 
> 896abcdf0727f986eef3a1a9304a0e4847094057 
>   src/master/allocator/mesos/hierarchical.cpp 
> 56d6791baa64189523df668749f4a7ab67d6b363 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56330/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> Will add a test case to enable suppress per role in follow up patches.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 56378: Added test case for suppress and revive with multi role framework.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Added test case for suppress and revive with multi role framework.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="HierarchicalAllocatorTest.SuppressAndReviveOffersWithMultiRole" 
--gtest_repeat=100
```


Thanks,

Guangya Liu

Review Request 56371: Enabled `ReviveOffersMessage` support revive per role.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled `ReviveOffersMessage` support revive per role.


Diffs
-

  src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Review Request 56374: Enabled revive offer per role.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled revive offer per role.


Diffs
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
56d6791baa64189523df668749f4a7ab67d6b363 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Review Request 56373: Augmented master `Revive` API to accept `Call::Revive`.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Augmented master `Revive` API to accept `Call::Revive`.


Diffs
-

  src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
  src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 56330: Enabled suppress offer per role.

[Guangya Liu]

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

(Updated 二月 7, 2017, 10:10 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled suppress offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
56d6791baa64189523df668749f4a7ab67d6b363 
  src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

Will add a test case to enable suppress per role in follow up patches.


Thanks,

Guangya Liu

Review Request 56376: Updated allocator test to support create multi role framework.

[Guangya Liu]

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

Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Updated allocator test to support create multi role framework.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 55710: Added agent capabilities to /state endpoint of master.

[Jay Guo]

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

(Updated Feb. 7, 2017, 5:59 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.


Summary (updated)
-

Added agent capabilities to /state endpoint of master.


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


Repository: mesos


Description (updated)
---

Master should be able to reflect agent capabilities via  `/state`(v0)
and `getState`(v1) endpoints.


Diffs (updated)
-

  include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 
  include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd 
  src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a 
  src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
  src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 

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


Testing
---

make check GTEST_FILTER="MasterTest.StateEndpointAgentCapabilities"
[   OK ] MasterTest.StateEndpointAgentCapabilities (85 ms)
[--] 1 test from MasterTest (94 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (122 ms total)

make check on Ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 56359: Add framework principal to the slave state endpoint

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56359]

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 Feb. 7, 2017, 2:14 a.m., Jeff Malnick wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56359/
> ---
> 
> (Updated Feb. 7, 2017, 2:14 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-7071
> https://issues.apache.org/jira/browse/MESOS-7071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds the framework principal to the slave state endpoint. The 
> documentation for the agent state API needs to be updated to reflect this 
> change.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d68c9adc1db43c2e853c8b2290705fdc7739d45c 
> 
> Diff: https://reviews.apache.org/r/56359/diff/
> 
> 
> Testing
> ---
> 
> Tests are in progress.
> 
> 
> Thanks,
> 
> Jeff Malnick
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Alexander Rukletsov]

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, lines 426-429
> > 
> >
> > why a `.repair()` here?
> 
> Gastón Kleiman wrote:
> To fail the future/check with a nice descriptive message that will later 
> be logged.
> 
> Vinod Kone wrote:
> The message returned by `http::connect` should be good enough? Do we use 
> this pattern elsewhere esp. with connect? Most uses of repair I have seen in 
> the code base, transform the future not really to add extra logging 
> information.

We use `.repair` for adjusting the message or changing the error type, which is 
technically the same. Here are some examples from the code:
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/3rdparty/libprocess/src/http.cpp#L1766-L1769
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/master/http.cpp#L4695-L4697
https://github.com/apache/mesos/blob/25f4feae487d53a701adb787fd8a2e5f6166b789/src/slave/containerizer/mesos/io/switchboard.cpp#L1632-L1638

Now, the question is whether connection failure message is good enough? Gastón, 
could you trigger a failure path and check what error message will be returned?


- Alexander


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


On Feb. 3, 2017, 7:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 3, 2017, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.

[Adam B]

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



There's a difference between backwards compatibility with frameworks that are 
not capable of multi-role and backwards compatibility with authorizer modules 
that are not multi-role capable. Let's not confuse the two.


src/master/master.cpp (line 2177)


I find it helpful to mention when the deprecation cycle started (release 
date or version), so we can compute when it ends, based on our current policy.



src/master/master.cpp (line 2178)


What does the framework's capability have to do with whether a custom 
authorizer can support multi-role authorization?
If I have an old authorizer module, and multi-role (phase 1) capable 
frameworks, I will only be able to authorize frameworks with a single role 
(regardless of capability). For a multi-role framework, I could authorize a 
single role from the list, but that essentially provides a back-door for those 
frameworks to use other roles. Or I could call the authorizer multiple times, 
one for each role. But we wouldn't want to do that for multi-role capable 
authorizers. Maybe we need the concept of "module capabilites" now too?
Am I overthinking this?


- Adam B


On Feb. 7, 2017, 12:32 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> ---
> 
> (Updated Feb. 7, 2017, 12:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
> https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> ---
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 55140: Added tests for parsing OCI image spec.

[Qian Zhang]

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

(Updated Feb. 7, 2017, 4:43 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests for parsing OCI image spec.


Diffs
-

  src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
  src/tests/CMakeLists.txt c12a9f7f7be0ed87a5dab8f78590be5f65517b6c 
  src/tests/containerizer/oci_spec_tests.cpp PRE-CREATION 

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


Testing (updated)
---

make check GTEST_FILTER="OCISpecTest.*"


Thanks,

Qian Zhang

Re: Review Request 55140: Added tests for parsing OCI image spec.

[Qian Zhang]

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

(Updated Feb. 7, 2017, 4:42 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Added tests for parsing OCI image spec.


Diffs (updated)
-

  src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
  src/tests/CMakeLists.txt c12a9f7f7be0ed87a5dab8f78590be5f65517b6c 
  src/tests/containerizer/oci_spec_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

(Updated Feb. 7, 2017, 4:41 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
  src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55140: Added tests for parsing OCI image spec.

[Qian Zhang]

> On Feb. 5, 2017, 12:09 p.m., Jie Yu wrote:
> > src/tests/containerizer/oci_spec_tests.cpp, line 39
> > 
> >
> > Why you need a JSON here? Can that just be a string?
> > 
> > Ditto elsewhere in this file.

Thanks for catching this! I followed the same way of the tests in 
`appc_spec_test.cpp`, so I think we'd better to fix it in that file, right?


- Qian


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


On Feb. 1, 2017, 9:14 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55140/
> ---
> 
> (Updated Feb. 1, 2017, 9:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for parsing OCI image spec.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/tests/CMakeLists.txt 44b74ee591b14d7d69bc0c03c26167b33eaa789c 
>   src/tests/containerizer/oci_spec_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55140/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>