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

[Adam B]

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



Thanks for your patience. Just a few more minor concerns.


src/master/master.cpp (lines 2175 - 2176)


s/cannot be used to authorize `MULTI_ROLE` frameworks/will get an empty 
string value for `MULTI_ROLE` frameworks/



src/master/master.cpp (line 2177)


This could use a reference to MESOS-7073 (especially if we remove the other 
reference in LocalAuthorizer)



src/master/master.cpp (lines 2178 - 2179)


Why not check `if(protobuf::framework::getRoles(frameworkInfo).size() <= 
1)` instead of checking the capability? A legacy authorizer could still 
authorize a multi-role-capable framework if it's only trying to register with a 
single role.


- Adam B


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

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

[Guangya Liu]

> On 二月 8, 2017, 6:48 a.m., Jay Guo wrote:
> > src/tests/hierarchical_allocator_tests.cpp, lines 203-205
> > 
> >
> > IMO, since this is for testing purpose, we probably don't need to 
> > rigidly require test writers to construct frameworkInfo with `MULTI_ROLE` 
> > capability. If `createFrameworkInfo` is called with multiple roles, we 
> > could simply add `MULTI_ROLE` implicitly. And one could still explicitly 
> > add `MULTI_ROLE` to capabilities with a single role. Then we don't need to 
> > change existing test cases and avoid future confusion. What do you think?

Yes, but the only problem is that we cannot specfify multiple roles with the 
first paramter here `const string& role`, so I have to update it to `const 
set& roles`.


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56376/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator test to support create multi role framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> c681d03c3f94f7d071143366a5aad0421108ebec 
> 
> Diff: https://reviews.apache.org/r/56376/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Adam B]

> On Feb. 3, 2017, 2:26 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 244
> > 
> >
> > Seems like framework_info is always set, so how/why would we ever fall 
> > through to the other cases?
> 
> Benjamin Bannier wrote:
> Yes, this is currently dead code. I was also wondering whether it should 
> be removed, but decided against it since it provides some level of redundancy 
> as long as `value` still exists and code in the master and in authorizers 
> might not evolve consistently.
> 
> Do you believe it should be removed?
> 
> Alexander Rojas wrote:
> The main reason the `object->value` is still there, is that the local 
> authorizer is a reference implementation for module writers who want to build 
> their own modules, as such, it does provide a reference. I myself will vote 
> to remove the `value` field if possible. However, we makred as deprecated in 
> November 2016 which means we need it there until June (at the same time it 
> had said it is supposed to be removed on 1.2).
> 
> Adam B wrote:
> Alright. Please add a comment explaining why we're keeping dead code 
> around, so I am not tempted to delete it next time I see it.
> 
> Benjamin Bannier wrote:
> Yes, a comment is in order here, updated the patch. I also slipped in a 
> `TODO` to remove this branch when `value` is purged.

This code is in the LocalAuthorizer, which doesn't support other custom 
authorizers. While Mesos master/agent should continue to set both FrameworkInfo 
and `value`, to support custom authorizers, an authorizer implementation like 
LocalAuthorizer doesn't need to look at `value` if it's already looking at 
FrameworkInfo. Even as a reference implementation, we should not show an 
example of using the deprecated field. I think we can safely remove this entire 
block.
I am, however, happy that we now have MESOS-7073 to track the eventual removal 
of the value field altogether.


- Adam


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


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

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Qian Zhang]

> On Feb. 9, 2017, 7:36 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/oci/paths.hpp, lines 36-39
> > 
> >
> > Can you elaborate a bit more the layout here?
> > 
> > so `layer_id` is the digest? What is `config.json`? Does it store 
> > decompressed content, or the original content? what about image configs?
> > 
> > Given this 
> > https://github.com/opencontainers/image-spec/blob/master/image-layout.md, 
> > my hunch is that we should probably cache the original blob, as well as 
> > decompressed content, potentially in separate subdirectories.
> > 
> > ```
> > 
> >|--- blobs/  # this is origional CAS blobs.
> >|--- sha256/
> >|--- 
> >|--- 
> >|--- ...
> >|--- layers/ # this is decompressed filesystem change sets
> >|--- 
> >|--- ...
> > ```
> > 
> > Looks like we probably do not need `configs` or `manifests` directory 
> > for now because the only media type is json, and can be directly read from 
> > `blobs/`.
> > 
> > Another thing to consider is the conversion to overlayfs opaque file 
> > from whiteout files as you did before. So for `layers` directory, we 
> > probably need to have a `backend` in the path (e.g., 
> > `layers/overlayfs/...`).
> 
> Jie Yu wrote:
> Also, worth taking a look at how Docker (or other OCI system) organize 
> its cache.

Please take a look at https://reviews.apache.org/r/56147/diff/1#4, I have 
changed it to:
```
 |--layers
|--
|-- rootfs
|--
|-- rootfs
|--
```
Here both `` and `` are digest. `/rootfs/` 
stores decompressed content(filesystem) extracted by `command::untar()` from 
the layer tar ball. `` stores the image configuration.

Can you please let me know why we need to cache the original blobs? In my 
current implementation, the orignal blobs will only be fetched by fetcher into 
a temp directory under `staging` directory, `layers` directory will only store 
the decompressed content, and that temp directory under `staging` directory 
will be removed after fetching image is done. I do not think we need the 
original blobs once the fetching image is done and we have cached the image in 
the store.

And actually under `layers/`, there can be two sub-directories: 
`rootfs` (for aufs/bind/copy) and `rootfs.overlay` (for overlay), this is same 
with what I did for for Docker store. Do you think we need to change it to 
`layers/non-overlay/` and `layers/overlay/` for OCI store?


- Qian


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


On Feb. 1, 2017, 9:17 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54639/
> ---
> 
> (Updated Feb. 1, 2017, 9:17 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the 'create()' method of OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Adam B]

