Re: Review Request 56962: Windows: Undefined `ACL` macro defined in Zookeeper headers.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56962]

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. 23, 2017, 1:24 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56962/
> ---
> 
> (Updated Feb. 23, 2017, 1:24 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On Windows, Zookeeper will define a `ACL` macro because `Windows.h`
> defines a `struct ACL`, different from Zookeeper's `struct ACL`.
> Since this is a macro, it bleeds into other sources and ends up
> replacing odd bits of code, such as when we access our namespaced
> `ACL`s.  i.e. `mesos::ACL::RunTask`.
> 
> This commit undefines the `ACL` macro after the inclusion of the
> Zookeeper headers in two places, while redefining the macro in the
> one source file where we use Zookeeper's `struct ACL`.
> 
> 
> Diffs
> -
> 
>   include/mesos/zookeeper/authentication.hpp 
> 6a955abfe02c5e1f8993cb5cb444a2fd257401b9 
>   include/mesos/zookeeper/zookeeper.hpp 
> d4723dde90ffcca3208d70fb1e8cae87a0d0d1a8 
>   src/tests/api_tests.cpp 378612dd4d038fb4d65fba60a4be00d4950d0c02 
>   src/zookeeper/authentication.cpp 0fd99b019a5b4f65d2094eee637351f7ff2206a9 
> 
> Diff: https://reviews.apache.org/r/56962/diff/
> 
> 
> Testing
> ---
> 
> msbuild Mesos.sln
> 
> src/mesos-tests.exe
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Jay Guo]

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

(Updated Feb. 23, 2017, 12:31 p.m.)


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


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


Repository: mesos


Description
---

In a mixed cluster, when an old agent (not MULTI_ROLE capable)
registers with new master (MULTI_ROLE capable), it cannot correctly
handle tasks from a MULTI_ROLE framework. Therefore, we should
disallow resources from these agents being allocated to MULTI_ROLE
frameworks.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 

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


Testing
---

./bin/mesos-tests.sh


Thanks,

Jay Guo

Re: Review Request 56837: Added agent capabilities to HierarchicalAllocatorProcess.

[Jay Guo]

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

(Updated Feb. 23, 2017, 12:31 p.m.)


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


Changes
---

address bmahler's comments.


Summary (updated)
-

Added agent capabilities to HierarchicalAllocatorProcess.


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


Repository: mesos


Description (updated)
---

Allocator should be aware of agent capabilities in order to make
correct/smarter decisions on resource allocations. The interface
`addSlave` is augmented to pass in agent capabilities and we store
them in HierarchicalAllocatorProcess. Tests are updated accordingly.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
23413d1f9d76009c999b35d2bc98afc52c136404 
  src/master/allocator/mesos/allocator.hpp 
38fbf16a0500fc9f6ce7a498a6af3d81d63fc215 
  src/master/allocator/mesos/hierarchical.hpp 
d33306745a7287b750cb4a5242c7527369d58d65 
  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
  src/tests/allocator.hpp c741a985660e1540ad8e3a9c387d513247e56714 
  src/tests/api_tests.cpp 378612dd4d038fb4d65fba60a4be00d4950d0c02 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 
  src/tests/master_allocator_tests.cpp 25c67d32eec5fede78eb9fcbc1009eeff7da0dad 
  src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 
  src/tests/persistent_volume_endpoints_tests.cpp 
ec8df33a3f0d4ad790d5cc753e1691c5517c39c0 
  src/tests/reservation_endpoints_tests.cpp 
7432d752120b43560aa96cad8bf3981ee8102e67 
  src/tests/reservation_tests.cpp 309ce8b9acc9131110198c14c655427d7fe9d603 
  src/tests/slave_recovery_tests.cpp 0e295915fea0a7314e173857249bd8726eeccd76 

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


Testing
---

make check


Thanks,

Jay Guo

Review Request 56967: Exposed agent capabilities via SlaveInfo::Capability directly.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Instead of storing a vector of SlaveInfo::Capability::Type and convert
to Capability in the code, we switched to store a vector of Capability
directly for easier use.


Diffs
-

  src/slave/constants.hpp 753756d373a2d68dde51600bf4ec1e7985186e2c 
  src/slave/constants.cpp dbd2ecb146f288a4dec50bf041768fd5c4b3cb72 
  src/slave/http.cpp 8a9fabf861369d3ae659dce21fa3932f6f7b9161 
  src/slave/slave.cpp 45905297836017e9031359894fc71e614c13cfcc 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 56771: Added regression test against fetcher SSL spillover.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [56771, 56711]

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

Error:
2017-02-23 03:43:19 URL:https://reviews.apache.org/r/56711/diff/raw/ 
[2000/2000] -> "56711.patch" [1]
error: patch failed: src/slave/containerizer/fetcher.cpp:22
error: src/slave/containerizer/fetcher.cpp: patch does not apply

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

- Mesos Reviewbot


On Feb. 23, 2017, 12:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56771/
> ---
> 
> (Updated Feb. 23, 2017, 12:43 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test against fetcher SSL spillover.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 9c7e8b94071501c435e26850d66a8f3e8950c6cd 
> 
> Diff: https://reviews.apache.org/r/56771/diff/
> 
> 
> Testing
> ---
> 
> Before applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> W0216 23:31:25.794740 12402 fetcher.cpp:899] Begin fetcher log (stderr in 
> sandbox) for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9 from running 
> command: /Users/till/Development/mesos-private/build/src/mesos-fetcher
> I0216 23:31:25.746598 3022980032 fetcher.cpp:531] Fetcher Info: 
> {"cache_directory":"\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/mesos\/fetch\/slaves\/","items":[{"action":"BYPASS_CACHE","uri":{"extract":true,"value":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM\/Fo2V66\/zpbEP1.gz"}}],"sandbox_directory":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM"}
> I0216 23:31:25.752025 3022980032 fetcher.cpp:442] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.752058 3022980032 fetcher.cpp:283] Fetching directly into the 
> sandbox directory
> I0216 23:31:25.752096 3022980032 fetcher.cpp:220] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.756001 3022980032 fetcher.cpp:205] Copied resource 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
>  to 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/zpbEP1.gz'
> SSL requires key! NOTE: Set path with LIBPROCESS_SSL_KEY_FILE
> 
> End fetcher log for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9
> E0216 23:31:25.795078 12402 fetcher.cpp:555] Failed to run mesos-fetcher: 
> Failed to fetch all URIs for container 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' 
> with exit status: 256
> ../../src/tests/fetcher_tests.cpp:1134: Failure
> (fetch).failure(): Failed to fetch all URIs for container 
> 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' with exit status: 256
> [  FAILED  ] FetcherTest.EnvironmentSpillover (115 ms)
> ```
> 
> After applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> [   OK ] FetcherTest.EnvironmentSpillover (300 ms)
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56837, 56530]

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. 22, 2017, 5:59 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56530/
> ---
> 
> (Updated Feb. 22, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a mixed cluster, when an old agent (not MULTI_ROLE capable)
> registers with new master (MULTI_ROLE capable), it cannot correctly
> handle tasks from a MULTI_ROLE framework. Therefore, we should
> disallow resources from these agents being allocated to MULTI_ROLE
> frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5441fa9d1fad1ca7819038db49c6d88e40571e4a 
> 
> Diff: https://reviews.apache.org/r/56530/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56809: Added test for nested container agent reboot case.

[Gilbert Song]

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

(Updated Feb. 22, 2017, 6:33 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.


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


Repository: mesos


Description
---

Added test for nested container agent reboot case.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
ce57c06d50b47a150ff40412c1fde99f16892434 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 56939: Validate that Resource.allocation_info.roles are not mixed.

[Benjamin Mahler]

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



High level thoughts:

(1) Can you split out the validation for task / executor / volume / 
reservation? That will make it easier to review and commit them piece by piece.

(2) Looks like there are no tests for volumes and reservations?


src/master/validation.cpp (line 588)


How about 'validateSingleAllocationRole'?



src/master/validation.cpp (lines 602 - 603)


These error messages compose a bit oddly:

```
Executor mixes allocation roles: Resources mixes allocations roles: role1 
and role2
```

How about:

```
Invalid executor resources: The resources have multiple allocation roles 
('role1' and 'role2') but only one allocation role is allowed
```

Here `"Invalid executor resources:"` comes from the caller and `"The 
resources have multiple allocation roles ('role1' and 'role2') but only one 
allocation role is allowed"` comes from the helper function.



src/master/validation.cpp (line 1658)


Why couldn't you use 
`resource::validateAllocationRole(reserve.resources())` here?


- Benjamin Mahler


On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56939/
> ---
> 
> (Updated Feb. 22, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6636
> https://issues.apache.org/jira/browse/MESOS-6636
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With support for multi-role frameworks, we need to make sure that
> individual tasks and executors cannot mix roles. Likewise, we do
> not want to allow a scheduler to make a reservation or create a
> volume based on resources with different allocated roles.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 
>   src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f 
>   src/tests/master_validation_tests.cpp 
> 53771f6b5492009fe75cbbfc03a2b542856c1070 
> 
> Diff: https://reviews.apache.org/r/56939/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 56962: Windows: Undefined `ACL` macro defined in Zookeeper headers.

[Joseph Wu]

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

Review request for mesos, Andrew Schwartzmeyer and Gastón Kleiman.


Repository: mesos


Description
---

On Windows, Zookeeper will define a `ACL` macro because `Windows.h`
defines a `struct ACL`, different from Zookeeper's `struct ACL`.
Since this is a macro, it bleeds into other sources and ends up
replacing odd bits of code, such as when we access our namespaced
`ACL`s.  i.e. `mesos::ACL::RunTask`.

This commit undefines the `ACL` macro after the inclusion of the
Zookeeper headers in two places, while redefining the macro in the
one source file where we use Zookeeper's `struct ACL`.


Diffs
-

  include/mesos/zookeeper/authentication.hpp 
6a955abfe02c5e1f8993cb5cb444a2fd257401b9 
  include/mesos/zookeeper/zookeeper.hpp 
d4723dde90ffcca3208d70fb1e8cae87a0d0d1a8 
  src/tests/api_tests.cpp 378612dd4d038fb4d65fba60a4be00d4950d0c02 
  src/zookeeper/authentication.cpp 0fd99b019a5b4f65d2094eee637351f7ff2206a9 

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


Testing
---

msbuild Mesos.sln

src/mesos-tests.exe


Thanks,

Joseph Wu

Re: Review Request 56941: Add a test for a MULTI_ROLE master re-registering an old agent.

[Benjamin Mahler]

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



Looks good overall, left some suggestions below. Main thing is I think we can 
simplify the test a bit if we don't need the mock executor and test 
containerizer. Also, seems these types of tests should be collected in a file 
called upgrade_tests.cpp so that we can more easily migrate them towards an 
approach that actually spins up different versions of components.


src/tests/master_tests.cpp (lines 6690 - 6695)


I would suggest moving this into a file called upgrade_tests.cpp with a 
comment at the top like the following:

```
// This file contains upgrade integration tests. Note that tests
// in this file can only "spoof" an old version of a component,
// since these run against the current version of the repository.
// The long term plan for integration tests is to checkout
// different releases of the repository and run components from
// different releases against each other.
```

That way we can hopefully collect all of the upgrade tests together, and 
ensure that these tests are migrated to tests that actually spin up different 
versions.



src/tests/master_tests.cpp (lines 6690 - 6694)


We should clarify here that we start a regular agent to get a task 
launched, then we restart the master and when the agent re-registers we strip 
the allocation info in order to spoof an old (non-MULTI_ROLE agent).



src/tests/master_tests.cpp (lines 6704 - 6705)


Why did you need a mock executor and test containerizer? Seems to me this 
can be simplified by just running a sleep task with no mocking?

E.g.

```
TaskInfo task = createTask(offers1.get()[0], "sleep 1000");
```

You can find examples with:

```
$ grep -R -i sleep src/test
```



src/tests/master_tests.cpp (line 6783)


Is this flaky? Looking at the agent source, I believe this has to be 
`slaveFlags.registration_backoff_factor * 2`, no?



src/tests/master_tests.cpp (lines 6796 - 6798)


You can wrap your loops like this if it fits in 80 characters:

```
  foreach (ExecutorInfo& executorInfo,
   *strippedReregisterSlaveMessage.mutable_executor_infos()) {
```



src/tests/master_tests.cpp (line 6834)


Can you do a sweep for `s/.get()./->/` ?



src/tests/master_tests.cpp (line 6838)


Feel free to use `.at("frameworks")` to avoid mutation here, ditto below 
for "tasks" and "executors"


- Benjamin Mahler


On Feb. 22, 2017, 5:43 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56941/
> ---
> 
> (Updated Feb. 22, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7063
> https://issues.apache.org/jira/browse/MESOS-7063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a non-MULTI_ROLE agent with running tasks re-registers, master
> should inject allocation roles for tasks/executors sent during
> re-registration.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 357a9a47ef87be6cbc4256d8afabe63cd41b1ea1 
> 
> Diff: https://reviews.apache.org/r/56941/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56814, 56195]

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. 22, 2017, 7:50 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56195/
> ---
> 
> (Updated Feb. 22, 2017, 7:50 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
> Jie Yu, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the containizer launch path would leak FDs if the
> containerizer launch path failed between successfully calling
> prepare() on either the ContainerLogger (in the case of the Docker
> containerizer) or the IOSwitchboard (in the case of the mesos
> containerizer) and forking the actual container.
> 
> These components relied on the Subprocess call inside launcher->fork()
> to close these FDS on their behalf. If the containerizer launch path
> failed somewhere between calling prepare() and making this fork()
> call, these FDs would never be closed.
> 
> In the case of the IOSwitchboard, this would lead to deadlock in the
> destroy path because the future returned by the IOSwitchboard's
> cleanup function would never be satisfied. The IOSwitchboard doesn't
> shutdown until the FDs it allocates to the container have been closed.
> 
> This commit fixes this problem by updating the
> ContainerLogger::SubprocessInfo::FD abstraction to change the way
> manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
> label and forcing the launcher->fork() call to deal with closing the
> FDs once it's forked a new subprocess, we now do things slightly
> differently now.
> 
> We now keep the default DUP label on each FD (instead fo changing it
> to OWNED) to cause launcher->fork() to dup it before mapping it onto
> the stdin/stdout/stderr of the subprocess. It then only closes the
> duped FD, leaving the original one open.
> 
> In doing so, it s now the contaienrizer's responsibility to ensure
> that these FDs are closed properly (whether that's between a
> successful prepare() call and launcher->fork()) or after
> launcher->fork() has completed successfully. While this has the
> potential to complicate things slightly on the SUCCESS path,
> at least it is now the containerizers's responsibility to close these
> FDS in *all* cases, rather than splitting that responsibility across
> components.
> 
> In order to simplify this, we've also modified the
> ContainerLogger::SubprocessInfo::FD abstraction to hold a Shared
> pointer to its underlying file descriptor and (optionally) close it on
> destruction. With this, we can ensure that all file descriptors
> created through this abstraction will be automatically closed onced
> their final reference goes out of scope (even if its been copied
> around several times).
> 
> In essence, this release the containerizer from the burden of manually
> closing these FDS itself. So long as it holds the final reference to
> these FDs (which it does), they will be automatically closed along
> *any* path out of containerizer->launch(). These are exactly the
> semantics we wanted to achieve.
> 
> In the case of the the ContainerLogger, ownership of these FDs (and
> thus their final reference) is passed to the containerizer in the
> SubprocessInfo struct returned by prepare(). In the case of the
> IOSwitchboard, we had to add a new API call to transfer ownership
> (since it is an isolator and prepare() can only return a protobuf),
> but the end result is the same.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   include/mesos/slave/containerizer.proto 
> c70d437ac3f8f813fcb71c04275cc8eeeb946065 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 10a9b57660388ac2409458a4d07af64cc3b185e2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 7a7ad2da047ef316f4382e45526d503c87a903a0 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> b948f3c44dd2e1af2228ca1579f24ae3a94160b0 
> 
> Diff: https://reviews.apache.org/r/56195/diff/
> 
> 
> Testing
> ---
> 
> Linux CentOS 7:
> ```
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 56809: Added test for nested container agent reboot case.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 23, 2017, 1:05 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56809/
> ---
> 
> (Updated Feb. 23, 2017, 1:05 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7152
> https://issues.apache.org/jira/browse/MESOS-7152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for nested container agent reboot case.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> ce57c06d50b47a150ff40412c1fde99f16892434 
> 
> Diff: https://reviews.apache.org/r/56809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56809: Added test for nested container agent reboot case.

[Gilbert Song]

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

(Updated Feb. 22, 2017, 5:05 p.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.


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


Repository: mesos


Description
---

Added test for nested container agent reboot case.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
ce57c06d50b47a150ff40412c1fde99f16892434 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 22, 2017, 4:41 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56711/
> ---
> 
> (Updated Feb. 22, 2017, 4:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Bugs: MESOS-6751 and MESOS-7133
> https://issues.apache.org/jira/browse/MESOS-6751
> https://issues.apache.org/jira/browse/MESOS-7133
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed fetcher to not pick up environment variables it should not see.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 9ec38dc95dddfcd990369d0146986e20b15da1a0 
> 
> Diff: https://reviews.apache.org/r/56711/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56771: Added regression test against fetcher SSL spillover.

[Till Toenshoff]

> On Feb. 22, 2017, 1:16 a.m., Adam B wrote:
> > src/tests/fetcher_tests.cpp, line 1112
> > 
> >
> > Maybe at least choose different strings for dirname and path?

Not sure I understand. Both names are very different in that they are 
indiviually randomized. Note that those "X"s are replaced with a unique 
alphanumeric combination by those `mktemp` `mkdtemp` functions.


- Till


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


On Feb. 23, 2017, 12:43 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56771/
> ---
> 
> (Updated Feb. 23, 2017, 12:43 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added regression test against fetcher SSL spillover.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 9c7e8b94071501c435e26850d66a8f3e8950c6cd 
> 
> Diff: https://reviews.apache.org/r/56771/diff/
> 
> 
> Testing
> ---
> 
> Before applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> W0216 23:31:25.794740 12402 fetcher.cpp:899] Begin fetcher log (stderr in 
> sandbox) for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9 from running 
> command: /Users/till/Development/mesos-private/build/src/mesos-fetcher
> I0216 23:31:25.746598 3022980032 fetcher.cpp:531] Fetcher Info: 
> {"cache_directory":"\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/mesos\/fetch\/slaves\/","items":[{"action":"BYPASS_CACHE","uri":{"extract":true,"value":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM\/Fo2V66\/zpbEP1.gz"}}],"sandbox_directory":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM"}
> I0216 23:31:25.752025 3022980032 fetcher.cpp:442] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.752058 3022980032 fetcher.cpp:283] Fetching directly into the 
> sandbox directory
> I0216 23:31:25.752096 3022980032 fetcher.cpp:220] Fetching URI 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
> I0216 23:31:25.756001 3022980032 fetcher.cpp:205] Copied resource 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
>  to 
> '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/zpbEP1.gz'
> SSL requires key! NOTE: Set path with LIBPROCESS_SSL_KEY_FILE
> 
> End fetcher log for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9
> E0216 23:31:25.795078 12402 fetcher.cpp:555] Failed to run mesos-fetcher: 
> Failed to fetch all URIs for container 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' 
> with exit status: 256
> ../../src/tests/fetcher_tests.cpp:1134: Failure
> (fetch).failure(): Failed to fetch all URIs for container 
> 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' with exit status: 256
> [  FAILED  ] FetcherTest.EnvironmentSpillover (115 ms)
> ```
> 
> After applying RR56711:
> ```
> [ RUN  ] FetcherTest.EnvironmentSpillover
> [   OK ] FetcherTest.EnvironmentSpillover (300 ms)
> ```
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56771: Added regression test against fetcher SSL spillover.

[Till Toenshoff]

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

(Updated Feb. 23, 2017, 12:43 a.m.)


Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.


Repository: mesos


Description
---

Added regression test against fetcher SSL spillover.


Diffs (updated)
-

  src/tests/fetcher_tests.cpp 9c7e8b94071501c435e26850d66a8f3e8950c6cd 

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


Testing
---

Before applying RR56711:
```
[ RUN  ] FetcherTest.EnvironmentSpillover
W0216 23:31:25.794740 12402 fetcher.cpp:899] Begin fetcher log (stderr in 
sandbox) for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9 from running 
command: /Users/till/Development/mesos-private/build/src/mesos-fetcher
I0216 23:31:25.746598 3022980032 fetcher.cpp:531] Fetcher Info: 
{"cache_directory":"\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/mesos\/fetch\/slaves\/","items":[{"action":"BYPASS_CACHE","uri":{"extract":true,"value":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM\/Fo2V66\/zpbEP1.gz"}}],"sandbox_directory":"\/private\/var\/folders\/_t\/rdp354gx7j5fjww270kbk6_rgn\/T\/KBvUlM"}
I0216 23:31:25.752025 3022980032 fetcher.cpp:442] Fetching URI 
'/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
I0216 23:31:25.752058 3022980032 fetcher.cpp:283] Fetching directly into the 
sandbox directory
I0216 23:31:25.752096 3022980032 fetcher.cpp:220] Fetching URI 
'/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
I0216 23:31:25.756001 3022980032 fetcher.cpp:205] Copied resource 
'/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/Fo2V66/zpbEP1.gz'
 to '/private/var/folders/_t/rdp354gx7j5fjww270kbk6_rgn/T/KBvUlM/zpbEP1.gz'
SSL requires key! NOTE: Set path with LIBPROCESS_SSL_KEY_FILE

End fetcher log for container b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9
E0216 23:31:25.795078 12402 fetcher.cpp:555] Failed to run mesos-fetcher: 
Failed to fetch all URIs for container 'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' 
with exit status: 256
../../src/tests/fetcher_tests.cpp:1134: Failure
(fetch).failure(): Failed to fetch all URIs for container 
'b0fd3f0f-ae72-4d88-b8c2-b4c1af1edee9' with exit status: 256
[  FAILED  ] FetcherTest.EnvironmentSpillover (115 ms)
```

After applying RR56711:
```
[ RUN  ] FetcherTest.EnvironmentSpillover
[   OK ] FetcherTest.EnvironmentSpillover (300 ms)
```


Thanks,

Till Toenshoff

Re: Review Request 56711: Fixed fetcher to not pick up environment variables it should not see.

[Till Toenshoff]

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

(Updated Feb. 23, 2017, 12:41 a.m.)


Review request for mesos, Adam B, Benjamin Bannier, and Greg Mann.


Changes
---

Addressed comments - thanks for those!


Bugs: MESOS-6751 and MESOS-7133
https://issues.apache.org/jira/browse/MESOS-6751
https://issues.apache.org/jira/browse/MESOS-7133


Repository: mesos


Description
---

Fixed fetcher to not pick up environment variables it should not see.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 9ec38dc95dddfcd990369d0146986e20b15da1a0 

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


Testing
---

make check


Thanks,

Till Toenshoff

Re: Review Request 56830: [WIP] Introduced `TaskInfo::TerminationPolicy` protobuf.

[Anand Mazumdar]

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

(Updated Feb. 23, 2017, 12:09 a.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


Changes
---

Review comments from Neil. Changing the summary to be WIP as there were a few 
offline discussions between me and Vinod that might result in further changes
to the way the protobuf is structured.


Summary (updated)
-

[WIP] Introduced `TaskInfo::TerminationPolicy` protobuf.


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


Repository: mesos


Description
---

Describes the termination policy associated with a task and is
applied upon a task termination.


Diffs (updated)
-

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
---

make check (tests later in the chain)


Thanks,

Anand Mazumdar

Re: Review Request 56945: Removed inconsistent use of override.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56945]

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. 22, 2017, 6:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56945/
> ---
> 
> (Updated Feb. 22, 2017, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes inconsistent use of the 'override' keyword where the
> 'LibeventSSLSocketImpl' class contained both a method explicitly
> marked 'override' and implicitly 'override' functions. This is
> diagnosed as an inconsistency by clang-4.0, e.g.,
> 
> /PATH/libevent_ssl_socket.hpp|43 col 27| warning: \
>   'connect' overrides a member function but is not marked \
>   'override' [-Winconsistent-missing-override]
>   virtual Future connect(const Address& address);
>   ^
> /PATH/socket.hpp|148 col 27| note: \
>   overridden virtual function is here
>   virtual Future connect(const Address& address) = 0;
> 
> A proper fix will be implemented as part of MESOS-4871.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/56945/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56667: Added support for JSON Web Tokens.

[Greg Mann]

> On Feb. 20, 2017, 7:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current 
> > implementation isn't compliant with RFCs 7515/7519? The one thing I noticed 
> > was the lack of support for the 'crit' header parameter.
> 
> Jan Schlicht wrote:
> There isn't support for `alg=none` and it is strongly recommended to also 
> support `alg=RS256`. Standard claims aren't validated, though it's up to the 
> specific applications to specify which of these claims are mandatory; it 
> would make sense to validate them as part of a general-purpose JWT 
> implementation. Decoded JSON isn't tested for line breaks, whitespaces, 
> correct UTF-8 encoding.
> 
> Jan Schlicht wrote:
> Sorry, I've read the RFC wrong: We don't have to test the JSON for line 
> breaks, but the base64. I'll add support for `alg=none` and the `crit` header.

Thx Jan!! If we run into any spec-compliance issues which will take a bunch of 
time to implement, we can always just make a note in comments/documentation of 
how we differ from the RFCs, and create tickets to update in the future.


> On Feb. 20, 2017, 7:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, line 77
> > 
> >
> > Do you think it's worth using a `JSON::Object` for the header? This 
> > would let the module accommodate arbitrary header keys (AKA 'Private Header 
> > Parameter Names' from RFC-7515), which could be useful for users who want 
> > to use the module for other purposes?
> 
> Jan Schlicht wrote:
> Same as above: It depends on what the scope of this implementation should 
> be. An internal-only JWT implementation doesn't need a `JSON::Object` for the 
> header, because we control what will be in the header. Of course, if this 
> module should be more general-purpose, this would need to be changed, along 
> with some other changes. But then we could also strive for full RFC 
> compliance, which would mean (among others) support for `alg=none`, probably 
> `alg=RS256` and other subtleties.

Looks like this isn't a strict requirement of the spec, so feel free to ignore 
for now :)


- Greg


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


On Feb. 22, 2017, 2:26 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated Feb. 22, 2017, 2:26 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56667/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56834: Send a TASK_ERROR update if a task specifies `TerminationPolicy`.

[Vinod Kone]

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




src/docker/executor.cpp (line 136)


TASK_ERROR?


- Vinod Kone


On Feb. 22, 2017, 6:17 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56834/
> ---
> 
> (Updated Feb. 22, 2017, 6:17 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker/command executor currently do not support it yet.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
>   src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> Diff: https://reviews.apache.org/r/56834/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56809: Added test for nested container agent reboot case.

[Jie Yu]

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 988 - 1000)


Why you need to create the containerizer here?



src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 1015)


Can you just call provisioner->recover()?


- Jie Yu


On Feb. 22, 2017, 7:17 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56809/
> ---
> 
> (Updated Feb. 22, 2017, 7:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7152
> https://issues.apache.org/jira/browse/MESOS-7152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for nested container agent reboot case.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> ea01fe55a28d17105157004d8cf0976202a49b7c 
> 
> Diff: https://reviews.apache.org/r/56809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Feb. 22, 2017, 5:59 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56530/
> ---
> 
> (Updated Feb. 22, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In a mixed cluster, when an old agent (not MULTI_ROLE capable)
> registers with new master (MULTI_ROLE capable), it cannot correctly
> handle tasks from a MULTI_ROLE framework. Therefore, we should
> disallow resources from these agents being allocated to MULTI_ROLE
> frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5441fa9d1fad1ca7819038db49c6d88e40571e4a 
> 
> Diff: https://reviews.apache.org/r/56530/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Greg Mann]

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




3rdparty/libprocess/include/process/authenticator.hpp (lines 110 - 112)


Could you elaborate a bit here on the implementation? i.e., the type of 
signature supported, RFC compliance or lack thereof, etc.

Also, it might be more accurate to say something like "Implements the 
"Bearer" authentication scheme using JSON web tokens. Token signatures are 
validated using the specified secret key."



3rdparty/libprocess/src/jwt_authenticator.cpp (line 58)


Do we need the `http::` namespace here? It isn't used for 
`JWTAuthenticator::authenticate` below.



3rdparty/libprocess/src/jwt_authenticator.cpp (line 61)


AFAIK we should return a WWW-Authenticate header containing 'Bearer 
realm="REALM"' here, since we're expecting the 'Bearer' scheme in client 
requests.



3rdparty/libprocess/src/jwt_authenticator.cpp (lines 69 - 73)


Since you explicitly limit the number of tokens to 2 here, I don't think we 
need the `token.size() != 2` check.



3rdparty/libprocess/src/jwt_authenticator.cpp (lines 77 - 89)


In these error cases, we could provide additional error information in the 
WWW-Authenticate header. See https://tools.ietf.org/html/rfc6750#section-3

We can use "error=invalid_token, error_description=ERROR_MESSAGE", with 
appropriate messages for each case. Perhaps it makes sense to construct and 
return the Unauthorized response within each conditional so that we can set the 
header appropriately at construction?



3rdparty/libprocess/src/jwt_authenticator.cpp (lines 81 - 89)


Is there a reason to do this in two stages, as opposed to doing 
`jwt->payload.find("sub")`?



3rdparty/libprocess/src/jwt_authenticator.cpp (line 97)


Is it possible to use `JSON::String` here directly?



3rdparty/libprocess/src/jwt_authenticator.cpp (lines 99 - 101)


Are you sure this won't end up silently ignoring fields which have 
numerical values? If the field's value is a JSON number, we should probably 
convert it to a string and place it in the map. Similarly with other JSON 
types; could we stringify a JSON object and place it into the map as well?



3rdparty/libprocess/src/jwt_authenticator.cpp (lines 104 - 107)


FYI I updated the `AuthenticationContext` patches to use a `map` instead of 
`Option`, and I added a constructor `AuthenticationContext(Option, 
map)`, so you could make use of it here. In that case we would 
duplicate the principal in the "sub" claim, but perhaps that's OK?



3rdparty/libprocess/src/jwt_authenticator.cpp (line 138)


I think this should return "Bearer"


- Greg Mann


On Feb. 22, 2017, 2:28 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 22, 2017, 2:28 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> Diff: https://reviews.apache.org/r/56753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56837: Added slave capabilities to HierarchicalAllocatorProcess.

[Benjamin Mahler]

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



Needs to be updated still to include `updateSlave` changes. Otherwise looks 
good.


include/mesos/allocator/allocator.hpp (line 179)


I think this is supposed to say:

```
   * @param capabilities Capabilities of the agent.
```



src/tests/hierarchical_allocator_tests.cpp (lines 80 - 81)


Why not just use the one in slave/constants.hpp?

If we use only MULTI_ROLE here, then whenever other capabilities are added 
these tests won't pick them up.


- Benjamin Mahler


On Feb. 22, 2017, 5:58 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56837/
> ---
> 
> (Updated Feb. 22, 2017, 5:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-6940
> https://issues.apache.org/jira/browse/MESOS-6940
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocator should be aware of slave capabilities in order to make
> correct/smarter decisions on resource allocations. The interface
> `addSlave` is augmented to pass in agent capabilities and we store
> them in HierarchicalAllocatorProcess. Tests are updated accordingly.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 23413d1f9d76009c999b35d2bc98afc52c136404 
>   src/master/allocator/mesos/allocator.hpp 
> 38fbf16a0500fc9f6ce7a498a6af3d81d63fc215 
>   src/master/allocator/mesos/hierarchical.hpp 
> d33306745a7287b750cb4a5242c7527369d58d65 
>   src/master/allocator/mesos/hierarchical.cpp 
> eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
>   src/tests/allocator.hpp c741a985660e1540ad8e3a9c387d513247e56714 
>   src/tests/api_tests.cpp 378612dd4d038fb4d65fba60a4be00d4950d0c02 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5441fa9d1fad1ca7819038db49c6d88e40571e4a 
>   src/tests/master_allocator_tests.cpp 
> 25c67d32eec5fede78eb9fcbc1009eeff7da0dad 
>   src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> ec8df33a3f0d4ad790d5cc753e1691c5517c39c0 
>   src/tests/reservation_endpoints_tests.cpp 
> 7432d752120b43560aa96cad8bf3981ee8102e67 
>   src/tests/reservation_tests.cpp 309ce8b9acc9131110198c14c655427d7fe9d603 
>   src/tests/slave_recovery_tests.cpp 0e295915fea0a7314e173857249bd8726eeccd76 
> 
> Diff: https://reviews.apache.org/r/56837/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56621: Updated Mesos tests to use 'AuthenticationContext'.

[Greg Mann]

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

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


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


Changes
---

Accommodated validation logging changes.


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


Repository: mesos


Description
---

This patch updates the Mesos tests to use
authenticated handlers which accept an
`AuthenticationContext` instead of an
`Option principal`.


Diffs (updated)
-

  src/tests/files_tests.cpp 6c6353e406249f021803e83909415e9908ded28c 
  src/tests/http_authentication_tests.cpp 
95f01c432f77d1a944dd78c343e21b310676417f 
  src/tests/master_validation_tests.cpp 
53771f6b5492009fe75cbbfc03a2b542856c1070 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56813: Updated master handlers to use 'AuthenticationContext'.

[Greg Mann]

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

(Updated Feb. 22, 2017, 7:53 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


Changes
---

Updated master's `scheduler` endpoint code.


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


Repository: mesos


Description
---

This patch updates the HTTP endpoint handlers in the
agent process to accept an `AuthenticationContext`
instead of an `Option& principal`.


Diffs (updated)
-

  src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 

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


Testing
---

Testing details can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56941: Add a test for a MULTI_ROLE master re-registering an old agent.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56938, 56939, 56941]

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. 22, 2017, 5:43 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56941/
> ---
> 
> (Updated Feb. 22, 2017, 5:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7063
> https://issues.apache.org/jira/browse/MESOS-7063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a non-MULTI_ROLE agent with running tasks re-registers, master
> should inject allocation roles for tasks/executors sent during
> re-registration.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 357a9a47ef87be6cbc4256d8afabe63cd41b1ea1 
> 
> Diff: https://reviews.apache.org/r/56941/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Kevin Klues]

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

(Updated Feb. 22, 2017, 7:50 p.m.)


Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
Jie Yu, Joseph Wu, and Vinod Kone.


Changes
---

Addressed comments. Fixed a rogue whitespace.


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


Repository: mesos


Description
---

Previously, the containizer launch path would leak FDs if the
containerizer launch path failed between successfully calling
prepare() on either the ContainerLogger (in the case of the Docker
containerizer) or the IOSwitchboard (in the case of the mesos
containerizer) and forking the actual container.

These components relied on the Subprocess call inside launcher->fork()
to close these FDS on their behalf. If the containerizer launch path
failed somewhere between calling prepare() and making this fork()
call, these FDs would never be closed.

In the case of the IOSwitchboard, this would lead to deadlock in the
destroy path because the future returned by the IOSwitchboard's
cleanup function would never be satisfied. The IOSwitchboard doesn't
shutdown until the FDs it allocates to the container have been closed.

This commit fixes this problem by updating the
ContainerLogger::SubprocessInfo::FD abstraction to change the way
manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
label and forcing the launcher->fork() call to deal with closing the
FDs once it's forked a new subprocess, we now do things slightly
differently now.

We now keep the default DUP label on each FD (instead fo changing it
to OWNED) to cause launcher->fork() to dup it before mapping it onto
the stdin/stdout/stderr of the subprocess. It then only closes the
duped FD, leaving the original one open.

In doing so, it s now the contaienrizer's responsibility to ensure
that these FDs are closed properly (whether that's between a
successful prepare() call and launcher->fork()) or after
launcher->fork() has completed successfully. While this has the
potential to complicate things slightly on the SUCCESS path,
at least it is now the containerizers's responsibility to close these
FDS in *all* cases, rather than splitting that responsibility across
components.

In order to simplify this, we've also modified the
ContainerLogger::SubprocessInfo::FD abstraction to hold a Shared
pointer to its underlying file descriptor and (optionally) close it on
destruction. With this, we can ensure that all file descriptors
created through this abstraction will be automatically closed onced
their final reference goes out of scope (even if its been copied
around several times).

In essence, this release the containerizer from the burden of manually
closing these FDS itself. So long as it holds the final reference to
these FDs (which it does), they will be automatically closed along
*any* path out of containerizer->launch(). These are exactly the
semantics we wanted to achieve.

In the case of the the ContainerLogger, ownership of these FDs (and
thus their final reference) is passed to the containerizer in the
SubprocessInfo struct returned by prepare(). In the case of the
IOSwitchboard, we had to add a new API call to transfer ownership
(since it is an isolator and prepare() can only return a protobuf),
but the end result is the same.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 
  include/mesos/slave/containerizer.proto 
c70d437ac3f8f813fcb71c04275cc8eeeb946065 
  src/slave/containerizer/mesos/containerizer.hpp 
10a9b57660388ac2409458a4d07af64cc3b185e2 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/io/switchboard.hpp 
7a7ad2da047ef316f4382e45526d503c87a903a0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
b948f3c44dd2e1af2228ca1579f24ae3a94160b0 

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


Testing
---

Linux CentOS 7:
```
GTEST_FILTER="" make -j check
sudo src/mesos-tests
```


Thanks,

Kevin Klues

Re: Review Request 56814: Wrapped IOSwitchboard.connect() in a dispatch.

[Kevin Klues]

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

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


Review request for mesos and Jie Yu.


Changes
---

Addressed Benjamin Bannier's comments.


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


Repository: mesos


Description
---

Since the IOSwitchboard is implemented as a MesosIsolatorProcess, most
of its API calls are automatically dispatched onto its underlying
process by an Isolator wrapper. However, the IOSwitchboard also
includes an additional connect() call which is not accessed through
the Isolator wrapper. As such, we need to wrap it in a dispatch call
manually.


Diffs (updated)
-

  src/slave/containerizer/mesos/io/switchboard.hpp 
7a7ad2da047ef316f4382e45526d503c87a903a0 
  src/slave/containerizer/mesos/io/switchboard.cpp 
b948f3c44dd2e1af2228ca1579f24ae3a94160b0 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 56901: Updated master validation code to use 'AuthenticationContext'.

[Greg Mann]

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

(Updated Feb. 22, 2017, 7:48 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


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


Repository: mesos


Description
---

This patch updates master validation code to make use
of the `AuthenticationContext` instead of an
`Option principal`.


Diffs (updated)
-

  src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 
  src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f 

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


Testing
---

Testing details can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56938: Added allocation role to tasks/executors in v0 /state API.

[Benjamin Mahler]

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



This needs to update the following as well:
https://github.com/apache/mesos/blob/807b2343fac186d4fe5f42362c07a729fcaad80f/src/slave/http.cpp#L123-L139


src/common/http.cpp (lines 562 - 567)


A comment on this condition would be good. When is an executor with 0 
resources allowed? Is this possible for any executor, or just the command 
executor?


- Benjamin Mahler


On Feb. 22, 2017, 5:49 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56938/
> ---
> 
> (Updated Feb. 22, 2017, 5:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Guangya Liu, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7158
> https://issues.apache.org/jira/browse/MESOS-7158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocation role of tasks/executors to the /state v0 API. Unlike
> v1 API where allocation_info is nested in each one of resources, we
> add that information at task/executor level for v0 API for the sake
> of simplicity. Note that resources of one task/executor are guaranteed
> to have same allocation role.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
>   src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 
> 
> Diff: https://reviews.apache.org/r/56938/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56809: Added test for nested container agent reboot case.

[Gilbert Song]

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

(Updated Feb. 22, 2017, 11:17 a.m.)


Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.


Summary (updated)
-

Added test for nested container agent reboot case.


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


Repository: mesos


Description (updated)
---

Added test for nested container agent reboot case.


Diffs (updated)
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
ea01fe55a28d17105157004d8cf0976202a49b7c 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 56945: Removed inconsistent use of override.

[Benjamin Bannier]

> On Feb. 22, 2017, 7:58 p.m., Greg Mann wrote:
> > Is the motivation here to eliminate the clang warnings? IIUC having the 
> > `override` here, while inconsistent, is strictly an improvement from the 
> > previous state without `override`?

Yes, we currently emit warnings in the clang build without being interested in 
fixing this, potentially causing other more interesting warnings to slip 
through (libprocess is not built with `-Werror`). An alternative fix would be 
to use `override` correctly here, i.e., mark all overriding methods with as 
`override` and to drop the now completely redundant `virtual`.

I agree that this is a strict improvement over the previous state, but I feel 
implementing MESOS-4871 instead would be the proper fix instead of introducing 
it incompletely (incompletely both on the code base, and even incompletely on a 
class here).


- Benjamin


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


On Feb. 22, 2017, 7:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56945/
> ---
> 
> (Updated Feb. 22, 2017, 7:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes inconsistent use of the 'override' keyword where the
> 'LibeventSSLSocketImpl' class contained both a method explicitly
> marked 'override' and implicitly 'override' functions. This is
> diagnosed as an inconsistency by clang-4.0, e.g.,
> 
> /PATH/libevent_ssl_socket.hpp|43 col 27| warning: \
>   'connect' overrides a member function but is not marked \
>   'override' [-Winconsistent-missing-override]
>   virtual Future connect(const Address& address);
>   ^
> /PATH/socket.hpp|148 col 27| note: \
>   overridden virtual function is here
>   virtual Future connect(const Address& address) = 0;
> 
> A proper fix will be implemented as part of MESOS-4871.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/56945/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 56830: Introduced `TaskInfo::TerminationPolicy` protobuf.

[Neil Conway]

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




include/mesos/mesos.proto (line 1603)


It isn't clear that "task termination" does not include "when the task is 
explicitly killed by the scheduler." I'd say something like:

"Describes the policy that will be used when a task terminates. This policy 
is only used when a task terminates (i.e., finishes successfully or exits with 
an error); it is not used for tasks that are explicitly killed by the 
scheduler."

You should also explain whether the policy is used when tasks are killed by 
the agent (e.g., due to oversubscription or because of a containerizer error).



include/mesos/mesos.proto (line 1611)


"it is"



include/mesos/mesos.proto (line 1614)


`IGNORE` would be an alternative to `DO_NOTHING`; I don't have a strong 
view on which term is better.



include/mesos/mesos.proto (line 1623)


"The task group will always be killed, regardless of whether the task 
finished successfully or failed -- i.e., transitioned to the TASK_FINISHED or 
TASK_FAILED states, respectively."

Last sentence doesn't make sense to me -- i.e., "zero exit code or failed" 
covers all cases, right?



include/mesos/mesos.proto (line 1629)


"The task group will only be killed when the task fails -- i.e., 
transitions to the TASK_FAILED state. For the default executor, this happens 
when the task exits with a non-zero exit code."

Note that OOM killer (i.e., SIGKILL) does _not_ result in the exit code 
being "unknown" -- see http://unix.stackexchange.com/a/99134


- Neil Conway


On Feb. 22, 2017, 6:16 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> ---
> 
> (Updated Feb. 22, 2017, 6:16 p.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
> https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Describes the termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> ---
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56945: Removed inconsistent use of override.

[Greg Mann]

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



Is the motivation here to eliminate the clang warnings? IIUC having the 
`override` here, while inconsistent, is strictly an improvement from the 
previous state without `override`?

- Greg Mann


On Feb. 22, 2017, 6:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56945/
> ---
> 
> (Updated Feb. 22, 2017, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes inconsistent use of the 'override' keyword where the
> 'LibeventSSLSocketImpl' class contained both a method explicitly
> marked 'override' and implicitly 'override' functions. This is
> diagnosed as an inconsistency by clang-4.0, e.g.,
> 
> /PATH/libevent_ssl_socket.hpp|43 col 27| warning: \
>   'connect' overrides a member function but is not marked \
>   'override' [-Winconsistent-missing-override]
>   virtual Future connect(const Address& address);
>   ^
> /PATH/socket.hpp|148 col 27| note: \
>   overridden virtual function is here
>   virtual Future connect(const Address& address) = 0;
> 
> A proper fix will be implemented as part of MESOS-4871.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/56945/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 56945: Removed inconsistent use of override.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu.


Repository: mesos


Description
---

This removes inconsistent use of the 'override' keyword where the
'LibeventSSLSocketImpl' class contained both a method explicitly
marked 'override' and implicitly 'override' functions. This is
diagnosed as an inconsistency by clang-4.0, e.g.,

/PATH/libevent_ssl_socket.hpp|43 col 27| warning: \
  'connect' overrides a member function but is not marked \
  'override' [-Winconsistent-missing-override]
  virtual Future connect(const Address& address);
  ^
/PATH/socket.hpp|148 col 27| note: \
  overridden virtual function is here
  virtual Future connect(const Address& address) = 0;

A proper fix will be implemented as part of MESOS-4871.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
e589a04d14378f265a8fca871c9f5b0c577f5713 

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


Testing
---

make check (OS X, clang/trunk).


Thanks,

Benjamin Bannier

Re: Review Request 56836: Added test for a task specifying termination policy.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:18 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased


Summary (updated)
-

Added test for a task specifying termination policy.


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


Repository: mesos


Description (updated)
---

Added test for a task specifying termination policy.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp eaf63946735b28b21bfb7e16aca5924a5a82 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56937: Renamed `killingTaskGroup` to `killingGroup` on the default executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This is consistent with the `KILL_GROUP` operation we introduced
on `TerminationPolicy`. In the future, we might have a `KILL_GROUP`
call for killing a task group.


Diffs
-

  src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56835: Made the default executor handle termination policy for a task.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:18 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased.


Summary (updated)
-

Made the default executor handle termination policy for a task.


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


Repository: mesos


Description (updated)
---

Made the default executor handle termination policy for a task.


Diffs (updated)
-

  src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56831: Minor fix to a comment + docs.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:16 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


Summary (updated)
-

Minor fix to a comment + docs.


Repository: mesos


Description (updated)
---

Modified a comment now that the default executor can launch
multiple task groups. Also, updated the nested task groups
docs to reflect this.


Diffs (updated)
-

  docs/nested-container-and-task-group.md 
debf9f82622557aff915dd04ef06a50793f496b6 
  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56832: Added output operator for `TerminationPolicy::Type` enum.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:17 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


Summary (updated)
-

Added output operator for `TerminationPolicy::Type` enum.


Repository: mesos


Description (updated)
---

Added output operator for `TerminationPolicy::Type` enum.


Diffs (updated)
-

  include/mesos/type_utils.hpp c7f86ac3a8c166512410d89678a4dd2622771bf0 
  src/common/type_utils.cpp d86d56d4e1d353d6e82ccff89357bf2abec6eded 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56834: Send a TASK_ERROR update if a task specifies `OnTerminationPolicy`.

[Anand Mazumdar]

> On Feb. 21, 2017, 9:58 p.m., Vinod Kone wrote:
> > src/docker/executor.cpp, line 133
> > 
> >
> > If termination policy is only valid for task group tasks and not normal 
> > tasks, we can do that validation in validation.cpp instead of here.

hmm, a termination policy can be specified for a task too by a custom executor. 
Also, as per our offline discussion, we need to do the validation here on the 
executor rather than on the master/agent.


- Anand


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


On Feb. 20, 2017, 5:01 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56834/
> ---
> 
> (Updated Feb. 20, 2017, 5:01 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker/command executor currently do not support it yet.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
>   src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> Diff: https://reviews.apache.org/r/56834/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56834: Send a TASK_ERROR update if a task specifies `TerminationPolicy`.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:17 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased.


Summary (updated)
-

Send a TASK_ERROR update if a task specifies `TerminationPolicy`.


Repository: mesos


Description
---

The docker/command executor currently do not support it yet.


Diffs (updated)
-

  src/docker/executor.cpp 6c11320fc40ba1eebdbdf95f0a52ac383646d9f8 
  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56833: Added validation for termination policy to the master.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:17 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments


Summary (updated)
-

Added validation for termination policy to the master.


Repository: mesos


Description (updated)
---

Added validation for termination policy to the master.


Diffs (updated)
-

  src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 
  src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f 
  src/tests/master_validation_tests.cpp 
53771f6b5492009fe75cbbfc03a2b542856c1070 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56831: Minor fix to a comment + docs.

[Anand Mazumdar]

> On Feb. 21, 2017, 8:42 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 655
> > 
> >
> > Can you also update the docs that say it only accepts a single 
> > executor? Also, worth calling out in 1.3 CHANGELOG (WIP).

Updated the nested task groups doc. Would update the CHANGELOG in a separate 
patch.


- Anand


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


On Feb. 22, 2017, 6:16 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56831/
> ---
> 
> (Updated Feb. 22, 2017, 6:16 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified a comment now that the default executor can launch
> multiple task groups. Also, updated the nested task groups
> docs to reflect this.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md 
> debf9f82622557aff915dd04ef06a50793f496b6 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56831/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::OnTerminationPolicy` protobuf.

[Anand Mazumdar]

> On Feb. 21, 2017, 9:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1581
> > 
> >
> > also for unknown status (e.g., OOM)
> 
> Jiang Yan Xu wrote:
> Since Mesos never defined tasks as processes only, It's whatever the 
> executor decides, I think it's fine to not define it directly here but rather 
> refer to `TASK_FAILED`. i.e., however TASK_FAILED is defined.
> 
> Would this be clear:
> 
> ```
> // The task group would be only killed when the task failed,
> // i.e., it will be transitioned to TASK_FAILED state.
> ```

Fair enough. Clarified comments specifically for the default executor case.


> On Feb. 21, 2017, 9:50 p.m., Vinod Kone wrote:
> > include/mesos/mesos.proto, line 1558
> > 
> >
> > s/On//
> 
> Jiang Yan Xu wrote:
> I think `OnTerminationPolicy` looks ugly not because of `On` but rather 
> it's a three word term which is a bit mouthful. `OnTermination` or 
> `OnTerminate` by itself is widely used in similar contexts. I started to 
> wonder if the word `Policy` add much additional info. The protobuf is clearly 
> about how to **handle** (i.e., **action** to take on) termination. Maybe none 
> of these words are necessary?

hmm, `OnTermination` and `OnTerminate` look more like handler names and hence 
not quite suitable. I think it's fine to use `TerminationPolicy` here while 
being explicit in the comments to dissambiguate it.


- Anand


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


On Feb. 20, 2017, 5 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56830/
> ---
> 
> (Updated Feb. 20, 2017, 5 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7067
> https://issues.apache.org/jira/browse/MESOS-7067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Describes the on termination policy associated with a task and is
> applied upon a task termination.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
>   include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
> 
> Diff: https://reviews.apache.org/r/56830/diff/
> 
> 
> Testing
> ---
> 
> make check (tests later in the chain)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 56830: Introduced `TaskInfo::TerminationPolicy` protobuf.

[Anand Mazumdar]

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

(Updated Feb. 22, 2017, 6:16 p.m.)


Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.


Changes
---

Review comments + Modified `PROCEED` action to `DO_NOTHING` since it better 
signifies the intent based on offline discussions.

+ Adding Neil as a reviewer too.


Summary (updated)
-

Introduced `TaskInfo::TerminationPolicy` protobuf.


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


Repository: mesos


Description (updated)
---

Describes the termination policy associated with a task and is
applied upon a task termination.


Diffs (updated)
-

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 

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


Testing
---

make check (tests later in the chain)


Thanks,

Anand Mazumdar

Re: Review Request 56754: Implemented a JWT secret generator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56623, 56617, 56618, 56901, 56619, 56812, 56813, 56624, 
56621, 56665, 5, 56667, 56757, 56754]

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. 22, 2017, 2:29 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56754/
> ---
> 
> (Updated Feb. 22, 2017, 2:29 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7000
> https://issues.apache.org/jira/browse/MESOS-7000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can be used to create a 'Secret' from an 'AuthenticationContext'.
> The resulting secret will provide a JWT.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 89fc72b8b4757f6b7a2f090167cca49ff46cff52 
>   src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
>   src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
>   src/tests/secret_generator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56754/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56530: Prevent resource of old agents being allocated to MULTI_ROLE frameworks.

[Jay Guo]

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

(Updated Feb. 23, 2017, 1:59 a.m.)


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


Changes
---

address bmahler's comments


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


Repository: mesos


Description
---

In a mixed cluster, when an old agent (not MULTI_ROLE capable)
registers with new master (MULTI_ROLE capable), it cannot correctly
handle tasks from a MULTI_ROLE framework. Therefore, we should
disallow resources from these agents being allocated to MULTI_ROLE
frameworks.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 

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


Testing
---

./bin/mesos-tests.sh


Thanks,

Jay Guo

Re: Review Request 56837: Added slave capabilities to HierarchicalAllocatorProcess.

[Jay Guo]

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

(Updated Feb. 23, 2017, 1:58 a.m.)


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


Changes
---

address bmahler's comment


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


Repository: mesos


Description
---

Allocator should be aware of slave capabilities in order to make
correct/smarter decisions on resource allocations. The interface
`addSlave` is augmented to pass in agent capabilities and we store
them in HierarchicalAllocatorProcess. Tests are updated accordingly.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
23413d1f9d76009c999b35d2bc98afc52c136404 
  src/master/allocator/mesos/allocator.hpp 
38fbf16a0500fc9f6ce7a498a6af3d81d63fc215 
  src/master/allocator/mesos/hierarchical.hpp 
d33306745a7287b750cb4a5242c7527369d58d65 
  src/master/allocator/mesos/hierarchical.cpp 
eeb44fe89d4bfd26900b11833c1182157e5c7e5c 
  src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
  src/tests/allocator.hpp c741a985660e1540ad8e3a9c387d513247e56714 
  src/tests/api_tests.cpp 378612dd4d038fb4d65fba60a4be00d4950d0c02 
  src/tests/hierarchical_allocator_tests.cpp 
5441fa9d1fad1ca7819038db49c6d88e40571e4a 
  src/tests/master_allocator_tests.cpp 25c67d32eec5fede78eb9fcbc1009eeff7da0dad 
  src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 
  src/tests/persistent_volume_endpoints_tests.cpp 
ec8df33a3f0d4ad790d5cc753e1691c5517c39c0 
  src/tests/reservation_endpoints_tests.cpp 
7432d752120b43560aa96cad8bf3981ee8102e67 
  src/tests/reservation_tests.cpp 309ce8b9acc9131110198c14c655427d7fe9d603 
  src/tests/slave_recovery_tests.cpp 0e295915fea0a7314e173857249bd8726eeccd76 

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


Testing
---

make check


Thanks,

Jay Guo

Review Request 56938: Added allocation role to tasks/executors in v0 /state API.

[Jay Guo]

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

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


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


Repository: mesos


Description
---

Added allocation role of tasks/executors to the /state v0 API. Unlike
v1 API where allocation_info is nested in each one of resources, we
add that information at task/executor level for v0 API for the sake
of simplicity. Note that resources of one task/executor are guaranteed
to have same allocation role.


Diffs
-

  src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
  src/master/http.cpp e2fd71c5ae4178564b9a08756df5175aec5d6ca1 

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


Testing
---

make check


Thanks,

Jay Guo

Review Request 56939: Validate that Resource.allocation_info.roles are not mixed.

[Jay Guo]

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

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


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


Repository: mesos


Description
---

With support for multi-role frameworks, we need to make sure that
individual tasks and executors cannot mix roles. Likewise, we do
not want to allow a scheduler to make a reservation or create a
volume based on resources with different allocated roles.


Diffs
-

  src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 
  src/master/validation.cpp b8c6b49ca5ca1f15e58a2967adc335436a35071f 
  src/tests/master_validation_tests.cpp 
53771f6b5492009fe75cbbfc03a2b542856c1070 

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


Testing
---

make check


Thanks,

Jay Guo

Review Request 56941: Add a test for a MULTI_ROLE master re-registering an old agent.

[Jay Guo]

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

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


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


Repository: mesos


Description
---

When a non-MULTI_ROLE agent with running tasks re-registers, master
should inject allocation roles for tasks/executors sent during
re-registration.


Diffs
-

  src/tests/master_tests.cpp 357a9a47ef87be6cbc4256d8afabe63cd41b1ea1 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 56809: Added test for nested container machine reboot case.

[Jie Yu]

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




src/tests/containerizer/provisioner_docker_tests.cpp (lines 409 - 411)


Why you need to initialize a containerizer? can you just use provisioner?


- Jie Yu


On Feb. 21, 2017, 10:34 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56809/
> ---
> 
> (Updated Feb. 21, 2017, 10:34 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7152
> https://issues.apache.org/jira/browse/MESOS-7152
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for nested container machine reboot case.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> ce57c06d50b47a150ff40412c1fde99f16892434 
> 
> Diff: https://reviews.apache.org/r/56809/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56623, 56617, 56618, 56901, 56619, 56812, 56813, 56624, 
56621, 56665, 5, 56667, 56753]

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. 22, 2017, 2:28 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 22, 2017, 2:28 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> Diff: https://reviews.apache.org/r/56753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56667: Added support for JSON Web Tokens.

[Jan Schlicht]

> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current 
> > implementation isn't compliant with RFCs 7515/7519? The one thing I noticed 
> > was the lack of support for the 'crit' header parameter.
> 
> Jan Schlicht wrote:
> There isn't support for `alg=none` and it is strongly recommended to also 
> support `alg=RS256`. Standard claims aren't validated, though it's up to the 
> specific applications to specify which of these claims are mandatory; it 
> would make sense to validate them as part of a general-purpose JWT 
> implementation. Decoded JSON isn't tested for line breaks, whitespaces, 
> correct UTF-8 encoding.

Sorry, I've read the RFC wrong: We don't have to test the JSON for line breaks, 
but the base64. I'll add support for `alg=none` and the `crit` header.


- Jan


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


On Feb. 22, 2017, 3:26 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated Feb. 22, 2017, 3:26 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56667/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56754: Implemented a JWT secret generator.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:29 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This can be used to create a 'Secret' from an 'AuthenticationContext'.
The resulting secret will provide a JWT.


Diffs (updated)
-

  src/Makefile.am 89fc72b8b4757f6b7a2f090167cca49ff46cff52 
  src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
  src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
  src/tests/secret_generator_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56757: Added the SecretGenerator module interface.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:29 p.m.)


Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This interface will be used by agents to create credentials for the
default executor.


Diffs
-

  include/mesos/authentication/secret_generator.hpp PRE-CREATION 
  include/mesos/module/secret_generator.hpp PRE-CREATION 
  src/Makefile.am c21a073bdbefbb4547e45ed13b7a8a563854fd82 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:28 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This HTTP authenticator extracts a JWT from the requests' authorization
header using the 'Bearer' schema and validates it against a secret using
HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
other claims will be additional labels of the 'AuthenticationContext'.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56667: Added support for JSON Web Tokens.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:26 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

JSON Web Tokens can be used to create claim-based access tokens and is
typically used for HTTP authentication.
This implementation is intended for internal use, e.g. Mesos is supposed
to only parse tokens that it also created. It doesn't fully comply with
RFC 7519. Currently the only supported cryptographic algorithm is HMAC
with SHA-256.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
  3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56666: Added a HMAC SHA256 generator.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:25 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

HMAC SHA256 can be used to create or verify message signatures.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/utilities.hpp 
c2f64a91a505675d568ddf5aa081125e4e32fe17 
  3rdparty/libprocess/src/ssl/utilities.cpp 
8aec613312eee3dd11d9df8c3828a5185407e073 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56665: Added a URL-safe base64 implementation.

[Jan Schlicht]

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

(Updated Feb. 22, 2017, 3:24 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Base64 has many variants that use different alphabets for encoding.
"Base 64 Encoding with URL and Filename Safe Alphabet" is a variant
described in RFC 4648.


Diffs (updated)
-

  3rdparty/stout/include/stout/base64.hpp 
2ac04c4602bc919633a2a480dd2168b7aa301bd7 
  3rdparty/stout/tests/base64_tests.cpp 
32e516861d44c7e3a36e1a29b4d1fe5960684e3b 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56667: Added support for JSON Web Tokens.

[Jan Schlicht]

> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > Regarding the description: I'm curious how exactly the current 
> > implementation isn't compliant with RFCs 7515/7519? The one thing I noticed 
> > was the lack of support for the 'crit' header parameter.

There isn't support for `alg=none` and it is strongly recommended to also 
support `alg=RS256`. Standard claims aren't validated, though it's up to the 
specific applications to specify which of these claims are mandatory; it would 
make sense to validate them as part of a general-purpose JWT implementation. 
Decoded JSON isn't tested for line breaks, whitespaces, correct UTF-8 encoding.


> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, lines 44-48
> > 
> >
> > Do we want to include support for the 'crit' header parameter for spec 
> > compliance?

If JWT parsing is kept internal only, e.g. we only parse JWTs created by the 
secret generator, it's IMHO okay to leave out 'crit' parameter handling, 
because we wouldn't have it in any header.


> On Feb. 20, 2017, 8:36 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/jwt.hpp, line 77
> > 
> >
> > Do you think it's worth using a `JSON::Object` for the header? This 
> > would let the module accommodate arbitrary header keys (AKA 'Private Header 
> > Parameter Names' from RFC-7515), which could be useful for users who want 
> > to use the module for other purposes?

Same as above: It depends on what the scope of this implementation should be. 
An internal-only JWT implementation doesn't need a `JSON::Object` for the 
header, because we control what will be in the header. Of course, if this 
module should be more general-purpose, this would need to be changed, along 
with some other changes. But then we could also strive for full RFC compliance, 
which would mean (among others) support for `alg=none`, probably `alg=RS256` 
and other subtleties.


- Jan


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


On Feb. 16, 2017, 10:35 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated Feb. 16, 2017, 10:35 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56667/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56657: Add support for local resolution of Docker images.

[Santhosh Kumar Shanmugham]

> On Feb. 15, 2017, 11:51 a.m., Mesos Reviewbot wrote:
> > Patch looks great!
> > 
> > Reviews applied: [56657]
> > 
> > Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' 
> > COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 
> > MESOS_VERBOSE=1'; ./support/docker-build.sh

Ping!


- Santhosh Kumar


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


On Feb. 15, 2017, 10:08 a.m., Santhosh Kumar Shanmugham wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56657/
> ---
> 
> (Updated Feb. 15, 2017, 10:08 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-7089
> https://issues.apache.org/jira/browse/MESOS-7089
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosContainerizer's local Docker does not have support for
> mutable tags which can point to different immutable digests. This
> indirection is required when an operator wants to rollout updates
> without affecting the customer's configuration.
> 
> Introduce a new optional agent flag `docker_local_resolution_mapping`
> which will point to a JSON-formatted file, that contains mappings,
> to be used for local resolution of docker images.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 656aaa34915eaee91d388febbc7574287b9f51b5 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 68ce265a5d0a61f8d9ed55dd14c630dcf893a7d2 
>   src/slave/flags.hpp 2c4bd6ae628a272a4c6c2f02670baef011df4505 
>   src/slave/flags.cpp 71935dea0f898d4594de9a5d8a3d1c7fc1a21606 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> ce57c06d50b47a150ff40412c1fde99f16892434 
> 
> Diff: https://reviews.apache.org/r/56657/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Re: Review Request 56904: Documented provisioner auto backend semantic.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56904]

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. 22, 2017, 7:04 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56904/
> ---
> 
> (Updated Feb. 22, 2017, 7:04 a.m.)
> 
> 
> Review request for mesos, Adam B, Avinash sridharan, Jie Yu, and Neil Conway.
> 
> 
> Bugs: MESOS-7154
> https://issues.apache.org/jira/browse/MESOS-7154
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented provisioner auto backend semantic.
> 
> 
> Diffs
> -
> 
>   docs/container-image.md 6740ce154e832647f80e91042e335aad08200e64 
> 
> Diff: https://reviews.apache.org/r/56904/diff/
> 
> 
> Testing
> ---
> 
> github gist
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>