> On Feb. 7, 2017, 1:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > 
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?
> 
> Benjamin Bannier wrote:
> There is only a weak relation between this framework capability and the 
> capabilities of authorizers. My thinking here was roughly:
> 
> * The way authorization works ("send at most a single authorization 
> request per authorizable action") all information required for authorization 
> needs to be packed into a single authorization request.
> * There are authorizers relying only on the `value` field to determine 
> the role of a framework; this approach is deprecated.
> * In the current authorization approach there is no meaningful value for 
> `value` for multirole frameworks.
> * Multirole frameworks are a new feature so not updating deprecated 
> features to work with them does not break backwards compatibility.
> 
> With this change we
> 
> * maintain the current, single-request authorization approach,
> * avoid breaking authorizers assuming frameworks can only have a single 
> role,
> * avoid breaking authorizers using `value` and assuming a single-role 
> world, and
> * enable authorizers using `framework_info` to authorize multirole 
> frameworks.
> 
> This is very much in line with how e.g., `FrameworkInfo` has both a 
> (deprecated) `role` and a `roles` field, and the correct action is taken 
> depending on framework capabilities.
> 
> Would we instead e.g., send an authorization request for each role, we 
> wouldn't just introduce new code to use a deprecated field (`value`), but we 
> would also introduce a lot of potential overhead, all while ignoring the 
> existence of `framework_info` which already now bundles all this information 
> and can be sent as part of an authorization request.
> 
> Adam B wrote:
> Thanks for the explanation.
> I agree that sending an authz request for each role is overkill just to 
> support a deprecated field. Forget about that.
> My only concern is that authz decisions generally prefer to deny access 
> when they don't know what to do, and an old authorizer that only looks at the 
> `value` field will try to authorize a Framework registering with no role. Is 
> that the same as registering with `*`? If not, they probably already handle 
> that as an error. If so, they would probably authorize based on `*` (allow), 
> when they should be authorizing based on `foo` and `bar` (deny). If that's a 
> serious concern, I'd rather remove the `value` field or otherwise break their 
> compilation so they are forced to review the API changes.
> 
> Benjamin Bannier wrote:
> > My only concern is that authz decisions generally prefer to deny access 
> when they don't know what to do, and an old authorizer that only looks at the 
> value field will try to authorize a Framework registering with no role. Is 
> that the same as registering with *?
> 
> Yes, for single role frameworks, if no role is set, we currently and in 
> the future will explicitly propagate the default `*` -- that's 
> (unfortunately) part of our `FrameworkInfo` proto API.
> 
> For multirole frameworks we leave `value` unset, and since there is no 
> default value for `value` in the `Object` proto, consumers of this value 
> would get the default value for the string type (empty string). So 
> authorizers relying on `value` and not explicitly checking if `value` has 
> been set, would see different behavior for new multirole-capable frameworks 
> (empty string vs. `*`) which I believe is good and should give them some 
> pause. I'd argue that since we explicitly propagate even an unset single-role 
> framework role as `*` to authorizers, having rules to match the empty string 
> in an authorizer implementation seems unlikely as the empty string is not a 
> valid role value.
> 
> > If not, they probably already handle that as an error. If so, they 
> would probably authorize based on * (allow), when they should be authorizing 
> based on foo and bar (deny). If that's a serious concern, I'd rather remove 
> the value field or otherwise break their compilation so they are forced to 
> review 

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

[Jay Guo]

> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > include/mesos/master/master.proto, line 310
> > 
> >
> > `s/agent_capabilities/capabilities/`?

This name has been used in some of BenM's patches, do you still want to make 
the terminology change?


> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > include/mesos/v1/master/master.proto, line 310
> > 
> >
> > `s/agent_capabilities/capabilities/`

same as above.


> On Feb. 9, 2017, 7:46 a.m., Michael Park wrote:
> > src/master/http.cpp, lines 380-384
> > 
> >
> > I think it makes sense to add:
> > ```cpp
> > static void json(
> > JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
> > {
> >   writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
> > }
> > ```
> > 
> > then let `protobuf::slave::Capabilities` keep track of
> > `std::set capabilities`.
> > 
> > and just do:
> > ```cpp
> > writer->field("capabilities", slave.capabilities.capabilities);
> > ```

Instead of `std::set`, I use `RepeatedPtrField` to store agent capabilities to 
avoid adding more methods to type_utils.hpp


- Jay


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


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

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

[Benjamin Bannier]

> On Feb. 7, 2017, 10:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > 
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?
> 
> Benjamin Bannier wrote:
> There is only a weak relation between this framework capability and the 
> capabilities of authorizers. My thinking here was roughly:
> 
> * The way authorization works ("send at most a single authorization 
> request per authorizable action") all information required for authorization 
> needs to be packed into a single authorization request.
> * There are authorizers relying only on the `value` field to determine 
> the role of a framework; this approach is deprecated.
> * In the current authorization approach there is no meaningful value for 
> `value` for multirole frameworks.
> * Multirole frameworks are a new feature so not updating deprecated 
> features to work with them does not break backwards compatibility.
> 
> With this change we
> 
> * maintain the current, single-request authorization approach,
> * avoid breaking authorizers assuming frameworks can only have a single 
> role,
> * avoid breaking authorizers using `value` and assuming a single-role 
> world, and
> * enable authorizers using `framework_info` to authorize multirole 
> frameworks.
> 
> This is very much in line with how e.g., `FrameworkInfo` has both a 
> (deprecated) `role` and a `roles` field, and the correct action is taken 
> depending on framework capabilities.
> 
> Would we instead e.g., send an authorization request for each role, we 
> wouldn't just introduce new code to use a deprecated field (`value`), but we 
> would also introduce a lot of potential overhead, all while ignoring the 
> existence of `framework_info` which already now bundles all this information 
> and can be sent as part of an authorization request.
> 
> Adam B wrote:
> Thanks for the explanation.
> I agree that sending an authz request for each role is overkill just to 
> support a deprecated field. Forget about that.
> My only concern is that authz decisions generally prefer to deny access 
> when they don't know what to do, and an old authorizer that only looks at the 
> `value` field will try to authorize a Framework registering with no role. Is 
> that the same as registering with `*`? If not, they probably already handle 
> that as an error. If so, they would probably authorize based on `*` (allow), 
> when they should be authorizing based on `foo` and `bar` (deny). If that's a 
> serious concern, I'd rather remove the `value` field or otherwise break their 
> compilation so they are forced to review the API changes.

> My only concern is that authz decisions generally prefer to deny access when 
> they don't know what to do, and an old authorizer that only looks at the 
> value field will try to authorize a Framework registering with no role. Is 
> that the same as registering with *?

Yes, for single role frameworks, if no role is set, we currently and in the 
future will explicitly propagate the default `*` -- that's (unfortunately) part 
of our `FrameworkInfo` proto API.

For multirole frameworks we leave `value` unset, and since there is no default 
value for `value` in the `Object` proto, consumers of this value would get the 
default value for the string type (empty string). So authorizers relying on 
`value` and not explicitly checking if `value` has been set, would see 
different behavior for new multirole-capable frameworks (empty string vs. `*`) 
which I believe is good and should give them some pause. I'd argue that since 
we explicitly propagate even an unset single-role framework role as `*` to 
authorizers, having rules to match the empty string in an authorizer 
implementation seems unlikely as the empty string is not a valid role value.

> If not, they probably already handle that as an error. If so, they would 
> probably authorize based on * (allow), when they should be authorizing based 
> on foo and bar (deny). If that's a serious concern, I'd rather remove the 
> value field or otherwise break their compilation so they are forced to review 
> the API changes.

We are on the `if not` branch.


- Benjamin

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

[Jay Guo]

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

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


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


Changes
---

address mcypark's comments.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

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

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

make check on Ubuntu 14.04


Thanks,

Jay Guo

Re: Review Request 56476: Enabled loading multiple HTTP authenticators in Mesos.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56474, 56475, 56476]

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. 9, 2017, 1:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56476/
> ---
> 
> (Updated Feb. 9, 2017, 1:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Jan Schlicht, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7004
> https://issues.apache.org/jira/browse/MESOS-7004
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the Mesos code to allow master and agent
> to load multiple HTTP authenticator modules.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 
> 
> Diff: https://reviews.apache.org/r/56476/diff/
> 
> 
> Testing
> ---
> 
> Still need to add tests which test the master and agent with multiple 
> authenticator modules loaded.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56374: Enabled revive offer per role.

[Guangya Liu]

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

(Updated 二月 9, 2017, 6:36 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled revive offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
017253cc8f5fbcccd9d4057c5b189f352779513c 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 56139: Removed redundant 'Times(1)' from master validation tests.

[Greg Mann]

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

(Updated Feb. 9, 2017, 6:35 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


Repository: mesos


Description
---

This patch removes calls to `.Times(1)` from `EXPECT_CALL`
invocations in the master validation tests because it is
redundant.


Diffs (updated)
-

  src/tests/master_validation_tests.cpp 
0c2649089d7fd29eb021ac75c71e6a74368577dc 

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


Testing
---

`bin/mesos-tests.sh --gtest_filter="*Validation*"`


Thanks,

Greg Mann

Re: Review Request 56053: Added a 'SECRET' type to the 'Environment' protobuf message.

[Greg Mann]

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

(Updated Feb. 9, 2017, 6:34 a.m.)


Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

This patch adds a field of type `Secret` to the
`Environment` protobuf message, enabling the passing
of secrets into the environments of executors and
tasks. Additional validation and test code is added
as well.


Diffs (updated)
-

  include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
  include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
  src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897 
  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81 
  src/tests/master_validation_tests.cpp 
0c2649089d7fd29eb021ac75c71e6a74368577dc 
  src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Greg Mann]

> On Feb. 3, 2017, 9:10 a.m., Adam B wrote:
> > include/mesos/mesos.proto, lines 1967-1968
> > 
> >
> > What are these two fields used for? Can you give an example to 
> > demonstrate the difference between name and key?

Good call, I added an example here, let me know what you think.


- Greg


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


On Feb. 9, 2017, 6:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Feb. 9, 2017, 6:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
>   include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Greg Mann]

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

(Updated Feb. 9, 2017, 6:33 a.m.)


Review request for mesos, Adam B, Jie Yu, Kapil Arya, Jan Schlicht, and Vinod 
Kone.


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


Repository: mesos


Description
---

This patch adds a new `Secret` protobuf message which
is designed to serve as a generic mechanism for passing
priviledged information within Mesos.


Diffs (updated)
-

  include/mesos/mesos.proto 34a288b6f5c2fca090a7aa7a61798e3255d6663a 
  include/mesos/v1/mesos.proto 6638111d10f4a36cdf91dfce1019871e9839c306 

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


Testing
---

`make check`


Thanks,

Greg Mann

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

[Guangya Liu]

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

(Updated 二月 9, 2017, 6:21 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled `ReviveOffersMessage` support revive per role.


Diffs (updated)
-

  src/master/master.hpp 32915d049a2d91d648b7d33c15ef50c8bc0c72cd 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 

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


Testing
---

make
make check


Thanks,

Guangya Liu

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

[Guangya Liu]

> On 二月 8, 2017, 9:20 p.m., Benjamin Mahler wrote:
> > I'm fine adding this here, but it seems weird that we wold update the 
> > protobuf but not update the scheduler driver. Are we assuming that other 
> > bindings might benefit from this? Are we still updating the old API?

Had some offline discussion with Anand, till now, the Go bindings are the only 
one that use the old style protobuf handlers on the master currently, but they 
would eventually move to use the v1 API too once James merges the changes to 
use the v1 API from the next branch 
(https://github.com/mesos/mesos-go/tree/next) to the master, so I will not 
update the old API, comments?


- Guangya


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


On 二月 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56371/
> ---
> 
> (Updated 二月 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled `ReviveOffersMessage` support revive per role.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 
> 
> Diff: https://reviews.apache.org/r/56371/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 56479: Renamed 'unsetAuthenticator' to 'unsetAuthenticators' in Mesos.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Renamed 'unsetAuthenticator' to 'unsetAuthenticators' in Mesos.


Diffs
-

  src/tests/cluster.cpp 04b70831ec05f715074cd93426c3645572d866ca 
  src/tests/files_tests.cpp 6c6353e406249f021803e83909415e9908ded28c 
  src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
  src/tests/metrics_tests.cpp 27fc35f3bc47fc2905f4ce400b1051307dcacb2e 
  src/tests/registrar_tests.cpp fb693ea24ae0786cdb3fc09b4cc9f47d754d4c6a 

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


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 56478: Renamed 'unsetAuthenticator' to 'unsetAuthenticators' in libprocess.

[Greg Mann]

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

Review request for mesos, Jan Schlicht and Vinod Kone.


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


Repository: mesos


Description
---

Renamed 'unsetAuthenticator' to 'unsetAuthenticators' in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
3a406963c1defc037d58971f2400e40855503fbb 
  3rdparty/libprocess/src/authenticator_manager.hpp 
0dc8fd24b411d649bcc62208bde5784cac4ea997 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 
  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 
  3rdparty/libprocess/src/tests/metrics_tests.cpp 
d7fdb06060b273e16be27a263b5ee268842aa25c 
  3rdparty/libprocess/src/tests/profiler_tests.cpp 
995bd02f6ecce484cd9b2aca355c2707d73d40b2 

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


Testing
---

`make check`


Thanks,

Greg Mann

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

[Guangya Liu]

> On 二月 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3230-3236
> > 
> >
> > Why do we need to validate the role? It seems sufficient to just check 
> > whether it is one of the framework's roles since all of the framework roles 
> > are valid.

Yes, all of the frameworks roles are valid here, but there maybe cases that the 
framework developer set an `invalid role` when setting the `Call:SUPPRESS` in 
the framework? 

I updated the comments a bit here:

```
// There maybe cases that the framework developer set an invalid role
// when constructing `scheduler::Call::Suppress`.
```


> On 二月 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp, lines 3238-3246
> > 
> >
> > " because it is not one of the framework's subscribed roles"
> > 
> > I would suggest two follow ups patches here:
> > 
> > (1) Let's store the roles set within the Framework struct, so that we 
> > don't have to keep re-computing it and can just write:
> > 
> > ```
> > if (framework->roles.count(role.get()) == 0) {
> >   ...
> > }
> > ```
> > 
> > (2) Add a drop overload to avoid custom logging here:
> > 
> > ```
> > drop(framework,
> >   suppress,
> >  "suppression role ' + role.get() + " is not one"
> >  " of the frameworks's subscribed roles");
> > ```

Added some `TODO` here to follow up later.


- Guangya


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


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

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

[Guangya Liu]

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

(Updated 二月 9, 2017, 6:05 a.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


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


Repository: mesos


Description
---

Enabled suppress offer per role.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
71a40537b673e44ecdd5327d9a9f083faa7fc13a 
  src/master/allocator/mesos/allocator.hpp 
e3c86181677302dbfc3b213715196122f96e312c 
  src/master/allocator/mesos/hierarchical.hpp 
896abcdf0727f986eef3a1a9304a0e4847094057 
  src/master/allocator/mesos/hierarchical.cpp 
017253cc8f5fbcccd9d4057c5b189f352779513c 
  src/master/master.cpp 0b65345d48192a1536d43973cf782ade3c1c8163 
  src/tests/allocator.hpp 32c291213d18d1c8fe5d9e8194b92c10716b9961 
  src/tests/hierarchical_allocator_tests.cpp 
c681d03c3f94f7d071143366a5aad0421108ebec 

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


Testing
---

make
make check

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


Thanks,

Guangya Liu

Re: Review Request 56472: Revert "Revert "Use the stout ELF parser to collect rootfs files."".

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56465, 56466, 56472]

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. 8, 2017, 11:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56472/
> ---
> 
> (Updated Feb. 8, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7060
> https://issues.apache.org/jira/browse/MESOS-7060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit 273aca1d2df1553df5fbf27c3e7479d5dc0f5dbb.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/56472/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora, Ubuntu)
> 
> ```
> vagrant@vagrant-ubuntu-trusty-64:~/mesos/build/src$ sudo ./mesos-tests 
> --gtest_filter=*IsolatorTest.ROOT_*
> ...
> [==] 64 tests from 10 test cases ran. (85512 ms total)
> [  PASSED  ] 64 tests.
> ```
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56366: Windows: Refactor executor to use `SharedHandle` semantics.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [56366, 56364, 56363]

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

Error:
2017-02-09 02:37:50 URL:https://reviews.apache.org/r/56366/diff/raw/ 
[5273/5273] -> "56366.patch" [1]
error: patch failed: src/launcher/executor.cpp:140
error: src/launcher/executor.cpp: patch does not apply

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

- Mesos Reviewbot


On Feb. 8, 2017, 9:16 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56366/
> ---
> 
> (Updated Feb. 8, 2017, 9:16 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor now owns the `SharedHandle` for the process and job object,
> so that their lifetime is correctly tied to the executor and their
> handles will be closed properly.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
>   src/launcher/windows/executor.hpp 6f02912c477105819b4c27ddf248b7289799eaa0 
>   src/launcher/windows/executor.cpp b51fde7376c2083119f342ea599b446944ede000 
> 
> Diff: https://reviews.apache.org/r/56366/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Jay Guo]

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

(Updated Feb. 9, 2017, 10:28 a.m.)


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


Changes
---

rebase and address Michael's comment


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


Repository: mesos


Description
---

This test ensures that multi-role framework should receive offers for
each of its roles.


Diffs (updated)
-

  src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 

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


Testing
---

Added a test.

make check GTEST_FILTER="MasterTest.MultiRoleFrameworkReceivesOffers"


Thanks,

Jay Guo

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

[Adam B]

> On Feb. 7, 2017, 1:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > 
> >
> > What does the framework's capability have to do with whether a custom 
> > authorizer can support multi-role authorization?
> > If I have an old authorizer module, and multi-role (phase 1) capable 
> > frameworks, I will only be able to authorize frameworks with a single role 
> > (regardless of capability). For a multi-role framework, I could authorize a 
> > single role from the list, but that essentially provides a back-door for 
> > those frameworks to use other roles. Or I could call the authorizer 
> > multiple times, one for each role. But we wouldn't want to do that for 
> > multi-role capable authorizers. Maybe we need the concept of "module 
> > capabilites" now too?
> > Am I overthinking this?
> 
> Benjamin Bannier wrote:
> There is only a weak relation between this framework capability and the 
> capabilities of authorizers. My thinking here was roughly:
> 
> * The way authorization works ("send at most a single authorization 
> request per authorizable action") all information required for authorization 
> needs to be packed into a single authorization request.
> * There are authorizers relying only on the `value` field to determine 
> the role of a framework; this approach is deprecated.
> * In the current authorization approach there is no meaningful value for 
> `value` for multirole frameworks.
> * Multirole frameworks are a new feature so not updating deprecated 
> features to work with them does not break backwards compatibility.
> 
> With this change we
> 
> * maintain the current, single-request authorization approach,
> * avoid breaking authorizers assuming frameworks can only have a single 
> role,
> * avoid breaking authorizers using `value` and assuming a single-role 
> world, and
> * enable authorizers using `framework_info` to authorize multirole 
> frameworks.
> 
> This is very much in line with how e.g., `FrameworkInfo` has both a 
> (deprecated) `role` and a `roles` field, and the correct action is taken 
> depending on framework capabilities.
> 
> Would we instead e.g., send an authorization request for each role, we 
> wouldn't just introduce new code to use a deprecated field (`value`), but we 
> would also introduce a lot of potential overhead, all while ignoring the 
> existence of `framework_info` which already now bundles all this information 
> and can be sent as part of an authorization request.

Thanks for the explanation.
I agree that sending an authz request for each role is overkill just to support 
a deprecated field. Forget about that.
My only concern is that authz decisions generally prefer to deny access when 
they don't know what to do, and an old authorizer that only looks at the 
`value` field will try to authorize a Framework registering with no role. Is 
that the same as registering with `*`? If not, they probably already handle 
that as an error. If so, they would probably authorize based on `*` (allow), 
when they should be authorizing based on `foo` and `bar` (deny). If that's a 
serious concern, I'd rather remove the `value` field or otherwise break their 
compilation so they are forced to review the API changes.


- Adam


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


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

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

[Benjamin Mahler]

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


Fix it, then Ship it!




Will make some adjustments based on the comment below.


src/master/validation.cpp (lines 1581 - 1593)


I think we can remove these per my previous comments.

For checking against the framework having no roles, this is a special case 
of the reservation role being absent from the framework's roles, which is 
caught below. Given that a framework with no roles would not receive offers, 
there doesn't seem to be a lot of value in special casing the error message for 
this with an additional check of the special case.

For checking against the framework having `{*}` role, it seems odd to check 
for the framework role's being `{*}` when `{*,foo}` has the same problem: the 
framework cannot make a reservation for `*`. So, this a special case of the 
general dynamic reservation check within `resources::validate()` and we can 
remove this check as well.


- Benjamin Mahler


On Feb. 8, 2017, 11:41 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> ---
> 
> (Updated Feb. 8, 2017, 11:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
> https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/master/validation.hpp fc1eaeebd0643b897f4e14aabc76e7d94f0de263 
>   src/master/validation.cpp 37d171512ee54f260aabb6e1071739bcc3769fb0 
>   src/tests/master_validation_tests.cpp 
> 51185031cb67e64cd69ec6ce1c8f722a0c349970 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Adam B]

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


Ship it!




Ship It!

- Adam B


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

Review Request 56476: Enabled loading multiple HTTP authenticators in Mesos.

[Greg Mann]

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

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


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


Repository: mesos


Description
---

This patch updates the Mesos code to allow master and agent
to load multiple HTTP authenticator modules.


Diffs
-

  src/common/http.cpp abfbf7248beb2d4068be06b7f5f829d7617f943e 

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


Testing
---

Still need to add tests which test the master and agent with multiple 
authenticator modules loaded.


Thanks,

Greg Mann

Review Request 56475: Added libprocess tests for multiple authenticators.

[Greg Mann]

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

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


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


Repository: mesos


Description
---

This patch adds the following libprocess tests to ensure
correct behavior when multiple authenticators are loaded
for a single realm:
  HttpAuthenticationTest.UnauthorizedMultipleAuthenticators
  HttpAuthenticationTest.ForbiddenMultipleAuthenticators
  HttpAuthenticationTest.AuthenticatedMultipleAuthenticators


Diffs
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 

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


Testing
---

The following commands were used to test:
`make check`
`3rdparty/libprocess/libprocess-tests --gtest_filter="*MultipleAuthenticators*" 
--gtest_repeat=1`


Thanks,

Greg Mann

Review Request 56474: Added support for multiple authenticators to libprocess.

[Greg Mann]

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

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


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


Repository: mesos


Description
---

This patch updates the `AuthenticatorManager` to allow
multiple authenticators to be set for a single realm.


Diffs
-

  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 

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


Testing
---

Testing information can be found in the subsequent patch in this chain.


Thanks,

Greg Mann

Re: Review Request 56427: Added another sorter test case.

[Neil Conway]

> On Feb. 9, 2017, 12:46 a.m., Michael Park wrote:
> > What's the subsequent tie-breaker if the # of allocations is the same?

It's the client name (see `DRFComparator::operator()`). This test also covers 
that case (not super explicitly though).


- Neil


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


On Feb. 8, 2017, 3:40 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56427/
> ---
> 
> (Updated Feb. 8, 2017, 3:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When two clients have the same share, the sorter uses the total number
> of allocations made to each client as a tiebreaker.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp a57a4fa4dbf7a9184e29b5dca4f636fe65a3 
> 
> Diff: https://reviews.apache.org/r/56427/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56427: Added another sorter test case.

[Michael Park]

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


Ship it!




What's the subsequent tie-breaker if the # of allocations is the same?

- Michael Park


On Feb. 7, 2017, 7:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56427/
> ---
> 
> (Updated Feb. 7, 2017, 7:40 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When two clients have the same share, the sorter uses the total number
> of allocations made to each client as a tiebreaker.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp a57a4fa4dbf7a9184e29b5dca4f636fe65a3 
> 
> Diff: https://reviews.apache.org/r/56427/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [56368, 56362, 56364, 56363]

Error:
Circular dependency detected for review 56363.Please fix the 'depends_on' field.

- Mesos Reviewbot


On Feb. 8, 2017, 9:15 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56368/
> ---
> 
> (Updated Feb. 8, 2017, 9:15 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess: Windows: Remove `CREATE_JOB` parent hook.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 7117a2730ae8666d0b55cbe05162a0b4e843193c 
>   3rdparty/libprocess/src/subprocess.cpp 
> bc1b99b8f84332f42623c35fc2b8d5186ba8af70 
> 
> Diff: https://reviews.apache.org/r/56368/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Michael Park]

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


Ship it!





src/tests/master_tests.cpp (lines 6343 - 6349)


Not sure why you need the `this->` here?


- Michael Park


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

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


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

Re: Review Request 56375: Added a test to ensure offers with different roles cannot be combined.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 6, 2017, 11:34 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56375/
> ---
> 
> (Updated Feb. 6, 2017, 11:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multi-role framework cannot combine offers allocated to different
> roles of that framework in a single launchTask call.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3b4123b49ee32c902a5d2a01fcc7026da21fdd18 
> 
> Diff: https://reviews.apache.org/r/56375/diff/
> 
> 
> Testing
> ---
> 
> Added a test
> make check GTEST_FILTER="MasterTest.LaunchDifferentRoleLost"
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 56466: Capture ELF interpreters in the ldd utility.

[James Peach]

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

(Updated Feb. 8, 2017, 11:51 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

The ELF interpreter (ie. program loader) path is not necessarily
specified in the ELF dependencies section but we still need
to capture is as a dependency of an executable program.


Diffs
-

  src/linux/ldd.cpp dd5c9c55560dc2895763141393fdaf97be66a5a8 

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


Testing (updated)
---

make check (Fedora, Ubuntu)


Thanks,

James Peach

Review Request 56472: Revert "Revert "Use the stout ELF parser to collect rootfs files."".

[James Peach]

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

Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

This reverts commit 273aca1d2df1553df5fbf27c3e7479d5dc0f5dbb.


Diffs
-

  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

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


Testing
---

make check (Fedora, Ubuntu)

```
vagrant@vagrant-ubuntu-trusty-64:~/mesos/build/src$ sudo ./mesos-tests 
--gtest_filter=*IsolatorTest.ROOT_*
...
[==] 64 tests from 10 test cases ran. (85512 ms total)
[  PASSED  ] 64 tests.
```


Thanks,

James Peach

Re: Review Request 56465: Add support for the ELF .interp section.

[James Peach]

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

(Updated Feb. 8, 2017, 11:50 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add support for the ELF .interp section.


Diffs (updated)
-

  3rdparty/stout/include/stout/elf.hpp 2bf91a0c5ea0b0cac74a844345b0ce13777f1edb 

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


Testing
---

make check (Fedora, Ubuntu)


Thanks,

James Peach

Re: Review Request 56466: Capture ELF interpreters in the ldd utility.

[James Peach]

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

(Updated Feb. 8, 2017, 11:50 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
---

Split patch.


Summary (updated)
-

Capture ELF interpreters in the ldd utility.


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


Repository: mesos


Description (updated)
---

The ELF interpreter (ie. program loader) path is not necessarily
specified in the ELF dependencies section but we still need
to capture is as a dependency of an executable program.


Diffs (updated)
-

  src/linux/ldd.cpp dd5c9c55560dc2895763141393fdaf97be66a5a8 

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


Testing
---

make check (Fedora, Ubuntu)

```
vagrant@vagrant-ubuntu-trusty-64:~/mesos/build/src$ sudo ./mesos-tests 
--gtest_filter=*IsolatorTest.ROOT_*
...
[==] 64 tests from 10 test cases ran. (85512 ms total)
[  PASSED  ] 64 tests.
```


Thanks,

James Peach

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

[Michael Park]

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




include/mesos/master/master.proto (line 310)


`s/agent_capabilities/capabilities/`?



include/mesos/v1/master/master.proto (line 310)


`s/agent_capabilities/capabilities/`



src/master/http.cpp (lines 380 - 384)


I think it makes sense to add:
```cpp
static void json(
JSON::StringWriter* writer, const SlaveInfo::Capability& capability)
{
  writer->append(SlaveInfo::Capability::Type_Name(capability.type()));
}
```

then let `protobuf::slave::Capabilities` keep track of
`std::set capabilities`.

and just do:
```cpp
writer->field("capabilities", slave.capabilities.capabilities);
```


- Michael Park


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

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Jie Yu]

> On Feb. 8, 2017, 11:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/oci/paths.hpp, lines 36-39
> > 
> >
> > Can you elaborate a bit more the layout here?
> > 
> > so `layer_id` is the digest? What is `config.json`? Does it store 
> > decompressed content, or the original content? what about image configs?
> > 
> > Given this 
> > https://github.com/opencontainers/image-spec/blob/master/image-layout.md, 
> > my hunch is that we should probably cache the original blob, as well as 
> > decompressed content, potentially in separate subdirectories.
> > 
> > ```
> > 
> >|--- blobs/  # this is origional CAS blobs.
> >|--- sha256/
> >|--- 
> >|--- 
> >|--- ...
> >|--- layers/ # this is decompressed filesystem change sets
> >|--- 
> >|--- ...
> > ```
> > 
> > Looks like we probably do not need `configs` or `manifests` directory 
> > for now because the only media type is json, and can be directly read from 
> > `blobs/`.
> > 
> > Another thing to consider is the conversion to overlayfs opaque file 
> > from whiteout files as you did before. So for `layers` directory, we 
> > probably need to have a `backend` in the path (e.g., 
> > `layers/overlayfs/...`).

Also, worth taking a look at how Docker (or other OCI system) organize its 
cache.


- Jie


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


On Feb. 1, 2017, 1:17 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54639/
> ---
> 
> (Updated Feb. 1, 2017, 1:17 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the 'create()' method of OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Jie Yu]

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




src/Makefile.am (line 951)


Let's align the tailing backslash.



src/Makefile.am (line 1092)


Ditto.



src/slave/containerizer/mesos/provisioner/oci/paths.hpp (lines 36 - 39)


Can you elaborate a bit more the layout here?

so `layer_id` is the digest? What is `config.json`? Does it store 
decompressed content, or the original content? what about image configs?

Given this 
https://github.com/opencontainers/image-spec/blob/master/image-layout.md, my 
hunch is that we should probably cache the original blob, as well as 
decompressed content, potentially in separate subdirectories.

```

   |--- blobs/  # this is origional CAS blobs.
   |--- sha256/
   |--- 
   |--- 
   |--- ...
   |--- layers/ # this is decompressed filesystem change sets
   |--- 
   |--- ...
```

Looks like we probably do not need `configs` or `manifests` directory for 
now because the only media type is json, and can be directly read from `blobs/`.

Another thing to consider is the conversion to overlayfs opaque file from 
whiteout files as you did before. So for `layers` directory, we probably need 
to have a `backend` in the path (e.g., `layers/overlayfs/...`).



src/slave/containerizer/mesos/provisioner/oci/store.cpp (lines 62 - 68)


Joseph has a URI parser chain that parse any URI. We should try to support 
any uri that fetcher supports.
https://reviews.apache.org/r/46580/
https://reviews.apache.org/r/46588/

We should try to land that and use the URI parser here to parse the uri 
into `URI` struct, and call `fetcher->fetch(URI uri)`


- Jie Yu


On Feb. 1, 2017, 1:17 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54639/
> ---
> 
> (Updated Feb. 1, 2017, 1:17 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the 'create()' method of OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 56465: Add support for the ELF .interp section.

[James Peach]

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

Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Add support for the ELF .interp section.


Diffs
-

  3rdparty/stout/include/stout/elf.hpp 2bf91a0c5ea0b0cac74a844345b0ce13777f1edb 

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


Testing
---

make check (Fedora, Ubuntu)


Thanks,

James Peach

Review Request 56466: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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

Review request for mesos, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs
-

  src/linux/ldd.cpp dd5c9c55560dc2895763141393fdaf97be66a5a8 
  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

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


Testing
---

make check (Fedora, Ubuntu)

```
vagrant@vagrant-ubuntu-trusty-64:~/mesos/build/src$ sudo ./mesos-tests 
--gtest_filter=*IsolatorTest.ROOT_*
...
[==] 64 tests from 10 test cases ran. (85512 ms total)
[  PASSED  ] 64 tests.
```


Thanks,

James Peach

Re: Review Request 56367: Stout: Windows: patch `os::killtree` to terminate job objects.

[Andrew Schwartzmeyer]

> On Feb. 8, 2017, 10:53 p.m., Mesos Reviewbot wrote:
> > Bad review!
> > 
> > Reviews applied: [56367, 56362, 56364, 56363]
> > 
> > Error:
> > Circular dependency detected for review 56363.Please fix the 'depends_on' 
> > field.

Reviewbot, that's not enough information. I see no circular dependency in these 
reviews.

67 -> [62]
62 -> [64, 63]
64 -> [63]
63 -> []

63 did depend on 62, but that was corrected over two hours ago. It's the only 
thing I could that would create a circular dependency.
There's a join dependeny where [62, 64] -> [63], but that's not circular and 
shouldn't be a problem.


- Andrew


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


On Feb. 8, 2017, 9:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56367/
> ---
> 
> (Updated Feb. 8, 2017, 9:14 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before we deprecate `os::killtree`, we need to it at least work
> with the new job object semantics of the `WindowsLauncher`.
> Given the PID of the "process tree" to kill, this maps it to the
> name used for the job object, opens a new handle to it, and calls
> `os::kill_job` to terminate it.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
> 
> Diff: https://reviews.apache.org/r/56367/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 7, 2017, 6:54 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56424/
> ---
> 
> (Updated Feb. 7, 2017, 6:54 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This TODO was written before the code below it handled the
> optionality of the state `info` fields.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
> 
> Diff: https://reviews.apache.org/r/56424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 7, 2017, 6:54 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56423/
> ---
> 
> (Updated Feb. 7, 2017, 6:54 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent has an invariant that all tasks and executors have an
> allocation info. This adds CHECKs to ensure this is the case.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
> 
> Diff: https://reviews.apache.org/r/56423/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

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

[Michael Park]

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


Fix it, then Ship it!





src/slave/slave.cpp (line 1564)


`s/const ExecutorInfo& executorInfo_/ExecutorInfo executorInfo/`?



src/slave/slave.cpp (line 1577)


`foreach (Resource& resource, *resources) {`



src/slave/slave.cpp (lines 4603 - 4604)


Indent 4 spaces.


- Michael Park


On Feb. 7, 2017, 6:53 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56422/
> ---
> 
> (Updated Feb. 7, 2017, 6:53 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7077
> https://issues.apache.org/jira/browse/MESOS-7077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-7077, the agent needs to inject allocation info into
> tasks and executors coming from an old master to maintain the
> invariant that all tasks and executors on the agent have an
> allocation info.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
>   src/tests/slave_tests.cpp 7f357a653335b559b5c78d4618926715dc4a63c7 
> 
> Diff: https://reviews.apache.org/r/56422/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56367: Stout: Windows: patch `os::killtree` to terminate job objects.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [56367, 56362, 56364, 56363]

Error:
Circular dependency detected for review 56363.Please fix the 'depends_on' field.

- Mesos Reviewbot


On Feb. 8, 2017, 9:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56367/
> ---
> 
> (Updated Feb. 8, 2017, 9:14 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before we deprecate `os::killtree`, we need to it at least work
> with the new job object semantics of the `WindowsLauncher`.
> Given the PID of the "process tree" to kill, this maps it to the
> name used for the job object, opens a new handle to it, and calls
> `os::kill_job` to terminate it.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/killtree.hpp 
> 2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
> 
> Diff: https://reviews.apache.org/r/56367/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 7, 2017, 6:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56421/
> ---
> 
> (Updated Feb. 7, 2017, 6:38 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor and tasks fields in the agent's structs are publicly
> visible, but yet for correctness it is required that callers made
> modifications through the member functions which carry additional
> logic (e.g. tracking allocated resources). The TODO here is to
> make these private and to provide public views that are const.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 5049eb783b8ad7b9599f20c3701f7d3d654b4491 
> 
> Diff: https://reviews.apache.org/r/56421/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 56218: Added some check tests for default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56447, 56208, 56209, 56210, 56211, 56212, 56213, 56448, 
56449, 56214, 56215, 56217, 56218]

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. 8, 2017, 4:57 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> ---
> 
> (Updated Feb. 8, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
> 
> Diff: https://reviews.apache.org/r/56218/diff/
> 
> 
> Testing
> ---
> 
> Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54650: Added validation for roles in ACCEPT call.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Feb. 7, 2017, 7:34 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> ---
> 
> (Updated Feb. 7, 2017, 7:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd 
> 
> Diff: https://reviews.apache.org/r/54650/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 8, 2017, 8:30 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56284/
> ---
> 
> (Updated Feb. 8, 2017, 8:30 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
> https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies that the layer that is missing from the store will 
> be pulled.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 99e0820fa47303896f70c81cfb91cd34f0474e55 
> 
> Diff: https://reviews.apache.org/r/56284/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

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

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


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

Re: Review Request 56374: Enabled revive offer per role.

[Benjamin Mahler]

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


Fix it, then Ship it!





include/mesos/allocator/allocator.hpp (lines 359 - 365)


```
  /**
   * Revives offers to this framework for the specified role. This is
   * invoked by a framework when it wishes to receive filtered resources
   * immediately or get itself out of a suppressed state.
   *
   * @param role The optional role parameter allows frameworks with multiple
   * roles to do fine-grained revival.
   */
```



src/master/allocator/mesos/hierarchical.cpp (line 1220)


Do we do this parenthetical pluratity often? I don't think we should bother 
since it gets complicated with some english words where the plural form doesn't 
just add an 's'.

This seem fine to me:

```
Revived offers for roles {foo}
```



src/master/master.cpp (lines 4850 - 4870)


Ditto my comments on the supression patch, no need to validate the role 
here. Also the two follow up patches for a `drop()` overload and 
`framework->roles`.


- Benjamin Mahler


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

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

[Benjamin Mahler]

> On Feb. 8, 2017, 9:44 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > 
> >
> > This header doesn't use @param, we can just update the comment above:
> > 
> > ```
> > Informs the allocator to stop sending offers to this framework for the 
> > specified role. If the role is not specified, we will stop sending offers 
> > to this framework for all of its roles.
> > ```

Feel free to use @param, but be sure to update the overall comment for the 
function.


- Benjamin


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


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

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

[Benjamin Mahler]

> On Feb. 6, 2017, 9:15 p.m., Benjamin Mahler wrote:
> > include/mesos/allocator/allocator.hpp, lines 352-353
> > 
> >
> > Hm.. it doesn't look like any of the comments in this header make use 
> > of @param.
> 
> Guangya Liu wrote:
> Do you mean we do not need `@param`? But there are actually many places 
> using such format as 
> https://github.com/apache/mesos/blob/master/include/mesos/allocator/allocator.hpp#L76-L86
>  , comments?

Ah I see now, sounds good I will drop this.


- Benjamin


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


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

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

[Benjamin Mahler]

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


Fix it, then Ship it!





include/mesos/allocator/allocator.hpp (lines 352 - 353)


This header doesn't use @param, we can just update the comment above:

```
Informs the allocator to stop sending offers to this framework for the 
specified role. If the role is not specified, we will stop sending offers to 
this framework for all of its roles.
```



src/master/allocator/mesos/hierarchical.cpp (line 1182)


Feel free to just do s/role(s)/roles/, no need to bother with this IMHO.



src/master/master.cpp (lines 3230 - 3236)


Why do we need to validate the role? It seems sufficient to just check 
whether it is one of the framework's roles since all of the framework roles are 
valid.



src/master/master.cpp (lines 3238 - 3246)


" because it is not one of the framework's subscribed roles"

I would suggest two follow ups patches here:

(1) Let's store the roles set within the Framework struct, so that we don't 
have to keep re-computing it and can just write:

```
if (framework->roles.count(role.get()) == 0) {
  ...
}
```

(2) Add a drop overload to avoid custom logging here:

```
drop(framework,
  suppress,
 "suppression role ' + role.get() + " is not one"
 " of the frameworks's subscribed roles");
```


- Benjamin Mahler


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

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

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:34 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Requested changes.


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


Repository: mesos


Description
---

`os::create_job` now returns a `Try` instead of a raw
`HANDLE`, forcing ownership of the job object handle onto the caller
of the function. `create_job` requires a `std::string name` for the
job object, which is mapped from a PID using `os::name_job`.

The assignment of a process to the job object is now done via
`Try os::assign_job(SharedHandle, pid_t)`.

The equivalent of killing a process tree with job object semantics
is simply to terminate the job object. This is done via
`os::kill_job(SharedHandle)`.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/os.hpp 
b5172fca96c4151f4b1ebb6d343022558f45fc34 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

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

[Andrew Schwartzmeyer]

> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 729
> > 
> >
> > I believe using `BOOL` as a boolean results in a warning.  We often 
> > just compare it to `FALSE`.

Sure, I'll fix it for my changes. However, I'll note I was following earlier 
convention. These `BOOL`s are compared with a `!` all through the existing 
Windows OS code.


- Andrew


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


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

Re: Review Request 56362: Windows: Create the Windows container launcher.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:28 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Fix dependencies.


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


Repository: mesos


Description
---

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

This launcher is then refactored to use Windows' "Job Objects," which
are similar to Linux's cgroups, and enable us to reason about a group
of processes associated with a task/container as a "Job Object"
instead of a root PID and the tree containing its children. The latter
is not a reasonable approach on Windows, and has been the source of
subtle bugs.

The Job Object approach creates a named job with a one-to-one mapping
to the containerizer, and assigns the first process started for the
task to the job object. After being assigned, the Windows kernel
ensures all spawned child processes
(specifically those made with the `CreateProcess` system call) are
also assigned to the named job object. Thus this job object can then
be used to query the process group's resource usage, kill the process
group, and set limits on the process group.

So instead of seeing a process group as a tree and referring to it by
the root process's PID, the `WindowsLauncher` sees a process group as
a named Job Object, the same way the Windows kernel sees it. However,
the containerizer code which interacts with the launcher still refers
to a task group by the singular PID, and so we have a sort of shim
which maps the initial PID to the name of the job object. This is a an
unfortunate consequence of the shared containerizer code being
originally written for POSIX-like systems.

This abstraction sets us up for implementing resource usage limits on
the process group as a whole.


Diffs
-

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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

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

[Andrew Schwartzmeyer]

> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote:
> > I need to check how this is used in the rest of the review chain, but...
> > 
> > Giving the ownership of the HANDLE to the caller may require much larger 
> > changes in the codebase.  You may notice that we simply leak some pid's in 
> > some parts of the codebase.  So we have to make sure we aren't leaking 
> > these shared objects.
> 
> Alex Clemmer wrote:
> I'm not sure I understand, actually.
> 
> Just so we're on the same page, right now we leak the Job Object handles 
> because they're set to kill the corresponding Job when the last handle is 
> closed -- in other words, when the Agent dies. So any time the agent dies, 
> all of our Executors die, too. :(
> 
> One of the goals of this patch is to set the stage so the agent and 
> executor lifecycles _can_ be decoupled, so that when the agent dies, it can 
> recover and reconnect to the running Executors instead of simply killing them 
> all and restarting them.
> 
> This implies that the agent should be managing the lifecycle of the Job 
> Objects, which in particular seems to imply that it is convenient to keep 
> those handles (or _some_ sort of ID) as state.
> 
> Make sense?

What Alex said correctly describes these changes. Instead of leaking the 
`HANDLE` such that the process implicitly obtains ownership of the job object 
(keeping it alive for the liftime of the executor), this patch makes this an 
explicit action by forcing the launcher to own the `SharedHandle` to the job 
object. The lifetime semantics have not changed; it's just been made explicit 
instead of implicit.

I need to note that:
> Giving the ownership of the HANDLE to the caller may require much larger 
> changes in the codebase.

This is inaccurate. The original code already gave the caller ownership of the 
`HANDLE` (unsafely, and implicitly).

As for:
> So we have to make sure we aren't leaking these shared objects.

This is a valid concern. The `SharedHandle` needs to be owned explicitly. I 
believe I made this the case between this patch and 
https://reviews.apache.org/r/56366/; thus this is of utmost concern for review.


> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, lines 718-721
> > 
> >
> > This is a good default.  But we need a way to toggle this behavior, 
> > such that the agent's death does not kill child jobs.
> > 
> > i.e. A Windows version of ChildHook::SETSID
> 
> Alex Clemmer wrote:
> I asked Andy to follow up this patch with a different changeset that 
> decouples the life of the executor from the life of the agent. Since the 
> agent already kills all executors when it dies, I think it makes sense to 
> have one set of patches just adding Mesos Containers support to Windows, and 
> one fixing the semantics of a dying Agent.
> 
> Thoughts?

I don't think that making this behavior togglable belongs in this patch. I 
attempted to retain the existing lifecycle and behavior as closely as possible. 
If you note, in `recover()` there is a `TODO` that states we should attempt to 
reconnect to a possibly still running executor; this is not currently a 
possible scenario, but a later patch should enable this.


> On Feb. 8, 2017, 2:26 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, lines 740-744
> > 
> >
> > There's no `name` argument in this function...

Heh, yeah. This went through quite a few iterations ;) I'll fix, thanks.


- Andrew


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


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

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

[Benjamin Mahler]

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


Ship it!




I'm fine adding this here, but it seems weird that we wold update the protobuf 
but not update the scheduler driver. Are we assuming that other bindings might 
benefit from this? Are we still updating the old API?


src/messages/messages.proto (lines 269 - 271)


How about:

Removes filters for the specified role. If a role is not provided, then 
this this is equivalent to passing all of the framework's subscribed roles.

Do you want to note that this hasn't been added to the v0 API?


- Benjamin Mahler


On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56371/
> ---
> 
> (Updated Feb. 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled `ReviveOffersMessage` support revive per role.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
>   src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 
> 
> Diff: https://reviews.apache.org/r/56371/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56373/
> ---
> 
> (Updated Feb. 7, 2017, 10:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Bugs: MESOS-6638
> https://issues.apache.org/jira/browse/MESOS-6638
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Augmented master `Revive` API to accept `Call::Revive`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d881ad6dba9ba96057988db265faf0b3013c9b05 
>   src/master/master.hpp ceb9bf7e4be7e0e43a166d7706bd0ab4fd9ed578 
>   src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a 
> 
> Diff: https://reviews.apache.org/r/56373/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 56366: Windows: Refactor executor to use `SharedHandle` semantics.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:16 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Fix the patch dependency.


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


Repository: mesos


Description
---

The executor now owns the `SharedHandle` for the process and job object,
so that their lifetime is correctly tied to the executor and their
handles will be closed properly.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
  src/launcher/windows/executor.hpp 6f02912c477105819b4c27ddf248b7289799eaa0 
  src/launcher/windows/executor.cpp b51fde7376c2083119f342ea599b446944ede000 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

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

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:15 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Fix the patch dependency.


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


Repository: mesos


Description
---

Libprocess: Windows: Remove `CREATE_JOB` parent hook.


Diffs
-

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

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


Testing (updated)
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56367: Stout: Windows: patch `os::killtree` to terminate job objects.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:14 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Fix dependency.


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


Repository: mesos


Description
---

Before we deprecate `os::killtree`, we need to it at least work
with the new job object semantics of the `WindowsLauncher`.
Given the PID of the "process tree" to kill, this maps it to the
name used for the job object, opens a new handle to it, and calls
`os::kill_job` to terminate it.


Diffs
-

  3rdparty/stout/include/stout/os/windows/killtree.hpp 
2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56362: Windows: Create the Windows container launcher.

[Andrew Schwartzmeyer]

> On Feb. 8, 2017, 2 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/windows_launcher.cpp, lines 109-127
> > 
> >
> > Why are you removing the CREATE_JOB parent hook 
> > (https://reviews.apache.org/r/56368/ ) instead of using it here?
> > 
> > We may want to use JobObjects for other subprocesses in general, rather 
> > than just the launcher.  In which case, having a CREATE_JOB helper would be 
> > useful.

This lambda can't be done with a `CREATE_JOB` helper, as it captures `[this, 
containerId]` in order to associate the `SharedHandle` of the job object with 
the container in the `WindowsLauncher` containers.


- Andrew


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


On Feb. 8, 2017, 9:11 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56362/
> ---
> 
> (Updated Feb. 8, 2017, 9:11 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-6892
> https://issues.apache.org/jira/browse/MESOS-6892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of deriving `WindowsLauncher` from `PosixLauncher`,
> this commit implements a separate `WindowsLauncher` derived
> from `Launcher` parallel to the `PosixLauncher`.
> 
> This launcher is then refactored to use Windows' "Job Objects," which
> are similar to Linux's cgroups, and enable us to reason about a group
> of processes associated with a task/container as a "Job Object"
> instead of a root PID and the tree containing its children. The latter
> is not a reasonable approach on Windows, and has been the source of
> subtle bugs.
> 
> The Job Object approach creates a named job with a one-to-one mapping
> to the containerizer, and assigns the first process started for the
> task to the job object. After being assigned, the Windows kernel
> ensures all spawned child processes
> (specifically those made with the `CreateProcess` system call) are
> also assigned to the named job object. Thus this job object can then
> be used to query the process group's resource usage, kill the process
> group, and set limits on the process group.
> 
> So instead of seeing a process group as a tree and referring to it by
> the root process's PID, the `WindowsLauncher` sees a process group as
> a named Job Object, the same way the Windows kernel sees it. However,
> the containerizer code which interacts with the launcher still refers
> to a task group by the singular PID, and so we have a sort of shim
> which maps the initial PID to the name of the job object. This is a an
> unfortunate consequence of the shared containerizer code being
> originally written for POSIX-like systems.
> 
> This abstraction sets us up for implementing resource usage limits on
> the process group as a whole.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3a4ace9c8011ac8eec5067cd085fa7fe4166b9ee 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/launcher.hpp 
> 79f6eea0ee8e564e90b36208672df150dbc5d540 
>   src/slave/containerizer/mesos/launcher.cpp 
> 5114c130efbfb252dde1e85c081f5174e66f57af 
>   src/slave/containerizer/mesos/windows_launcher.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/windows_launcher.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/56362/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 56362: Windows: Create the Windows container launcher.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:11 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Squash the copy of the `PosixLauncher` and the refactor into one patch; address 
some review comments.


Summary (updated)
-

Windows: Create the Windows container launcher.


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


Repository: mesos


Description (updated)
---

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

This launcher is then refactored to use Windows' "Job Objects," which
are similar to Linux's cgroups, and enable us to reason about a group
of processes associated with a task/container as a "Job Object"
instead of a root PID and the tree containing its children. The latter
is not a reasonable approach on Windows, and has been the source of
subtle bugs.

The Job Object approach creates a named job with a one-to-one mapping
to the containerizer, and assigns the first process started for the
task to the job object. After being assigned, the Windows kernel
ensures all spawned child processes
(specifically those made with the `CreateProcess` system call) are
also assigned to the named job object. Thus this job object can then
be used to query the process group's resource usage, kill the process
group, and set limits on the process group.

So instead of seeing a process group as a tree and referring to it by
the root process's PID, the `WindowsLauncher` sees a process group as
a named Job Object, the same way the Windows kernel sees it. However,
the containerizer code which interacts with the launcher still refers
to a task group by the singular PID, and so we have a sort of shim
which maps the initial PID to the name of the job object. This is a an
unfortunate consequence of the shared containerizer code being
originally written for POSIX-like systems.

This abstraction sets us up for implementing resource usage limits on
the process group as a whole.


Diffs (updated)
-

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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

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

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:10 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Remove the patch dependency (there is none).


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


Repository: mesos


Description (updated)
---

`os::create_job` now returns a `Try` instead of a raw
`HANDLE`, forcing ownership of the job object handle onto the caller
of the function. `create_job` requires a `std::string name` for the
job object, which is mapped from a PID using `os::name_job`.

The assignment of a process to the job object is now done via
`Try os::assign_job(SharedHandle, pid_t)`.

The equivalent of killing a process tree with job object semantics
is simply to terminate the job object. This is done via
`os::kill_job(SharedHandle)`.


Diffs (updated)
-

  3rdparty/stout/include/stout/windows/os.hpp 
b5172fca96c4151f4b1ebb6d343022558f45fc34 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56363: Stout: Add predicate `find` semantics to `hashmap`.

[Andrew Schwartzmeyer]

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

(Updated Feb. 8, 2017, 9:09 p.m.)


Review request for mesos, Alex Clemmer and Joseph Wu.


Changes
---

Remove patch dependency (there is none).


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


Repository: mesos


Description
---

`Option hashmap::findIf(lambda::function predicate)`
filters `hashmap` values given a predicate (for more advanced kinds of
`Value`, say compare structs on a field).

`bool hashmap::containsIf(lambda::function predicate)`
checks whether a `Value` matching predicate exists in the `hashmap`
(just passes to `findIf` and acts like `hashmap::contains`).


Diffs
-

  3rdparty/stout/include/stout/hashmap.hpp 
539bbfd9250f2f372c2b3211e2379ad2229fa35e 

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 9:06 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Simplified loop.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs (updated)
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 9:06 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Fixed bad rebase.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Ilya Pronin]

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

(Updated Feb. 8, 2017, 8:30 p.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

Fixed raised issues.


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


Repository: mesos


Description (updated)
---

Added a test that verifies that the layer that is missing from the store will 
be pulled.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
99e0820fa47303896f70c81cfb91cd34f0474e55 

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


Testing
---

`make check`


Thanks,

Ilya Pronin

Re: Review Request 54846: Removed `docker exec` when perform health checks in docker executor.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [54846]

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

Error:
2017-02-08 20:14:44 URL:https://reviews.apache.org/r/54846/diff/raw/ 
[8551/8551] -> "54846.patch" [1]
error: patch failed: src/docker/executor.cpp:474
error: src/docker/executor.cpp: patch does not apply

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

- Mesos Reviewbot


On Dec. 18, 2016, 5:30 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54846/
> ---
> 
> (Updated Dec. 18, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, and Lukas Loesche.
> 
> 
> Bugs: MESOS-4812
> https://issues.apache.org/jira/browse/MESOS-4812
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After Mesos could specify namespaces to enter to perform health checks,
> the command health check in docker executor doesn't need to wrap
> command with `docker exec` anymore. Remove `docker exec` aslo fixes
> the problem when escape quote character in command health checks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4 
> 
> Diff: https://reviews.apache.org/r/54846/diff/
> 
> 
> Testing
> ---
> 
> Added new test cases `HealthCheckTest.ROOT_DOCKER_DockerEnvironmentSetup` and 
> run 
> 
> ```
> $ sudo ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest*"
> [--] Global test environment tear-down
> [==] 22 tests from 1 test case ran. (75423 ms total)
> [  PASSED  ] 22 tests.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Andrew Schwartzmeyer]

> On Feb. 7, 2017, noon, Ilya Pronin wrote:
> > src/slave/containerizer/mesos/windows_launcher.cpp, line 105
> > 
> >
> > Systemd on Windows? ;)
> 
> Andrew Schwartzmeyer wrote:
> This iteration is the copy of the existing implementation. It was 
> refactored into the real `WindowsLauncher` in 
> [#56365](https://reviews.apache.org/r/56365/).
> 
> Joseph Wu wrote:
> You should consider squashing #56365 into this review.  You end up 
> modifying the bulk of this copied code, so the diff-of-a-diff ends up 
> distracting from the review.

I could do that. Most these comments below don't make sense in the context of 
this patch, which is, as described, a straight copy of the original 
implementation.


- Andrew


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


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

Re: Review Request 56208: Updated checks library with general check support.

[Gastón Kleiman]

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




src/checks/checker.hpp (line 70)


s/taskID/taskId/

We don't use the name `taskID` anywhere else in Mesos.



src/checks/checker.hpp (line 95)


ditto capitalization of the name.


- Gastón Kleiman


On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated Feb. 8, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56208/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Alexander Rukletsov]

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

I'd vote for keeping `.repair`.


- Alexander


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


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

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56214: Hashed unacknowledged updates by UUID string in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:57 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Update acknowledgements contain UUID as string, a conversion is
anyway necessary. In the future we may not have access to the
original UUID, while string representation is always available in
the protobuf message itself. Hence it seems reasonable to hash
updates by string instead of UUID.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56448: Shortened continuation name in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56447: Added a helper function for creating valid empty `CheckStatusInfo`s.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

If a check for a task has been defined, the corresponding
`check_status` field in each task status must be set to a
valid `CheckStatusInfo` message even if there is no check
status available yet.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 3ba689f30ae080ea9b2a0af4d819dd3e377c7bf5 
  src/common/protobuf_utils.cpp ed84e9ae083c2dc6cd8b49d9ce8dc624a8607b8a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56209: Hashed unacknowledged updates by UUID string in command executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Update acknowledgements contain UUID as string, a conversion is
anyway necessary. In the future we may not have access to the
original UUID, while string representation is always available in
the protobuf message itself. Hence it seems reasonable to hash
updates by string instead of UUID.


Diffs (updated)
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:57 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56217: Added support for general checks to default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:57 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56218: Added some check tests for default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:57 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 

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


Testing
---

Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
make check


Thanks,

Alexander Rukletsov

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56449: Moved health checker closer to container in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


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


Repository: mesos


Description
---

With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs (updated)
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs (updated)
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 4:56 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

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

[Mesos Reviewbot]

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



Patch looks great!

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

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

- Mesos Reviewbot


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

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

> On Feb. 7, 2017, 1:38 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp, line 273
> > 
> >
> > is the equality operator defined for the above statement to be true?

I'm not sure I understand you question, but we've defined equality operator 
ourselves: 
https://github.com/apache/mesos/blob/64863a573caf72510db367e3df0b12a5a235647f/src/common/type_utils.cpp#L453-L462


- Alexander


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


On Feb. 8, 2017, 3:13 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated Feb. 8, 2017, 3:13 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp b60666b86e260bc129ede52cd2eb18911c8fec29 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56208/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56448: Shortened continuation name in default executor.

[Alexander Rukletsov]

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

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


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


Summary (updated)
-

Shortened continuation name in default executor.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56217: Added support for general checks to default executor.

[Alexander Rukletsov]

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

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


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56218: Added some check tests for default executor.

[Alexander Rukletsov]

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

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


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 

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


Testing
---

Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
make check


Thanks,

Alexander Rukletsov

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

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 3:21 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56214: Hashed unacknowledged updates by UUID string in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 8, 2017, 3:20 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Update acknowledgements contain UUID as string, a conversion is
anyway necessary. In the future we may not have access to the
original UUID, while string representation is always available in
the protobuf message itself. Hence it seems reasonable to hash
updates by string instead of UUID.


Diffs (updated)
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56449: Moved health checker closer to container in default executor.

[Alexander Rukletsov]

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

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


Repository: mesos


Description
---

With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.


Diffs
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56448: Augmented continuation name in default executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov