Re: Review Request 60900: Updated Python linter to work with multiple directories.

[Armand Grillet]

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

(Updated July 18, 2017, 5:31 a.m.)


Review request for mesos, Eric Chung and Kevin Klues.


Changes
---

Fixed issues.


Repository: mesos


Description
---

This allows us to have different Pylint configurations
depending on the directory we lint. This will be useful
in the future to lint our standard Python packages and
the list of scripts in support/ differently.


Diffs (updated)
-

  src/python/cli_new/pylint.config  
  support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 


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

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


Testing
---

Updated a file in src/python/cli_new and checked that the linter was working as 
before.


Thanks,

Armand Grillet

Re: Review Request 60847: Add test cases for /slaves, /slave/containers, /frameworks endpoints.

[Greg Mann]

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




src/tests/master_tests.cpp
Lines 2593 (patched)


s/slave_id/slaveId/

here and elsewhere



src/tests/master_tests.cpp
Line 5856 (original), 5979 (patched)


s/termintated/terminated/



src/tests/master_tests.cpp
Lines 6047-6048 (patched)


Let's leave a comment here explaining why we need two different expected 
values.



src/tests/master_tests.cpp
Lines 6048-6076 (patched)


I think that we can find a more elegant way to handle scenarios like this. 
Consider the following:

```
Try frameworkJson1 = JSON::parse(
"{"
"\"id\":\"" + frameworkId1->value() + "\","
"\"name\":\"default\""
"}");

Try frameworkJson2 = JSON::parse(
"{"
"\"id\":\"" + frameworkId2->value() + "\","
"\"name\":\"default\""
"}");

ASSERT_SOME(frameworkJson1);
ASSERT_SOME(frameworkJson2);

// Since frameworks are stored in a hashmap, there is no strict guarantee of
// their ordering when listed. For this reason, we test both possibilities.
if (array->values[0].contains(frameworkJson1.get())) {
  ASSERT_TRUE(array->values[1].contains(frameworkJson2.get()));
} else {
  ASSERT_TRUE(array->values[0].contains(frameworkJson2.get()));
  ASSERT_TRUE(array->values[1].contains(frameworkJson1.get()));
}
```

What do you think? We could do something like this here and elsewhere.



src/tests/master_tests.cpp
Lines 6075-6076 (patched)


```
EXPECT_TRUE(
value->contains(expected1.get()) ||
value->contains(expected2.get()));
```



src/tests/master_tests.cpp
Lines 6114 (patched)


s/master has known that the framework is terminated/master has marked the 
framework as completed/



src/tests/master_tests.cpp
Lines 6120-6121 (patched)


How about:

"Complete the first framework. As a result, it will appear in the 
response's 'completed_frameworks' field."



src/tests/master_tests.cpp
Lines 6125 (patched)


You can remove this comment.



src/tests/master_tests.cpp
Lines 6128 (patched)


You can remove this comment.



src/tests/slave_tests.cpp
Lines 2403-2404 (patched)


Newline here.



src/tests/slave_tests.cpp
Line 2419 (original), 2459 (patched)


s/status/statistics/



src/tests/slave_tests.cpp
Lines 2470-2473 (patched)


I think you mean to say that this is called twice in the first query.

Also, perhaps this comment could mention that we extract the container ID 
for use later on. How about:

"Will be called twice during the first request. We extract the assigned 
container IDs for use when requesting information on a single container."



src/tests/slave_tests.cpp
Line 2421 (original), 2480-2481 (patched)


Newline here.



src/tests/slave_tests.cpp
Lines 2514 (patched)


Both of these `WillOnce` calls are for the first query, right?

How about:

"Will be called twice during the first request."



src/tests/slave_tests.cpp
Lines 2519-2520 (patched)


How about:

"Request information about all containers."


- Greg Mann


On July 14, 2017, 1:43 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60847/
> ---
> 
> (Updated July 14, 2017, 1:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter test cases for '/slaves' and '/frameworks' on
> the master, and '/slave/containers' endpoint on the slave.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 6e6461c2e13c3eb055aa3c2d8ad8e3ac54a1d197 
>   src/tests/slave_tests.cpp 035db18db3a64a9e358c1c54cc18a4bdeb85d8bf 
> 
> 
> 

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

[Benjamin Mahler]

> On July 18, 2017, 4:08 a.m., Benjamin Mahler wrote:
> >

Since these were pretty minor adjustments, I took care of these and committed 
your patch. I didn't add the `dataSizeMb` filter to clean some of the code up, 
so if you want to follow up with a patch for that, that would be much 
appreciated :)


- Benjamin


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/6/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/webui/master/static/agent.html
Lines 167 (patched)


Just to be very clear can we s/Role/Reservation Role/ here? I've seen users 
be confused about the distinction between allocation role and reservation role, 
and we'll be specifying this distinction anyway in the more complete table 
[1](https://docs.google.com/spreadsheets/d/19I3gNn5SvRcQLp2Th5yHGvIfFLfZTM1PWkoEQDuuGG4/edit#gid=0).



src/webui/master/static/agent.html
Lines 179 (patched)


It would clean up a lot of the UI html if we have a size-specific filter 
(i.e. `dataSizeMb`), because all of these `1024*1024`s we're accumulating are 
detracting from readability of the code and we sometimes forget to add them. 
Could you send a follow up patch to add `dataSizeMb` and clean up the 
`1024*1024`s throughout the html pages?



src/webui/master/static/js/controllers.js
Lines 604 (patched)


This is describing the "what", could we describe the "why"? E.g. we 
transform the map into an array for inclusion in ng-repeat

By the way, from what I can tell, we may not need to conver to an array 
(e.g. https://stackoverflow.com/a/15127934) but it seems that the 
recommendation is to always convert to an array as you've done due to some 
issues (I didn't dig in further, you might want to).



src/webui/master/static/js/controllers.js
Lines 605 (patched)


How about `reserved_resources_as_array`? It starts to get really confusing 
if we have variables like `reserved_resources` and `resource_reservations` 
since they're the same information in map vs array form but the names don't 
suggest this?


- Benjamin Mahler


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/6/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60539: Added test for allocated resources per each role in the agent endpoint.

[Benjamin Mahler]

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


Ship it!




Note that we're not adding a test anymore, so the summary needs to be updated.

- Benjamin Mahler


On July 17, 2017, 5:47 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60539/
> ---
> 
> (Updated July 17, 2017, 5:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for allocated resources per each role in the agent endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> 05b505fc4c643f56c7141b6b79f334b000470780 
> 
> 
> Diff: https://reviews.apache.org/r/60539/diff/3/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 60936: Removed stale FetcherProcess::setSpace declaration.

[James Peach]

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

Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Removed the stale declaration of FetcherProcess::setSpace(). The
implementation was previously removed in 6db493c07.


Diffs
-

  src/slave/containerizer/fetcher_process.hpp 
3ed7dc9db5b64b72881249767c0356a3bc5370e7 


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


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Re: Review Request 60791: Add fetcher cache space usage metrics.

[James Peach]

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

(Updated July 18, 2017, 2:13 a.m.)


Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

Add fetcher metrics to track the (constant) size of the cache size
and the (varying) amount of cache space use. The cache size is
published as `containerizer/fetcher/cache_size_total` and the used
space is `containerizer/fetcher/cache_size_used`. Both of these
metrics are in MB to be consistent with other disk space metrics.
We manually convert to fractional MB so that small amounts of space
usage don't get truncated away.


Diffs (updated)
-

  docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
  src/slave/containerizer/fetcher.cpp 6a664e0657a19d27afac98fd5298d6a18aabe43f 
  src/slave/containerizer/fetcher_process.hpp 
3ed7dc9db5b64b72881249767c0356a3bc5370e7 
  src/tests/fetcher_cache_tests.cpp 1c654e511d2079de746ac97a2c2718e1b926768e 


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

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


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Re: Review Request 60915: Enabled filtering of reservations in the agent.

[Benjamin Mahler]

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



Can you have alexander rojas review this patch?

- Benjamin Mahler


On July 17, 2017, 5:45 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60915/
> ---
> 
> (Updated July 17, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by the
> `/state` endpoint in the agent for fields `reserved_resources_full`,
> `reserved_resources` and `reserved_resources_allocated`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 
> 
> 
> Diff: https://reviews.apache.org/r/60915/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60400: Skipped consulting registry if the agent is in the `slaves.recovered`.

[James Peach]

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


Fix it, then Ship it!




Ship It!


src/master/master.cpp
Lines 6185 (patched)


s/regsitering/reregistering/



src/master/master.cpp
Lines 6206 (patched)


I think this would be clearer with an if/else.
```
if (slaves.recovered.contains(...)) {
  ...
  readmit
} else {
  ...
  update registry
  readmit 
}
```


- James Peach


On July 13, 2017, 11:01 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60400/
> ---
> 
> (Updated July 13, 2017, 11:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7711
> https://issues.apache.org/jira/browse/MESOS-7711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Skipped consulting registry if the agent is in the `slaves.recovered`.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp d895154c00eef17511c46ddd0b75922f952707eb 
> 
> 
> Diff: https://reviews.apache.org/r/60400/diff/2/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Will add more tests.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60369: Exposed allocated resources per each role in /state endpoint on agent.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 17, 2017, 5:45 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60369/
> ---
> 
> (Updated July 17, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The JSON key for this information is "reserved_resources_allocated"
> and "unreserved_resources_allocated". Note that this info might differ
> from the master's view about resource allocations.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 
> 
> 
> Diff: https://reviews.apache.org/r/60369/diff/8/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60907: Got rid of executor's `resources` variable in the agent.

[Benjamin Mahler]

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


Ship it!





src/slave/slave.cpp
Line 4626 (original), 4604 (patched)


Interesting, this looks like a bug before:

`Slave::__run` calls `containerizer->update` including the queued tasks, 
and immediately afterwards `Slave::_statusUpdate` calls `containerizer->update` 
without the queued tasks, and the executor becomes sized below what it should 
be!


- Benjamin Mahler


On July 17, 2017, 12:09 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60907/
> ---
> 
> (Updated July 17, 2017, 12:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `allocatedResources()` method is used instead of executor's variable
> `resources`, which doesn't include resources of pending tasks.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 
>   src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
>   src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 
> 
> 
> Diff: https://reviews.apache.org/r/60907/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?
> 
> James Peach wrote:
> > I was thinking to use --enable-libnl3 to check basic libnl3 features, 
> and use --enable_port_mapping_isolator and --enable-network-ports-isolator to 
> check the advanced libnl3 features needed by them respectively.
> 
> At this point, I'd prefer not to change the semantics of this. We have a 
> pretty clear, documented requirement for libnl >= 3.2.6 and if we start 
> accepting different versions, it becomes less clear.
> 
> > So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 
> 
> Yes, that's correct.
> 
> > and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, 
> right?
> 
> `MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed 
> either to the user or to the build. There is a new build conditional named 
> `ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
> included in the build
> 
> Qian Zhang wrote:
> Can you please let me know where `ENABLE_LINUX_ROUTING` is? I do not see 
> it in this patch and in the current Mesos code.

It gets introduced in the next patch 
[r/60492](https://reviews.apache.org/r/60492).


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

> On July 17, 2017, 11:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?
> 
> James Peach wrote:
> > I was thinking to use --enable-libnl3 to check basic libnl3 features, 
> and use --enable_port_mapping_isolator and --enable-network-ports-isolator to 
> check the advanced libnl3 features needed by them respectively.
> 
> At this point, I'd prefer not to change the semantics of this. We have a 
> pretty clear, documented requirement for libnl >= 3.2.6 and if we start 
> accepting different versions, it becomes less clear.
> 
> > So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 
> 
> Yes, that's correct.
> 
> > and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, 
> right?
> 
> `MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed 
> either to the user or to the build. There is a new build conditional named 
> `ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
> included in the build

Can you please let me know where `ENABLE_LINUX_ROUTING` is? I do not see it in 
this patch and in the current Mesos code.


- Qian


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


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jiang Yan Xu]

> On July 17, 2017, 1:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > 
> >
> > I see that `promise == that.promise` here is required by the Multimap 
> > but does it make sense to have `PathInfo` equality depend only on the path?
> > 
> > I don't think `Promise` is supposed to be comparable.
> 
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal 
> values under the same key in a Multimap (or std::multimap).
> 
> This case shouldn't happen with our GC so we should probably insert some 
> hard `CHECK`s for it (in a separate patch) but I don't think removing 
> `promise == that.promise` has anything to do with it, i.e., the problem 
> exists today.
> 
> Jacob Janco wrote:
> Updating and removing the blocking dependency. Actually, I don't think we 
> even use this operator...why not just remove this and add the checks like you 
> said.

I think it's required by Multimap.


- Jiang Yan


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


On July 17, 2017, 5:23 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 17, 2017, 5:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/9/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

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

Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, Vinod 
Kone, and Zhitao Li.


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


Repository: mesos


Description
---

Added protobuf scheme for blkio subsystem in CgroupInfo.


Diffs
-

  include/mesos/mesos.proto ab2a372184b7cfbaf7a38e90f487cba38c3e80b8 


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


Testing
---

make


Thanks,

Gilbert Song

Review Request 60934: Implemented blkio subsystem usage() for resource statistics.

[Gilbert Song]

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

Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, Vinod 
Kone, and Zhitao Li.


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


Repository: mesos


Description
---

Implemented blkio subsystem usage() for resource statistics.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
a2c575cc87a9e08612cf417013dac76ad6de873b 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
6be0f9ed4aa8c1a2273e5808ad54d3a4922c5e8d 


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


Testing
---

make check

Tested with `mesos-execute` and verified that the blkio statistics can be 
collected from the resource statistics endpoint:

Start the master:
sudo ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos

Start the agent:
sudo GLOG_v=1 ./bin/mesos-agent.sh --master=127.0.0.1:5050 
--isolation=cgroups/blkio,docker/runtime,filesystem/linux --work_dir=/tmp 
--image_providers=docker --executor_environment_variables="{}"

Launch `mesos-execute` test framework:
sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
--docker_image=alpine --shell=true --command="while true ; do echo 'hello' > 
test.txt ; done"

Collect the statistics for blkio:
```
vagrant@vagrant-ubuntu-wily-64:~$ curl localhost:5051/monitor/statistics.json | 
python -m json.tool
  % Total% Received % Xferd  Average Speed   TimeTime Time  Current
 Dload  Upload   Total   SpentLeft  Speed
100  1247  100  12470 0  39390  0 --:--:-- --:--:-- --:--:-- 40225
[
{
"executor_id": "test",
"executor_name": "Command Executor (Task: test) (Command: sh -c 'while 
true ;...')",
"framework_id": "39fb6d5c-d5bd-4b18-a632-0a42e417b946-",
"source": "test",
"statistics": {
"blkio": {
"cfq": [
{
"io_merged": [
{
"op": "TOTAL",
"value": 0
}
],
"io_queued": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_bytes": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_time": [
{
"op": "TOTAL",
"value": 0
}
],
"io_serviced": [
{
"op": "TOTAL",
"value": 0
}
],
"io_wait_time": [
{
"op": "TOTAL",
"value": 0
}
]
}
],
"cfq_recursive": [
{
"io_merged": [
{
"op": "TOTAL",
"value": 0
}
],
"io_queued": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_bytes": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_time": [
{
"op": "TOTAL",
"value": 0
}
],
"io_serviced": [
{
"op": "TOTAL",
"value": 0
}
],
"io_wait_time": [
{
"op": "TOTAL",
"value": 0
}
]
}
],
"throttling": [
{
"device": {
 

Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

[Gilbert Song]

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

Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, Vinod 
Kone, and Zhitao Li.


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


Repository: mesos


Description
---

- Data structure for Blkio entities
- Stats helpers for blkio.throttle.io* (generic blkio stats)
- Stats helpers for blkio.io* (CFQ related stats)
- Comments from the kernel blkio doc for helper functions


Diffs
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 60867: Added a method in the agent exposing an executor's allocated resources.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 17, 2017, 12:09 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60867/
> ---
> 
> (Updated July 17, 2017, 12:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to call a method to get the executor's allocated
> resources, instead of call-sites having to each iterate over the
> executor's pending and launched tasks. It turns out that most
> call-sites forgot to account for the pending tasks, and accounting
> for the pending tasks is non-trivial!
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
>   src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 
> 
> 
> Diff: https://reviews.apache.org/r/60867/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60917: Used a real UPID to send `LearnedMessage`.

[Joseph Wu]

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


Ship it!




LGTM.


src/log/network.hpp
Line 247 (original), 248-250 (patched)


I'd prefer this comment to be a `NOTE: ...`

:)


- Joseph Wu


On July 17, 2017, noon, Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60917/
> ---
> 
> (Updated July 17, 2017, noon)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7753
> https://issues.apache.org/jira/browse/MESOS-7753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously an empty UPID `@0.0.0.0:0` was used which could result in
> the message being dropped due to certain security check (MESOS-7401).
> 
> 
> Diffs
> -
> 
>   src/log/network.hpp 0afcfae27394527f7c957e31014cc9a32a4bf63b 
> 
> 
> Diff: https://reviews.apache.org/r/60917/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Megha Sharma]

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

(Updated July 17, 2017, 5:35 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Updated CHANGELOG for changed semantic of recovery.


Diffs
-

  CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
  docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
  docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 


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


Testing
---

Testing done, renders correctly.


Thanks,

Megha Sharma

Review Request 60931: Added test cases for framework events.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Greg Mann, and Zhitao Li.


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


Repository: mesos


Description
---

Added test cases for 'FRAMEWORK_ADDED', 'FRAMEWORK_UPDATED' and
'FRAMEWORK_REMOVED' events in v1 operator API.


Diffs
-

  src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f 


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


Testing
---

make check


Thanks,

Quinn Leng

Review Request 60930: Added 'FRAMEWORK_REMOVED' event for master streaming api.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Greg Mann, and Zhitao Li.


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


Repository: mesos


Description
---

Added 'FRAMEWORK_REMOVED' event for master streaming api.


Diffs
-

  include/mesos/master/master.proto 7a722a6deac484f28a85f63fd68ef49cb6a47606 
  include/mesos/v1/master/master.proto cf88ea64fa08e4bdcf685de4def28d374eb32fdf 
  src/common/protobuf_utils.hpp 2156f6d158bfee16d29aa3531bdd156af581df01 
  src/common/protobuf_utils.cpp 4e5ab02c90ad8490efbcc471f7fd7c48be0c4678 
  src/master/master.cpp d895154c00eef17511c46ddd0b75922f952707eb 


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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

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

(Updated July 18, 2017, 12:23 a.m.)


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


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


Repository: mesos


Description
---

- Previously, rmdir operations were serialized
  through the garbage collector. Expensive removal
  events had the potential to delay task launch.
- MESOS-6549


Diffs
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 


Diff: https://reviews.apache.org/r/53479/diff/9/


Testing
---

`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
--gtest_break_on_failure`


Thanks,

Jacob Janco

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

> On July 17, 2017, 8:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > 
> >
> > I see that `promise == that.promise` here is required by the Multimap 
> > but does it make sense to have `PathInfo` equality depend only on the path?
> > 
> > I don't think `Promise` is supposed to be comparable.
> 
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal 
> values under the same key in a Multimap (or std::multimap).
> 
> This case shouldn't happen with our GC so we should probably insert some 
> hard `CHECK`s for it (in a separate patch) but I don't think removing 
> `promise == that.promise` has anything to do with it, i.e., the problem 
> exists today.

Updating and removing the blocking dependency. Actually, I don't think we even 
use this operator...why not just remove this and add the checks like you said.


- Jacob


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


On July 15, 2017, 1:23 a.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 15, 2017, 1:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Added: https://reviews.apache.org/r/60893/
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/8/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jacob Janco]

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

(Updated July 18, 2017, 12:22 a.m.)


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


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


Repository: mesos


Description (updated)
---

- Previously, rmdir operations were serialized
  through the garbage collector. Expensive removal
  events had the potential to delay task launch.
- MESOS-6549


Diffs (updated)
-

  src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
  src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 


Diff: https://reviews.apache.org/r/53479/diff/9/

Changes: https://reviews.apache.org/r/53479/diff/8-9/


Testing
---

`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
--gtest_break_on_failure`


Thanks,

Jacob Janco

Review Request 60929: Added 'FRAMEWORK_UPDATED' event for master streaming event.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Greg Mann, Vinod Kone, and Zhitao Li.


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


Repository: mesos


Description
---

Added event 'FRAMEWORK_UPDATED' for master's v1 streaming operator API.
It will be generated when the framework re-registers with the master.


Diffs
-

  include/mesos/master/master.proto 7a722a6deac484f28a85f63fd68ef49cb6a47606 
  include/mesos/v1/master/master.proto cf88ea64fa08e4bdcf685de4def28d374eb32fdf 
  src/common/protobuf_utils.hpp 2156f6d158bfee16d29aa3531bdd156af581df01 
  src/common/protobuf_utils.cpp 4e5ab02c90ad8490efbcc471f7fd7c48be0c4678 
  src/master/master.cpp d895154c00eef17511c46ddd0b75922f952707eb 


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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 60890: WIP: Defined API for launching standalone containers.

[Joseph Wu]

> On July 17, 2017, 12:23 p.m., James DeFelice wrote:
> > Some questions about this API:
> > 
> > 1. If a launch request times out, how might someone query the state of the 
> > requested launch operation/container?
> > 2. When a stand alone container terminates (either success or failure), who 
> > is notified and when?
> > 3. What are the expected failure modes/codes for this API and what are the 
> > recovery semantics?

Without this follow-up feature 
(https://issues.apache.org/jira/browse/MESOS-7492 ), standalone containers will 
basically be launch-and-forget.

In fact, I may want to discard this API entirely as the DaemonManager component 
will supposedly have a more comprehensive API for launching standalone 
containers.


- Joseph


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


On July 14, 2017, 5:41 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60890/
> ---
> 
> (Updated July 14, 2017, 5:41 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Launching a standalone container is very similar to launching a
> nested container, except that the caller does not specify a
> ContainerID (which will be auto-generated) and must specify some
> Resources.  The CommandInfo and ContainerInfo fields behave similarly.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 9bac9541acd24e1123ca5dd5925e2a1381d13b4a 
>   include/mesos/v1/agent/agent.proto ea9282cf12fbe1c2ddeaa37223e4811685263734 
> 
> 
> Diff: https://reviews.apache.org/r/60890/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> See later patches in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 60928: Added 'FRAMEWORK_ADDED' event for master streaming event.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Greg Mann, Vinod Kone, and Zhitao Li.


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


Repository: mesos


Description
---

Added event 'FRAMEWORK_ADDED' for master's v1 streaming operator API.


Diffs
-

  include/mesos/master/master.proto 7a722a6deac484f28a85f63fd68ef49cb6a47606 
  include/mesos/v1/master/master.proto cf88ea64fa08e4bdcf685de4def28d374eb32fdf 
  src/common/protobuf_utils.hpp 2156f6d158bfee16d29aa3531bdd156af581df01 
  src/common/protobuf_utils.cpp 4e5ab02c90ad8490efbcc471f7fd7c48be0c4678 
  src/master/master.cpp d895154c00eef17511c46ddd0b75922f952707eb 


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


Testing
---

make check


Thanks,

Quinn Leng

Re: Review Request 60236: Linted support/verify-reviews.py.

[Joseph Wu]

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


Ship it!




LGTM.


support/verify-reviews.py
Lines 3-4 (original), 19 (patched)


I'm going to expand this a bunch because this script is a bit more widely 
used now (and I was the last one to shepherd a change to this script).


- Joseph Wu


On June 26, 2017, 8:28 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60236/
> ---
> 
> (Updated June 26, 2017, 8:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/verify-reviews.py 391bef5c15a7399f037e54600d1b13c9bd261811 
> 
> 
> Diff: https://reviews.apache.org/r/60236/diff/3/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Megha Sharma]

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

(Updated July 18, 2017, 12:02 a.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: 6223
https://issues.apache.org/jira/browse/6223


Repository: mesos


Description
---

Updated CHANGELOG for changed semantic of recovery.


Diffs (updated)
-

  CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
  docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
  docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 


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

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


Testing
---


Thanks,

Megha Sharma

Re: Review Request 60887: Decoupling `Docker::rm` from `Docker::stop` in agent recovery.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/docker.cpp
Line 1062 (original), 1062 (patched)


I would mention here that we call `docker stop` first so that the container 
can be gracefully shutdown. No matter what the result is, we call `docker rm 
-f` after that to cleanup the container. This will send a SIGKILL to the 
contianer if the container is still running.


- Jie Yu


On July 15, 2017, 12:28 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60887/
> ---
> 
> (Updated July 15, 2017, 12:28 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-
> https://issues.apache.org/jira/browse/MESOS-
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes `Docker::stop()` and `Docker::rm()` to explicit calls
> during agent recovery (which is the only place using this coupling).
> This decoupling avoids a `lambda::bind()` in `Docker::stop()` and thus
> enables us to mock `Docker::rm()` in `MockDocker`.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 5593cb635e073334c6c2566be3d803cd7febb1c3 
>   src/docker/docker.cpp 8081c0203bf62cf62aa3b93d745f0e829ad65509 
>   src/slave/containerizer/docker.cpp 2fe92272d7ac6d916371c55affe24598255f10eb 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 1e85a79f812399270575ea4a64db10e72f40e648 
>   src/tests/mock_docker.hpp 59873646be494c8fe6aebf5ede595d77e3ac4cae 
>   src/tests/mock_docker.cpp 0ed63862f5c07935f088282157979eafdc814084 
> 
> 
> Diff: https://reviews.apache.org/r/60887/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[James Peach]

> On July 17, 2017, 3:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.
> 
> Qian Zhang wrote:
> I was thinking to use `--enable-libnl3` to check basic libnl3 features, 
> and use `--enable_port_mapping_isolator` and 
> `--enable-network-ports-isolator` to check the advanced libnl3 features 
> needed by them respectively.
> 
> So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
> `--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
> `--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
> user, right?

> I was thinking to use --enable-libnl3 to check basic libnl3 features, and use 
> --enable_port_mapping_isolator and --enable-network-ports-isolator to check 
> the advanced libnl3 features needed by them respectively.

At this point, I'd prefer not to change the semantics of this. We have a pretty 
clear, documented requirement for libnl >= 3.2.6 and if we start accepting 
different versions, it becomes less clear.

> So in your solution, there will be only 2 --enable-xxx flags (i.e., 
> --enable_port_mapping_isolator, --enable-network-ports-isolator) and 
> --with-nl, 

Yes, that's correct.

> and MESOS_HAVE_LIBNL3 is internal which is not exposed to the user, right?

`MESOS_HAVE_LIBNL3` is just the autoconf macro name, so it's not exposed either 
to the user or to the build. There is a new build conditional named 
`ENABLE_LINUX_ROUTING` which causes the `src/linux/routing` files to be 
included in the build


- James


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


On July 16, 2017, 11:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 16, 2017, 11:43 p.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

[Qian Zhang]

> On July 18, 2017, 7:12 a.m., Avinash sridharan wrote:
> > src/messages/flags.proto
> > Lines 56 (patched)
> > 
> >
> > Had an internal discussion on this with Jie, and seems like having 
> > different DNS options for CNI and CNM is not something we need to worry 
> > about, so we should merge the fields into one.

OK, so the protobuf message should be like the below, right?
```
message ContainerDNS {
  message DNSInfo {
// Specify CNI network name or CNM network name as the value of
// of this field. For CNM host network, its name is `host`, for
// CNM default bridge network, its name is `bridge`, for a CNM
// user-defined network, its name is specified by:
// `ContainerInfo.network_infos(0).name`.
required string network = 1;

// For CNI network, all four fields in `slave.cni.spec.DNS` are
// supported, but for CNM network, we only support three fields:
// `nameservers`, `search` and `options` but not `domain` since
// Docker only has `--dns`, `--dns-search`, `--dns-option` options.
required slave.cni.spec.DNS dns = 2;
  }

  repeated DNSInfo dns = 1;
}
```


- Qian


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


On July 5, 2017, 3:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 5, 2017, 3:03 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 
>   src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Joseph Wu]

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




support/test-upgrade.py
Lines 114-115 (original), 142-147 (patched)


This refactoring doesn't retain the original logic.

The script does the following:

1) Launch the Master/Agent/Framework of version A.
2) Kill Agent A, kill Master A, start Master B, start Agent A, start 
Framework A.  The agent is only restarted here due to how the 
StandaloneMasterDetector is implemented.
3) Kill Agent A, start Agent B, start Framework A.
4) Start Framework B.

This refactoring will cause the master to be killed twice more than the 
original and the agent to be killed once more.

I think it would be fine to make 4 new methods for each of the test steps 
(start_cluster, upgrade_master, upgrade_agent, upgrade_framework).


- Joseph Wu


On June 26, 2017, 8:48 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 26, 2017, 8:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

[Avinash sridharan]

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




src/messages/flags.proto
Lines 56 (patched)


Had an internal discussion on this with Jie, and seems like having 
different DNS options for CNI and CNM is not something we need to worry about, 
so we should merge the fields into one.


- Avinash sridharan


On July 5, 2017, 7:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 5, 2017, 7:03 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 
>   src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60925]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 3:10 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60925/
> ---
> 
> (Updated July 17, 2017, 3:10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: 6223
> https://issues.apache.org/jira/browse/6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated CHANGELOG for changed semantic of recovery.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
>   docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
>   docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 
> 
> 
> Diff: https://reviews.apache.org/r/60925/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 60822: Added filtering to /slaves, /slave/containers and /frameworks endpoints.

[Greg Mann]

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




src/master/http.cpp
Line 529 (original), 537-538 (patched)


`&` should go next to the type:

```
  const Owned& authorizeRole_;
  const SlaveIDAcceptor& selectSlaveId_;
```



src/master/http.cpp
Line 2467 (original), 2482 (patched)


Indent this two more spaces.



src/master/http.cpp
Line 2793 (original), 2798 (patched)


Indent this two more spaces.



src/slave/http.cpp
Lines 2109 (patched)


Can we get rid of the `Owned` here?



src/slave/http.cpp
Lines 2131 (patched)


Since `AuthorizationAcceptor::accept()` returns a `bool`, the `Try` is not 
necessary here. You could combine this check with the other two in the `if` 
statement above.


- Greg Mann


On July 14, 2017, 12:13 a.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60822/
> ---
> 
> (Updated July 14, 2017, 12:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-7630
> https://issues.apache.org/jira/browse/MESOS-7630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added query parameter support for the '/master/slaves',
> '/master/frameworks' and '/slave/containers' endpoints.
> 
> This allows slaves, frameworks and containers to be queried
> by ID.
> 
> If no ID is specified, all records are returned, consistent
> with current behavior.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 948aa118101b6ce03410c9e0c945b6ca16668ca2 
>   src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
>   src/slave/http.cpp 3070b3b06a9d9e75b9d5afb255098e63a1056ec2 
> 
> 
> Diff: https://reviews.apache.org/r/60822/diff/4/
> 
> 
> Testing
> ---
> 
> Passed 'make check -j48'
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

[Jie Yu]

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




src/resource_provider/manager.cpp
Lines 189 (patched)


kill this line



src/resource_provider/manager.cpp
Lines 303 (patched)


`self().id` is not a uuid (unlike the master id).


- Jie Yu


On July 11, 2017, 12:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60771/
> ---
> 
> (Updated July 11, 2017, 12:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using HTTP, resource providers are expected to subscribe to the resource
> provider manager. The resource provider manager will assign a unique ID
> to subscribed resource providers and use events to communicate with
> them.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
>   src/tests/resource_provider_http_api_tests.cpp 
> 46a3f37ba046201e9d79e8ba037da2cb37f5c31f 
> 
> 
> Diff: https://reviews.apache.org/r/60771/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60900: Updated Python linter to work with multiple directories.

[Eric Chung]

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




support/mesos-style.py
Lines 309-319 (original), 325-335 (patched)


this can be done with less duplication:

```
process = subprocess.Popen(
[('. {virtualenv_dir}/bin/activate;'
  ' PYTHONPATH={lib_dir}:{bin_dir} pylint'
  ' --rcfile={pylint} --ignore={ignore} {files}').
 format(files=source_files,
**source_config)],
shell=True, stdout=subprocess.PIPE)
```


- Eric Chung


On July 16, 2017, 7:34 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60900/
> ---
> 
> (Updated July 16, 2017, 7:34 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to have different Pylint configurations
> depending on the directory we lint. This will be useful
> in the future to lint our standard Python packages and
> the list of scripts in support/ differently.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/pylint.config  
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60900/diff/1/
> 
> 
> Testing
> ---
> 
> Updated a file in src/python/cli_new and checked that the linter was working 
> as before.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60900: Updated Python linter to work with multiple directories.

[Eric Chung]

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




support/mesos-style.py
Line 357 (original), 372 (patched)


this will crap out because self.source_dirs has been changed to a dict


- Eric Chung


On July 16, 2017, 7:34 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60900/
> ---
> 
> (Updated July 16, 2017, 7:34 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to have different Pylint configurations
> depending on the directory we lint. This will be useful
> in the future to lint our standard Python packages and
> the list of scripts in support/ differently.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/pylint.config  
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60900/diff/1/
> 
> 
> Testing
> ---
> 
> Updated a file in src/python/cli_new and checked that the linter was working 
> as before.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Megha Sharma]

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

(Updated July 17, 2017, 10:10 p.m.)


Review request for mesos and Jiang Yan Xu.


Bugs: 6223
https://issues.apache.org/jira/browse/6223


Repository: mesos


Description
---

Updated CHANGELOG for changed semantic of recovery.


Diffs
-

  CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
  docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
  docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 


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


Testing
---


Thanks,

Megha Sharma

Review Request 60925: Updated CHANGELOG for changed semantic of recovery.

[Megha Sharma]

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

Review request for mesos.


Repository: mesos


Description
---

Updated CHANGELOG for changed semantic of recovery.


Diffs
-

  CHANGELOG 978747fc11c90a0d73d6815823474f974bc0c82e 
  docs/agent-recovery.md 1b3f2d2d2e630478a8d577eaa6474e3aedc3cb89 
  docs/upgrades.md dda55f90e2716e805fb145a369fe248cc178f5ac 


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


Testing
---


Thanks,

Megha Sharma

Re: Review Request 60234: Linted support/push-commits.py.

[Joseph Wu]

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


Ship it!




I'm a committer, but I have not used this script before :)

Anyway, style looks better than before.


support/push-commits.py
Lines 37-49 (original), 54-66 (patched)


Hmmm... This logic seems easy to trick.  (i.e. If a commit message contains 
multiple `Review: ...` lines.)

I'll open a separate issue to fix it: 
https://issues.apache.org/jira/browse/MESOS-7802


- Joseph Wu


On June 26, 2017, 8:49 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60234/
> ---
> 
> (Updated June 26, 2017, 8:49 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/push-commits.py 4835d67393aaf3a38a1da90ccc7c89a952bf9270 
> 
> 
> Diff: https://reviews.apache.org/r/60234/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60233: Linted support/post-reviews.py.

[Joseph Wu]

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


Ship it!




Linting LGTM.


support/post-reviews.py
Lines 36-37 (patched)


To my knowledge, everyone uses a `.reviewboardrc` file instead of 
specifying these options directly.  So I'll update the description to reflect 
this state of affairs.


- Joseph Wu


On June 27, 2017, 1:24 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60233/
> ---
> 
> (Updated June 27, 2017, 1:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py 410fb3371a3c46adbfd68c7584ffd6cf3b3010d1 
> 
> 
> Diff: https://reviews.apache.org/r/60233/diff/3/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60899: Moved new Mesos CLI to src/python/.

[Eric Chung]

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


Ship it!




otherwise lgtm, ship it!

- Eric Chung


On July 16, 2017, 7:33 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60899/
> ---
> 
> (Updated July 16, 2017, 7:33 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change allows to have all the Python files under
> only two directories : src/python/ and support/. This
> will improve the structure of our Python code in the future
> by sharing configuration files and virtual environments.
> 
> 
> Diffs
> -
> 
>   src/cli_new/.gitignore 3ce5f74748dae6ac60170a1f0c73191fc26177d9 
>   src/cli_new/README.md  
>   src/cli_new/activate  
>   src/cli_new/bin/main.py  
>   src/cli_new/bin/mesos  
>   src/cli_new/bin/mesos-cli-tests  
>   src/cli_new/bin/settings.py  
>   src/cli_new/bootstrap 4193d3f38961747a6f5e60a8cd1715eff76d660e 
>   src/cli_new/deactivate  
>   src/cli_new/lib/cli/__init__.py  
>   src/cli_new/lib/cli/config.py  
>   src/cli_new/lib/cli/docopt.py  
>   src/cli_new/lib/cli/exceptions.py  
>   src/cli_new/lib/cli/http.py  
>   src/cli_new/lib/cli/plugins/__init__.py  
>   src/cli_new/lib/cli/plugins/base.py  
>   src/cli_new/lib/cli/plugins/config/__init__.py  
>   src/cli_new/lib/cli/plugins/config/main.py  
>   src/cli_new/lib/cli/tests/__init__.py  
>   src/cli_new/lib/cli/tests/base.py 62a4ee8ac2d39c8ecd400560aa51485cab0410f2 
>   src/cli_new/lib/cli/tests/constants.py  
>   src/cli_new/lib/cli/tests/tests.py  
>   src/cli_new/lib/cli/util.py  
>   src/cli_new/mesos.bash_completion  
>   src/cli_new/pip-requirements.txt  
>   src/cli_new/pylint.config  
>   src/cli_new/tests/main.py  
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60899/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> ```
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

[Jie Yu]

> On July 17, 2017, 9:50 p.m., Jie Yu wrote:
> >

Sorry, I accidentally clicked "publish". I have more comments to come.


- Jie


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


On July 11, 2017, 12:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60771/
> ---
> 
> (Updated July 11, 2017, 12:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using HTTP, resource providers are expected to subscribe to the resource
> provider manager. The resource provider manager will assign a unique ID
> to subscribed resource providers and use events to communicate with
> them.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
>   src/tests/resource_provider_http_api_tests.cpp 
> 46a3f37ba046201e9d79e8ba037da2cb37f5c31f 
> 
> 
> Diff: https://reviews.apache.org/r/60771/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60771: Implemented the 'SUBSCRIBE' call in the resource provider manager.

[Jie Yu]

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




src/resource_provider/manager.cpp
Lines 109 (patched)


2 lines apart



src/resource_provider/manager.cpp
Lines 120 (patched)


2 lines apart.



src/resource_provider/manager.cpp
Lines 134 (patched)


Can we use `Owned` pointer here so that you don't need to release them in 
destructor.



src/resource_provider/manager.cpp
Lines 147 (patched)


No need for this is ResourceProviderID is UUID



src/resource_provider/manager.cpp
Line 64 (original), 299-305 (patched)


Let's use UUID for resource provider id.


- Jie Yu


On July 11, 2017, 12:45 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60771/
> ---
> 
> (Updated July 11, 2017, 12:45 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-7780
> https://issues.apache.org/jira/browse/MESOS-7780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using HTTP, resource providers are expected to subscribe to the resource
> provider manager. The resource provider manager will assign a unique ID
> to subscribed resource providers and use events to communicate with
> them.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp a9bf258a0896bd519b99a689a0bfab64d665172f 
>   src/tests/resource_provider_http_api_tests.cpp 
> 46a3f37ba046201e9d79e8ba037da2cb37f5c31f 
> 
> 
> Diff: https://reviews.apache.org/r/60771/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 60888: WIP: Added recovery logic for standalone containers.

[Joseph Wu]

> On July 17, 2017, 10:12 a.m., Jie Yu wrote:
> > src/slave/paths.hpp
> > Lines 56-57 (patched)
> > 
> >
> > If we want to completely decouple containerizer from agent (the goal) 
> > in the future, i think the container sandbox should be under it's own 
> > hierarchy?
> > 
> > To be backwards compatible, we can only do that for standalone 
> > containers first. Ultimately, the agent's directory can use a symlink to 
> > the real container's sandbox i think.
> 
> Joseph Wu wrote:
> So are you thinking something like this?
> ```
>   root ('--work_dir' flag)
> + |-- standalone
> + |   |--  (sandbox)
>   |-- slaves
>   |   |-- latest (symlink)
>   |   |-- 
>   |   |-- frameworks
>   |   |-- ...
> + |   |-- 
>   |-- meta
>   |-- slaves
>   |   |-- ...
> + |-- standalone
> + |-- forked.pid
> ```
> 
> Jie Yu wrote:
> We have `work_dir` (persist across reboot) and `runtime_dir` (does not 
> persist across reboot). Note that these two directories will be in differnet 
> location most likely, but operator might specify them to the be same (e.g., 
> for testing).
> 
> I was thinking something like this:
> ```
> 
>   |-- sandboxes
>  |-- 
>  ...
>   |-- slaves
>  |-- latest (symlink)
>  |-- 
> |-- ...
>   |-- meta
>  |-- ...
> ```
> 
> I was thinking that there will be a separate component to manage 
> standalone containers (DaemonManager). Its state should be in `` 
> (no need to persist across reboot?), and should not tied to 'agent'.

Summary of offline discussion:

* Standalone container sandboxes will live at the top level of the work 
directory under `sandboxes/`.
* We should refactor containerizer recovery logic so that it takes a set of 
ContainerStates which are "recoverable" rather than the SlaveState blob.
* Standalone containers will need a marker file (empty) in the runtime 
directory so that the containerizer knows to recover it.  When a DaemonManager 
(or thing which manages standalone containers) is added in future, we'll stop 
relying on this marker file.


- Joseph


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


On July 14, 2017, 5:40 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> ---
> 
> (Updated July 14, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
> 
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about.  The sandbox and
> metadata directories for standalone containers will be nested under
> `slaves//standalone/`.
> 
> In terms of metadata, standalone containers only checkpoint their
> forked PID at the moment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
>   src/slave/paths.cpp c08e83c14be30c9ef376326d23a8ec6b6b9ff246 
>   src/slave/state.hpp 18c43193349ca6ec3d18967dcee69324a435e2fa 
>   src/slave/state.cpp 24efd4b108ec292223016151d01b10ff2c7f75ca 
> 
> 
> Diff: https://reviews.apache.org/r/60888/diff/1/
> 
> 
> Testing
> ---
> 
> See next patch and later ones too.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 60821: Introduced a "no sender" UPID.

[Jiang Yan Xu]

> On July 12, 2017, 5:27 p.m., James Peach wrote:
> > I think that a better approach is to define a static `UPID 
> > UPID::anonymous()` function that returns a well-defined anonymous UPID for 
> > the system. Remove all the sending functions that don't specify a `from` 
> > UPID and force them to explicitly pass the anonymous UPID. This makes the 
> > used of anonymous message sends explicit which I think is preferable to 
> > this implicit approach.
> 
> Jiang Yan Xu wrote:
> Here's my thinking:
> 
> - In libprocess the PID is usually not constructed directly, aside from 
> being parsed but that's also hidden from the users of the library. Users of 
> libprocess tend to get the PID from the process that owns that PID or from 
> `spawn`. Afterwards it is used as an opaque ID. Therefore I feel like more 
> consistent with the current architecture to not expose it at all. Having to 
> know to use a speical ID is less straighfoward than calling a method that 
> doesn't take a `from` IMO.
> 
> - Right now the PID is just a simple data struct and not guarded by a 
> `process::initialize()` call. This special UPID and PID uses the value of 
> `__address__`, which initialized in `process::initialize()`. Therefore even 
> if we expose this anonymous / no sender PID directly, I feel it's probably 
> going to be constructed by something other than a static UPID method. This is 
> a minor point though.
> 
> James Peach wrote:
> * Anything that calls `post` is going to have to initialize `libprocess`, 
> so initializing `libprocess` when we generate the anonymous UPID seems OK 
> too. `UPID process::anonymous()`.
> * Sending anonymous messages is a special case, so it should be explicit. 
> `post` is also a special case since you already have to specify a target 
> UPID. I argue that explicit is better than implicit and 1 way to `post` is 
> better than 2.
> 
> Jiang Yan Xu wrote:
> Yeah I think the main thing I am arguing for here is that as you said, 
> the purpose of this is to support "Sending anonymous messages / message with 
> no sender address". IMO the sentence from the user's POV translates to a 
> method (and we already have this method), not an UPID.
> 
> In othe words, this special 
> [process::post](https://github.com/apache/mesos/blob/5645fddf07eab0fcd195a3514e9e5ad3ef25628d/3rdparty/libprocess/include/process/process.hpp#L626-L637)
>  overload serves a special purpose that
> - We currently have a use case directly
> - I don't see how it could be incorrectly used for other unintended ways.
> 
> 
> In contrast, exposing a special UPID to user:
> - I don't see how the UPID can be used for another legit purpose.
> - I can see it could used incorrectly by the user.
> 
> 
> I do support explictness, but I think saying a method "sends message with 
> no sender address" is pretty explicit too and without exposing too much 
> flexibility and a concept that could be hard(er) to undertand (than this).
> 
> Having said that, I am not adamant on the current design. Given another 
> vote/idea I can switch.
> 
> Joseph Wu wrote:
> If the eventual goal is to enable 
> `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH` by default, it would make sense to 
> either (A) add the special anonymous sender, or (B) remove the sender-less 
> `process::post` variant entirely.
> 
> In terms of implementing (A), I would opt for an implementation that is 
> explicit.  This is primarily due to the ambiguity from calling 
> `process::post(to, message)` within the context of a libprocess Process, vs a 
> main thread or test body.
> 
> That being said, we don't have any protections or validation regarding 
> the `from` field anyway.  Any process can say `process::post(not_me /* :P */, 
> to, message)` and that would be considered valid.
> 
> ---
> 
> I agree with the explicit anonymous sender approach.  But I don't think 
> we necessarily need to implement the anonymous sender yet.
> 
> I think going with the explicit route will give us an excuse to 
> double-check any places with no sender address/UPID.  If we see no reason to 
> keep it, then we could add the explicit anonymous sender if we ever find a 
> reason to include it.
> 
> Jiang Yan Xu wrote:
> Not sure if I 100% understand the recommendation here. I think that it is:
> 
> - We implement `UPID process::anonymous()` and use it to avoid 
> `LIBPROCESS_REQUIRE_PEER_ADDRESS_IP_MATCH` check failure.
> 
> > But I don't think we necessarily need to implement the anonymous sender 
> yet.
> 
> Did you mean to say that we do "(B) remove the sender-less process::post 
> variant entirely" later after we "double-check any places with no sender 
> address/UPID"?

Clarified with Joseph offline. The proposal is to just fix this specific 
situation in MESOS-7753 (I submitted /r/60917/ for review). I'll drop this 
review for now and we can 

Re: Review Request 53479: Perform agent GC asynchronously.

[Jiang Yan Xu]

> On July 17, 2017, 1:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > 
> >
> > I see that `promise == that.promise` here is required by the Multimap 
> > but does it make sense to have `PathInfo` equality depend only on the path?
> > 
> > I don't think `Promise` is supposed to be comparable.

OK I think strictly speaking we shouldn't assume that there are no equal values 
under the same key in a Multimap (or std::multimap).

This case shouldn't happen with our GC so we should probably insert some hard 
`CHECK`s for it (in a separate patch) but I don't think removing `promise == 
that.promise` has anything to do with it, i.e., the problem exists today.


- Jiang Yan


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


On July 14, 2017, 6:23 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 14, 2017, 6:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Added: https://reviews.apache.org/r/60893/
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/8/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 53479: Perform agent GC asynchronously.

[Jiang Yan Xu]

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




src/slave/gc.hpp
Lines 114-117 (original), 115-118 (patched)


I see that `promise == that.promise` here is required by the Multimap but 
does it make sense to have `PathInfo` equality depend only on the path?

I don't think `Promise` is supposed to be comparable.



src/slave/gc.cpp
Line 100 (original), 119 (patched)


`s/(unsigned long)1/1u/`



src/slave/gc.cpp
Lines 209 (patched)


`s/(unsigned long)1/1u/`


- Jiang Yan Xu


On July 14, 2017, 6:23 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated July 14, 2017, 6:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Added: https://reviews.apache.org/r/60893/
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/8/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 60594: Added a`network/ports` isolator nested container test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60491, 60493, 60494, 60764, 60901, 60836, 60902, 60492, 
60495, 60496, 60592, 60591, 60766, 60903, 60765, 60593, 60594]

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 July 3, 2017, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60594/
> ---
> 
> (Updated July 3, 2017, 10:32 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to verify that the `network/ports` isolator works
> correctly with nested containers.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/network_ports_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60594/diff/6/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

[Avinash sridharan]

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




src/messages/flags.proto
Lines 56 (patched)


I see Jie's point. From a framework's standpoint a name is a single domain 
of IP connectivity, so ideally if we have a network name that is both a CNI and 
CNM network (DC/OS overlay or Calico comes to mind here), it implies that it 
should be a single network and hence having a common DNS configuration across 
CNI/CNM is not incorrect. 

That said instead of using Mesos and Docker as the fields if we use CNI and 
CNM as the field names would that make sense? We would then be pegging against 
the standard rather than the `Containerizer`? The reason I am proposing to keep 
two different fields here is primarily to take into consideration any 
incosistencies that might pop up in these standards at some point (right now 
there don't seem to be any for DNS).


- Avinash sridharan


On July 5, 2017, 7:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 5, 2017, 7:03 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 
>   src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60917: Used a real UPID to send `LearnedMessage`.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60917]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 3 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60917/
> ---
> 
> (Updated July 17, 2017, 3 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-7753
> https://issues.apache.org/jira/browse/MESOS-7753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously an empty UPID `@0.0.0.0:0` was used which could result in
> the message being dropped due to certain security check (MESOS-7401).
> 
> 
> Diffs
> -
> 
>   src/log/network.hpp 0afcfae27394527f7c957e31014cc9a32a4bf63b 
> 
> 
> Diff: https://reviews.apache.org/r/60917/diff/1/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60890: WIP: Defined API for launching standalone containers.

[James DeFelice]

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



Some questions about this API:

1. If a launch request times out, how might someone query the state of the 
requested launch operation/container?
2. When a stand alone container terminates (either success or failure), who is 
notified and when?
3. What are the expected failure modes/codes for this API and what are the 
recovery semantics?

- James DeFelice


On July 15, 2017, 12:41 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60890/
> ---
> 
> (Updated July 15, 2017, 12:41 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Launching a standalone container is very similar to launching a
> nested container, except that the caller does not specify a
> ContainerID (which will be auto-generated) and must specify some
> Resources.  The CommandInfo and ContainerInfo fields behave similarly.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 9bac9541acd24e1123ca5dd5925e2a1381d13b4a 
>   include/mesos/v1/agent/agent.proto ea9282cf12fbe1c2ddeaa37223e4811685263734 
> 
> 
> Diff: https://reviews.apache.org/r/60890/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> See later patches in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

[Jie Yu]

> On July 14, 2017, 6:31 p.m., Jie Yu wrote:
> > src/messages/flags.proto
> > Lines 56 (patched)
> > 
> >
> > Any reason we seperate 'mesos' from 'docker'? Can we use the same?
> 
> Qian Zhang wrote:
> The reason that we have 'mesos' and 'docker' here is that we want 
> operator to be able to set DNS info for CNI networks and CNM networks 
> separately. Jie, did you mean we may need to merge them into one like below 
> so that we do not have 'mesos' and 'docker' anymore?
> ```
> {
>   [
> {
>   "network": "bridge",
>   "dns": {
> "nameservers": [ "8.8.8.8" ]
>   }
> },
> {
>   "network": "net1",
>   "dns": {
> "nameservers": [ "8.8.4.4" ]
>   }
> }
>   ]
> }
> ```
> I think it should be OK. The only potential issue for the above design in 
> my mind is, if there are 2 networks, 1 CNI and 1 CNM, but they have the same 
> name (say `net1`), and operator may want to set different DNS for them 
> respectively which can not be achieved with the above design.

The reason I am asking is that: we should ultimately avoid having flags that 
are specific to either Docker/Mesos containerizer. We should ask the 
containerizer to provide consistent behaviors. It looks to me that "default DNS 
for the container" should apply to both containerizers.


- Jie


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


On July 5, 2017, 7:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated July 5, 2017, 7:03 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp c6803eb54e09a5497755e1e5fef2872193eacba6 
>   src/slave/flags.cpp 398768656b5fa3b7c85474de2b4b008bf7b85cb3 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 60917: Used a real UPID to send `LearnedMessage`.

[Jiang Yan Xu]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Previously an empty UPID `@0.0.0.0:0` was used which could result in
the message being dropped due to certain security check (MESOS-7401).


Diffs
-

  src/log/network.hpp 0afcfae27394527f7c957e31014cc9a32a4bf63b 


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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 60888: WIP: Added recovery logic for standalone containers.

[Jie Yu]

> On July 17, 2017, 5:12 p.m., Jie Yu wrote:
> > src/slave/paths.hpp
> > Lines 56-57 (patched)
> > 
> >
> > If we want to completely decouple containerizer from agent (the goal) 
> > in the future, i think the container sandbox should be under it's own 
> > hierarchy?
> > 
> > To be backwards compatible, we can only do that for standalone 
> > containers first. Ultimately, the agent's directory can use a symlink to 
> > the real container's sandbox i think.
> 
> Joseph Wu wrote:
> So are you thinking something like this?
> ```
>   root ('--work_dir' flag)
> + |-- standalone
> + |   |--  (sandbox)
>   |-- slaves
>   |   |-- latest (symlink)
>   |   |-- 
>   |   |-- frameworks
>   |   |-- ...
> + |   |-- 
>   |-- meta
>   |-- slaves
>   |   |-- ...
> + |-- standalone
> + |-- forked.pid
> ```

We have `work_dir` (persist across reboot) and `runtime_dir` (does not persist 
across reboot). Note that these two directories will be in differnet location 
most likely, but operator might specify them to the be same (e.g., for testing).

I was thinking something like this:
```

  |-- sandboxes
 |-- 
 ...
  |-- slaves
 |-- latest (symlink)
 |-- 
|-- ...
  |-- meta
 |-- ...
```

I was thinking that there will be a separate component to manage standalone 
containers (DaemonManager). Its state should be in `` (no need to 
persist across reboot?), and should not tied to 'agent'.


- Jie


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


On July 15, 2017, 12:40 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> ---
> 
> (Updated July 15, 2017, 12:40 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
> 
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about.  The sandbox and
> metadata directories for standalone containers will be nested under
> `slaves//standalone/`.
> 
> In terms of metadata, standalone containers only checkpoint their
> forked PID at the moment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
>   src/slave/paths.cpp c08e83c14be30c9ef376326d23a8ec6b6b9ff246 
>   src/slave/state.hpp 18c43193349ca6ec3d18967dcee69324a435e2fa 
>   src/slave/state.cpp 24efd4b108ec292223016151d01b10ff2c7f75ca 
> 
> 
> Diff: https://reviews.apache.org/r/60888/diff/1/
> 
> 
> Testing
> ---
> 
> See next patch and later ones too.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60719]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 5:59 p.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60719/
> ---
> 
> (Updated July 17, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Part of MESOS-7310, this patch adds the test infrastructure necessary
> for reliably running unit tests for the mesos package located under
> src/python/lib.
> 
> setup.py is added under src/python/lib to both define the Python package
> and to allow tests to be run via `python setup.py test`, which delegates
> tests to pytest.
> 
> Review: https://reviews.apache.org/r/60719/
> 
> 
> Diffs
> -
> 
>   src/python/.gitignore 0d20b6487c61e7d1bde93acf4a14b7a89083a16d 
>   src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/requirements-test.in PRE-CREATION 
>   src/python/lib/requirements.in PRE-CREATION 
>   src/python/lib/setup.py PRE-CREATION 
>   src/python/lib/test.sh PRE-CREATION 
>   src/python/lib/tests/__init__.py PRE-CREATION 
>   src/python/lib/tests/test_mesos.py PRE-CREATION 
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60719/diff/8/
> 
> 
> Testing
> ---
> 
> under src/python/lib, run `bash test.sh`
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 60482: Defined Git variables in CMake configure file.

[Andrew Schwartzmeyer]

> On July 17, 2017, 10:18 a.m., Joseph Wu wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 347-348 (original), 367-368 (patched)
> > 
> >
> > This comment should be updated to reflect the doubling in output.

+1


- Andrew


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


On June 27, 2017, 2:19 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60482/
> ---
> 
> (Updated June 27, 2017, 2:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jeff Coffler, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7173
> https://issues.apache.org/jira/browse/MESOS-7173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-7173.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b51a7a8dfe05085298f240019ff7b9c4 
>   src/common/build_config.hpp.in 174d8aaba850d99ceeea965cd5faf0624c5051d6 
> 
> 
> Diff: https://reviews.apache.org/r/60482/diff/1/
> 
> 
> Testing
> ---
> 
> Built `mesos-agent` with CMake on Linux and started it with `GLOG_v=3`:
> 
> ```
> I0627 14:13:00.300671 75141 main.cpp:335] Git SHA: 
> ed1c00e9b12dbdead54f3630188db8c510654075
> ```
> 
> It appears to be working.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 60899: Moved new Mesos CLI to src/python/.

[Eric Chung]

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



missing bootstrap for mesos itself (config, make etc.) in the test instructions

- Eric Chung


On July 16, 2017, 7:33 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60899/
> ---
> 
> (Updated July 16, 2017, 7:33 p.m.)
> 
> 
> Review request for mesos, Eric Chung and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change allows to have all the Python files under
> only two directories : src/python/ and support/. This
> will improve the structure of our Python code in the future
> by sharing configuration files and virtual environments.
> 
> 
> Diffs
> -
> 
>   src/cli_new/.gitignore 3ce5f74748dae6ac60170a1f0c73191fc26177d9 
>   src/cli_new/README.md  
>   src/cli_new/activate  
>   src/cli_new/bin/main.py  
>   src/cli_new/bin/mesos  
>   src/cli_new/bin/mesos-cli-tests  
>   src/cli_new/bin/settings.py  
>   src/cli_new/bootstrap 4193d3f38961747a6f5e60a8cd1715eff76d660e 
>   src/cli_new/deactivate  
>   src/cli_new/lib/cli/__init__.py  
>   src/cli_new/lib/cli/config.py  
>   src/cli_new/lib/cli/docopt.py  
>   src/cli_new/lib/cli/exceptions.py  
>   src/cli_new/lib/cli/http.py  
>   src/cli_new/lib/cli/plugins/__init__.py  
>   src/cli_new/lib/cli/plugins/base.py  
>   src/cli_new/lib/cli/plugins/config/__init__.py  
>   src/cli_new/lib/cli/plugins/config/main.py  
>   src/cli_new/lib/cli/tests/__init__.py  
>   src/cli_new/lib/cli/tests/base.py 62a4ee8ac2d39c8ecd400560aa51485cab0410f2 
>   src/cli_new/lib/cli/tests/constants.py  
>   src/cli_new/lib/cli/tests/tests.py  
>   src/cli_new/lib/cli/util.py  
>   src/cli_new/mesos.bash_completion  
>   src/cli_new/pip-requirements.txt  
>   src/cli_new/pylint.config  
>   src/cli_new/tests/main.py  
>   support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 
> 
> 
> Diff: https://reviews.apache.org/r/60899/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> ```
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

(Updated July 17, 2017, 5:59 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

Fix linting issues


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.

Review: https://reviews.apache.org/r/60719/


Diffs (updated)
-

  src/python/.gitignore 0d20b6487c61e7d1bde93acf4a14b7a89083a16d 
  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 
  src/python/lib/test.sh PRE-CREATION 
  src/python/lib/tests/__init__.py PRE-CREATION 
  src/python/lib/tests/test_mesos.py PRE-CREATION 
  support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 


Diff: https://reviews.apache.org/r/60719/diff/8/

Changes: https://reviews.apache.org/r/60719/diff/7-8/


Testing
---

under src/python/lib, run `bash test.sh`


Thanks,

Eric Chung

Re: Review Request 60482: Defined Git variables in CMake configure file.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60482]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 27, 2017, 2:19 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60482/
> ---
> 
> (Updated June 27, 2017, 2:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jeff Coffler, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7173
> https://issues.apache.org/jira/browse/MESOS-7173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-7173.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b51a7a8dfe05085298f240019ff7b9c4 
>   src/common/build_config.hpp.in 174d8aaba850d99ceeea965cd5faf0624c5051d6 
> 
> 
> Diff: https://reviews.apache.org/r/60482/diff/1/
> 
> 
> Testing
> ---
> 
> Built `mesos-agent` with CMake on Linux and started it with `GLOG_v=3`:
> 
> ```
> I0627 14:13:00.300671 75141 main.cpp:335] Git SHA: 
> ed1c00e9b12dbdead54f3630188db8c510654075
> ```
> 
> It appears to be working.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 60791: Add fetcher cache space usage metrics.

[Joseph Wu]

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




docs/monitoring.md
Lines 1258-1261 (patched)


Perhaps mention that this value is constant over the life of a single 
fetcher.



src/slave/containerizer/fetcher_process.hpp
Lines 138-141 (original), 139-142 (patched)


This method appears to be un-used and unimplemented.  So you might want to 
remove it in a separate patch.



src/tests/fetcher_cache_tests.cpp
Lines 287-291 (patched)


Might have to be careful with this comparison.  Metrics output numbers with 
`double` precision.


- Joseph Wu


On July 16, 2017, 11:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60791/
> ---
> 
> (Updated July 16, 2017, 11:01 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7782
> https://issues.apache.org/jira/browse/MESOS-7782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add fetcher metrics to track the (constant) size of the cache size
> and the (varying) amount of cache space use. The cache size is
> published as `containerizer/fetcher/cache_size_total` and the used
> space is `containerizer/fetcher/cache_size_used`. Both of these
> metrics are in MB to be consistent with other disk space metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 38b8093ef683b5de65cbfa5330a6bbd1c9e10f12 
>   src/slave/containerizer/fetcher.cpp 
> 6a664e0657a19d27afac98fd5298d6a18aabe43f 
>   src/slave/containerizer/fetcher_process.hpp 
> 0a342303574927ebfb8c60e6ce2ebe67bd8a12a3 
>   src/tests/fetcher_cache_tests.cpp 6e4736ea42430017ca6eff9134c1136179a4b26f 
> 
> 
> Diff: https://reviews.apache.org/r/60791/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 50705: Changed master to allow partitioned slaves to reregister.

[Vinod Kone]

> On July 15, 2017, 4:31 p.m., David McLaughlin wrote:
> > With the new code-path for mark unreachable after failover, this change 
> > introduced a non-backwards compatible change - namely that TASK_LOST 
> > messages for each task on the agent are no longer sent when the slaveLost 
> > message is sent. This means that frameworks (like Aurora) no longer get the 
> > signal to schedule replacements for those tasks until they reconcile. Given 
> > that the tasks will be marked as LOST as soon as the agent reregisters 
> > anyway, seems like it's easy to maintain backwards compatibility here.

There is a discussion about this on the mailing list. Would you mind 
incorporating your feedback there?


- Vinod


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


On Sept. 12, 2016, 10:05 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50705/
> ---
> 
> (Updated Sept. 12, 2016, 10:05 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4049
> https://issues.apache.org/jira/browse/MESOS-4049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous behavior was to shutdown partitioned agents that attempt to
> reregister---unless the master has failed over, in which case the
> reregistration is allowed (when running in "non-strict" mode).
> 
> The new behavior is always to allow partitioned agents to reregister.
> This is part of a longer-term project to allow frameworks to define
> their own policies for handling tasks running on partitioned agents.
> 
> In particular, if a framework has the PARTITION_AWARE capability, any
> tasks running on the partitioned agent will continue to run after
> reregistration. If the framework is not PARTITION_AWARE, any tasks that
> were running on such an agent will be killed after the agent reregisters
> (unless the master has failed over). This is for backward compatibility
> with the previous ("non-strict") behavior. Note that regardless of the
> PARTITION_AWARE capability, the agent will not be shutdown, which is a
> change from the previous Mesos behavior.
> 
> This commit also changes the master so that if an agent is removed and
> then the master receives a message from that agent, the master will no
> longer attempt to shutdown the agent. This is consistent with the goal
> of getting the master out of the business of shutting down agents that
> we suspect are unhealthy. Such an agent will eventually realize it is
> not registered with the master (e.g., because it won't receive any pings
> from the master), which will cause it to reregister.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 4992ab0a0bb5babbf6a4fa3e6eff3577590fc879 
>   src/master/master.cpp 1dcce6cd66804990af238176c61aca03bb5c9471 
>   src/tests/master_tests.cpp 6cde15fcd6ca8ec40438c75aed980e83f8de9b86 
>   src/tests/partition_tests.cpp f3142ad8d50daafcdb70ad9dbb2772f8ba30db00 
> 
> 
> Diff: https://reviews.apache.org/r/50705/diff/10/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

(Updated July 17, 2017, 5:50 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

address comments


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.

Review: https://reviews.apache.org/r/60719/


Diffs (updated)
-

  src/python/.gitignore 0d20b6487c61e7d1bde93acf4a14b7a89083a16d 
  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 
  src/python/lib/test.sh PRE-CREATION 
  src/python/lib/tests/__init__.py PRE-CREATION 
  src/python/lib/tests/test_mesos.py PRE-CREATION 
  support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 


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

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


Testing
---

under src/python/lib, run `bash test.sh`


Thanks,

Eric Chung

Re: Review Request 60539: Added test for allocated resources per each role in the agent endpoint.

[Andrei Budnik]

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

(Updated July 17, 2017, 5:47 p.m.)


Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

Added test for allocated resources per each role in the agent endpoint.


Diffs
-

  src/tests/reservation_endpoints_tests.cpp 
05b505fc4c643f56c7141b6b79f334b000470780 


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


Testing
---

make check (mac os x, fedora 25)


Thanks,

Andrei Budnik

Re: Review Request 60369: Exposed allocated resources per each role in /state endpoint on agent.

[Andrei Budnik]

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

(Updated July 17, 2017, 5:45 p.m.)


Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

The JSON key for this information is "reserved_resources_allocated"
and "unreserved_resources_allocated". Note that this info might differ
from the master's view about resource allocations.


Diffs (updated)
-

  src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 


Diff: https://reviews.apache.org/r/60369/diff/8/

Changes: https://reviews.apache.org/r/60369/diff/7-8/


Testing
---

make check


Thanks,

Andrei Budnik

Review Request 60915: Enabled filtering of reservations in the agent.

[Andrei Budnik]

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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

Adds support of the `VIEW_ROLE` ACL to the results generated by the
`/state` endpoint in the agent for fields `reserved_resources_full`,
`reserved_resources` and `reserved_resources_allocated`.


Diffs
-

  src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 


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


Testing
---

make check


Thanks,

Andrei Budnik

Re: Review Request 60888: WIP: Added recovery logic for standalone containers.

[Joseph Wu]

> On July 17, 2017, 10:12 a.m., Jie Yu wrote:
> > src/slave/paths.hpp
> > Lines 56-57 (patched)
> > 
> >
> > If we want to completely decouple containerizer from agent (the goal) 
> > in the future, i think the container sandbox should be under it's own 
> > hierarchy?
> > 
> > To be backwards compatible, we can only do that for standalone 
> > containers first. Ultimately, the agent's directory can use a symlink to 
> > the real container's sandbox i think.

So are you thinking something like this?
```
  root ('--work_dir' flag)
+ |-- standalone
+ |   |--  (sandbox)
  |-- slaves
  |   |-- latest (symlink)
  |   |-- 
  |   |-- frameworks
  |   |-- ...
+ |   |-- 
  |-- meta
  |-- slaves
  |   |-- ...
+ |-- standalone
+ |-- forked.pid
```


- Joseph


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


On July 14, 2017, 5:40 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> ---
> 
> (Updated July 14, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
> 
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about.  The sandbox and
> metadata directories for standalone containers will be nested under
> `slaves//standalone/`.
> 
> In terms of metadata, standalone containers only checkpoint their
> forked PID at the moment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
>   src/slave/paths.cpp c08e83c14be30c9ef376326d23a8ec6b6b9ff246 
>   src/slave/state.hpp 18c43193349ca6ec3d18967dcee69324a435e2fa 
>   src/slave/state.cpp 24efd4b108ec292223016151d01b10ff2c7f75ca 
> 
> 
> Diff: https://reviews.apache.org/r/60888/diff/1/
> 
> 
> Testing
> ---
> 
> See next patch and later ones too.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 60482: Defined Git variables in CMake configure file.

[Joseph Wu]

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


Ship it!




Going to add some more comments and whitespace before committing.


cmake/CompilationConfigure.cmake
Lines 346 (patched)


I'm going to add an extra comment above this section.



cmake/CompilationConfigure.cmake
Lines 347-348 (original), 367-368 (patched)


This comment should be updated to reflect the doubling in output.


- Joseph Wu


On June 27, 2017, 2:19 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60482/
> ---
> 
> (Updated June 27, 2017, 2:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jeff Coffler, John Kordich, 
> Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7173
> https://issues.apache.org/jira/browse/MESOS-7173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-7173.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b51a7a8dfe05085298f240019ff7b9c4 
>   src/common/build_config.hpp.in 174d8aaba850d99ceeea965cd5faf0624c5051d6 
> 
> 
> Diff: https://reviews.apache.org/r/60482/diff/1/
> 
> 
> Testing
> ---
> 
> Built `mesos-agent` with CMake on Linux and started it with `GLOG_v=3`:
> 
> ```
> I0627 14:13:00.300671 75141 main.cpp:335] Git SHA: 
> ed1c00e9b12dbdead54f3630188db8c510654075
> ```
> 
> It appears to be working.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 60888: WIP: Added recovery logic for standalone containers.

[Jie Yu]

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




src/slave/paths.hpp
Lines 56-57 (patched)


If we want to completely decouple containerizer from agent (the goal) in 
the future, i think the container sandbox should be under it's own hierarchy?

To be backwards compatible, we can only do that for standalone containers 
first. Ultimately, the agent's directory can use a symlink to the real 
container's sandbox i think.


- Jie Yu


On July 15, 2017, 12:40 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> ---
> 
> (Updated July 15, 2017, 12:40 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
> 
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about.  The sandbox and
> metadata directories for standalone containers will be nested under
> `slaves//standalone/`.
> 
> In terms of metadata, standalone containers only checkpoint their
> forked PID at the moment.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 9376d14d66f5dc7e91c7c0e9da253f5eb9347539 
>   src/slave/paths.hpp 51b481fc0870f1e95448f85ee2fd485fceea1919 
>   src/slave/paths.cpp c08e83c14be30c9ef376326d23a8ec6b6b9ff246 
>   src/slave/state.hpp 18c43193349ca6ec3d18967dcee69324a435e2fa 
>   src/slave/state.cpp 24efd4b108ec292223016151d01b10ff2c7f75ca 
> 
> 
> Diff: https://reviews.apache.org/r/60888/diff/1/
> 
> 
> Testing
> ---
> 
> See next patch and later ones too.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60913]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 3:31 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60913/
> ---
> 
> (Updated July 17, 2017, 3:31 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Till Toenshoff.
> 
> 
> Bugs: MESOS-7792
> https://issues.apache.org/jira/browse/MESOS-7792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support for Elliptic Curve Diffie Hellman algorithm requires extra
> configuration parameters which weren't part of Mesos.
> 
> This patch enables the extra configuration to Mesos in order to
> support ECDH algorithm, it also adds the ssl flag
> `LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
> a specific elliptic curve.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
>   3rdparty/libprocess/src/openssl.cpp 
> e6f17e4591f573186e1dc9697e1e7b60a841fe4f 
> 
> 
> Diff: https://reviews.apache.org/r/60913/diff/2/
> 
> 
> Testing
> ---
> 
> Launched Mesos with only ECDHE handshake ciphers enabled, then reached mesos 
> with different browsers and command line tools.
> 
> ```shell
> LIBPROCESS_SSL_ENABLED=1 \
> LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
> LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
> LIBPROCESS_SSL_CIPHERS="ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA"
>  \
> ./bin/mesos-master.sh \
> --work_dir=/tmp/mesos/master \
> --log_dir=/tmp/mesos/master/log
> ```
> 
> Unit test coming.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Alexander Rojas]

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

(Updated July 17, 2017, 5:31 p.m.)


Review request for mesos, Jie Yu and Till Toenshoff.


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


Repository: mesos


Description
---

Support for Elliptic Curve Diffie Hellman algorithm requires extra
configuration parameters which weren't part of Mesos.

This patch enables the extra configuration to Mesos in order to
support ECDH algorithm, it also adds the ssl flag
`LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
a specific elliptic curve.


Diffs
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
  3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 


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


Testing (updated)
---

Launched Mesos with only ECDHE handshake ciphers enabled, then reached mesos 
with different browsers and command line tools.

```shell
LIBPROCESS_SSL_ENABLED=1 \
LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
LIBPROCESS_SSL_CIPHERS="ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA"
 \
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log
```

Unit test coming.


Thanks,

Alexander Rojas

Re: Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Alexander Rojas]

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

(Updated July 17, 2017, 5:31 p.m.)


Review request for mesos, Jie Yu and Till Toenshoff.


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


Repository: mesos


Description
---

Support for Elliptic Curve Diffie Hellman algorithm requires extra
configuration parameters which weren't part of Mesos.

This patch enables the extra configuration to Mesos in order to
support ECDH algorithm, it also adds the ssl flag
`LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
a specific elliptic curve.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
  3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 


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

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


Testing
---

Launched Mesos with only ECDHE handshake ciphers enabled, then reached mesos 
with different browsers and command line tools.

```shell
LIBPROCESS_SSL_ENABLED=1 \
LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
LIBPROCESS_SSL_CIPHERS="ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA"
 \
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log
```


Thanks,

Alexander Rojas

Review Request 60913: Adds support for OpenSSL's ECDH handshake.

[Alexander Rojas]

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

Review request for mesos, Jie Yu and Till Toenshoff.


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


Repository: mesos


Description
---

Support for Elliptic Curve Diffie Hellman algorithm requires extra
configuration parameters which weren't part of Mesos.

This patch enables the extra configuration to Mesos in order to
support ECDH algorithm, it also adds the ssl flag
`LIBPROCESS_SSL_ECDH_CURVES` which allows for the specification of
a specific elliptic curve.


Diffs
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
13fa7a0cc9d6d6d6849976a3ce383263c51504d7 
  3rdparty/libprocess/src/openssl.cpp e6f17e4591f573186e1dc9697e1e7b60a841fe4f 


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


Testing
---

Launched Mesos with only ECDHE handshake ciphers enabled, then reached mesos 
with different browsers and command line tools.

```shell
LIBPROCESS_SSL_ENABLED=1 \
LIBPROCESS_SSL_KEY_FILE=/tmp/ssl/self-signed.key \
LIBPROCESS_SSL_CERT_FILE=/tmp/ssl/self-signed.crt \
LIBPROCESS_SSL_CIPHERS="ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA"
 \
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log
```


Thanks,

Alexander Rojas

Re: Review Request 60902: Moved the libnl3 configure checks into a macro.

[Qian Zhang]

> On July 17, 2017, 11:20 a.m., Qian Zhang wrote:
> > m4/libnl3.m4
> > Lines 33-37 (patched)
> > 
> >
> > Mind to elaborate a bit about why introducing a new .m4 file? What 
> > about just introducing a new configure option (like `--enable-libnl3`) and 
> > make `--enable_port_mapping_isolator` depend on it? Just like the 
> > relationship between `--enable-ssl` and `--enable-libevent`: 
> > https://github.com/apache/mesos/blob/1.3.0/configure.ac#L1657:L1698
> > 
> > And for `--enable-libnl3`, I think we should not do much checks, just 
> > some very basic check should be OK, and on top of it, 
> > `--enable_port_mapping_isolator` can do some more advanced/specific check 
> > on libnl3 for itself.
> 
> James Peach wrote:
> > Mind to elaborate a bit about why introducing a new .m4 file?
> 
> This is because a separate file makes it easier to maintain. We could 
> inline the macro into `configure.ac` but that's not typical autotool practice.
> 
> > What about just introducing a new configure option (like 
> --enable-libnl3)
> 
> I don't see how `--enable-libnl3` makes anything simpler, unless you want 
> to to use that option to enable all the features that depend on `libnl3`. In 
> that case, however, I'd just implement that based on the existing `--with-nl`.
> 
> The semantics here are that we have `--enable-port-mapping-isolator`, 
> `--enable-network-ports-isolator` and `--with-nl` arguments. If either of the 
> first 2 options is enabled, we will attempt to use the `libnl3` from the 
> location of the 3rd option.
> 
> > I think we should not do much checks
> 
> The checks we are applying are all ones that we already had. I didn't 
> want to change the dependency requirements here.

I was thinking to use `--enable-libnl3` to check basic libnl3 features, and use 
`--enable_port_mapping_isolator` and `--enable-network-ports-isolator` to check 
the advanced libnl3 features needed by them respectively.

So in your solution, there will be only 2 `--enable-xxx` flags (i.e., 
`--enable_port_mapping_isolator`, `--enable-network-ports-isolator`) and 
`--with-nl`, and `MESOS_HAVE_LIBNL3` is internal which is not exposed to the 
user, right?


- Qian


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


On July 17, 2017, 7:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60902/
> ---
> 
> (Updated July 17, 2017, 7:43 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/ports` isolator will depend on libnl3, move those
> checks into a separate macro so that we can call it again when we
> add a configure option to enable it.
> 
> 
> Diffs
> -
> 
>   configure.ac 4d7c4a4679e5c624ee750226d542e0d8c228507a 
>   m4/libnl3.m4 PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60902/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26), manual verification of configuration logs.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60002, 60003]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On July 17, 2017, 11:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 17, 2017, 11:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7713
> https://issues.apache.org/jira/browse/MESOS-7713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, ::someMethod, param1, param2));
> 
> For the objects that do not support move semantics (e.g. protobuf messages), 
> number of copies is reduced from 8-10 to 6. If move semantics is supported, 
> then number of copies is reduced from 6-7 to 1 if parameter is passed with 
> std::move, or 2 otherwise.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/defer.hpp 
> 7f3369e723cb244e97930621cbba89cf7873567d 
>   3rdparty/libprocess/include/process/deferred.hpp 
> e446621be11ac51f5f91c417cc8975e363c2f715 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/future.hpp 
> cce950509f58022e79bb51a6e72ea1a005b9cb50 
>   3rdparty/libprocess/include/process/http.hpp 
> f637999174d92a98208b5fc49a65f9929efb11a0 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> Number of copies was checked by using defer to subscribe process for Future 
> callbacks, and passing parameters that count number of copies made.
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 60867: Added a method in the agent exposing an executor's allocated resources.

[Andrei Budnik]

> On July 14, 2017, 10:16 p.m., Benjamin Mahler wrote:
> > src/slave/slave.hpp
> > Lines 796-797 (original), 798-799 (patched)
> > 
> >
> > IIUC, this was supposed to represent the resources allocated the 
> > executor and is now obviated by your function, so it should be removed.
> > 
> > We don't have a clear definition of "consumed" here, my understanding 
> > is that it meant the resources "allocated" to the executor.

Fixed in a separate [patch](https://reviews.apache.org/r/60907/).


- Andrei


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


On July 17, 2017, 12:09 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60867/
> ---
> 
> (Updated July 17, 2017, 12:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to call a method to get the executor's allocated
> resources, instead of call-sites having to each iterate over the
> executor's pending and launched tasks. It turns out that most
> call-sites forgot to account for the pending tasks, and accounting
> for the pending tasks is non-trivial!
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
>   src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 
> 
> 
> Diff: https://reviews.apache.org/r/60867/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60867: Added a method in the agent exposing an executor's allocated resources.

[Andrei Budnik]

> On July 14, 2017, 10:35 p.m., Benjamin Mahler wrote:
> > src/slave/slave.cpp
> > Lines 7816-7818 (patched)
> > 
> >
> > Also, you don't want to include terminated tasks :)

Fixed.


- Andrei


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


On July 17, 2017, 12:09 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60867/
> ---
> 
> (Updated July 17, 2017, 12:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows us to call a method to get the executor's allocated
> resources, instead of call-sites having to each iterate over the
> executor's pending and launched tasks. It turns out that most
> call-sites forgot to account for the pending tasks, and accounting
> for the pending tasks is non-trivial!
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
>   src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 
> 
> 
> Diff: https://reviews.apache.org/r/60867/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60867: Added a method in the agent exposing an executor's allocated resources.

[Andrei Budnik]

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

(Updated July 17, 2017, 12:09 p.m.)


Review request for mesos, Benjamin Mahler and haosdent huang.


Summary (updated)
-

Added a method in the agent exposing an executor's allocated resources.


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


Repository: mesos


Description (updated)
---

This allows us to call a method to get the executor's allocated
resources, instead of call-sites having to each iterate over the
executor's pending and launched tasks. It turns out that most
call-sites forgot to account for the pending tasks, and accounting
for the pending tasks is non-trivial!


Diffs (updated)
-

  src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
  src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 


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

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


Testing
---

make check


Thanks,

Andrei Budnik

Review Request 60907: Got rid of executor's `resources` variable in the agent.

[Andrei Budnik]

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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

`allocatedResources()` method is used instead of executor's variable
`resources`, which doesn't include resources of pending tasks.


Diffs
-

  src/slave/http.cpp 60640e5dbf6c3f8c709351e77ca89f287cf12fc3 
  src/slave/slave.hpp 8bb03ecd86bfd87dfd27a800910130aec04e0919 
  src/slave/slave.cpp adbe65fbb7c555b098eaae228c7277402452d8a2 


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


Testing
---

make check


Thanks,

Andrei Budnik

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

> On July 4, 2017, 10:55 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/include/process/deferred.hpp
> > Line 77 (original), 79 (patched)
> > 
> >
> > This is more a question than an issue. Do we plan to forward functor 
> > references through `_Deferred`, hence the use of `std::forward()` here 
> > instead of `std::move()`?
> > 
> > Carrying a functor reference is unsafe. `template  
> > _Deferred defer(F&&)` overload silently captures references to lvalues 
> > in `_Deferred`. But as long as `_Deferred` is immediately used for 
> > conversion to `Deferred` or `std::function` it should be fine.

This is intentional, as in this case move/copy happens only once on conversion. 
Saving `f` by value would add an extra move/copy on `_Deferred` construction.
This implementation is relatively safe, as `_Deferred` is documented as an 
intermediate type, which is not supposed to be used directly, and it's somewhat 
protected from misuse by having public methods callable on rvalues only.


> On July 4, 2017, 10:55 a.m., Ilya Pronin wrote:
> > 3rdparty/libprocess/include/process/deferred.hpp
> > Lines 83-88 (original), 85-90 (patched)
> > 
> >
> > Can we eliminate more copyings here? `f_` is captured by value by 
> > lambda and then passed as a `const` to `dispatch`. We can make the lambda 
> > `mutable` and `move()` captured `f_`. Or, for example, we could store `f` 
> > value and `pid` in a structure, that will be captured by a 
> > `std::shared_ptr`.

We can't assume that lambda is called only once here, so we must make a copy. 
Avoiding this last copy is doable, but I left it out of scope of this review.


- Dmitry


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


On July 17, 2017, 11:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 17, 2017, 11:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7713
> https://issues.apache.org/jira/browse/MESOS-7713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, ::someMethod, param1, param2));
> 
> For the objects that do not support move semantics (e.g. protobuf messages), 
> number of copies is reduced from 8-10 to 6. If move semantics is supported, 
> then number of copies is reduced from 6-7 to 1 if parameter is passed with 
> std::move, or 2 otherwise.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/defer.hpp 
> 7f3369e723cb244e97930621cbba89cf7873567d 
>   3rdparty/libprocess/include/process/deferred.hpp 
> e446621be11ac51f5f91c417cc8975e363c2f715 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/future.hpp 
> cce950509f58022e79bb51a6e72ea1a005b9cb50 
>   3rdparty/libprocess/include/process/http.hpp 
> f637999174d92a98208b5fc49a65f9929efb11a0 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> Number of copies was checked by using defer to subscribe process for Future 
> callbacks, and passing parameters that count number of copies made.
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

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

(Updated July 17, 2017, 11:42 a.m.)


Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
Michael Park.


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


Repository: mesos


Description (updated)
---

This reduces number of copies made for each parameter in a code like this:
future.then(defer(pid, ::someMethod, param1, param2));

For the objects that do not support move semantics (e.g. protobuf messages), 
number of copies is reduced from 8-10 to 6. If move semantics is supported, 
then number of copies is reduced from 6-7 to 1 if parameter is passed with 
std::move, or 2 otherwise.


Diffs (updated)
-

  3rdparty/libprocess/include/process/defer.hpp 
7f3369e723cb244e97930621cbba89cf7873567d 
  3rdparty/libprocess/include/process/deferred.hpp 
e446621be11ac51f5f91c417cc8975e363c2f715 
  3rdparty/libprocess/include/process/dispatch.hpp 
3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
  3rdparty/libprocess/include/process/future.hpp 
cce950509f58022e79bb51a6e72ea1a005b9cb50 
  3rdparty/libprocess/include/process/http.hpp 
f637999174d92a98208b5fc49a65f9929efb11a0 


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

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


Testing
---

make check

Number of copies was checked by using defer to subscribe process for Future 
callbacks, and passing parameters that count number of copies made.


Thanks,

Dmitry Zhuk

Re: Review Request 60002: Added ENUM preprocessor macro.

[Dmitry Zhuk]

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

(Updated July 17, 2017, 11:41 a.m.)


Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
Michael Park.


Summary (updated)
-

Added ENUM preprocessor macro.


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


Repository: mesos


Description (updated)
---

This allows using templates to generate parameters lists, for example:
#define MOVE_TEMPLATE(Z, N, DATA) std::move(DATA ## N)

Then it can be used as
call(ENUM(2, MOVE_TEMPLATE, p));
which is expanded as
call(std::move(p1), std::move(p2));


Diffs (updated)
-

  3rdparty/stout/include/stout/preprocessor.hpp 
48d63318712d454b118db64af11c694da92bca6a 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 60215: WIP: Logged offer ids of sent offers.

[Till Toenshoff]

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




src/master/master.cpp
Lines 7311 (patched)


I am not sure this wont grow into output sizes that become significant load 
for the logging system. How about doing the offer detail logging on VLOG level 
2?


- Till Toenshoff


On June 19, 2017, 11:08 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60215/
> ---
> 
> (Updated June 19, 2017, 11:08 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Bannier, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To simplify debugging mesos clusters, it can be valuable to know,
> when a specific offer has been sent to a framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 287a5b71bc61949648ac0edff7668f217357a054 
> 
> 
> Diff: https://reviews.apache.org/r/60215/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 60214: Logged when an offer is removed.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On June 19, 2017, 10:42 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60214/
> ---
> 
> (Updated June 19, 2017, 10:42 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Bannier, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 287a5b71bc61949648ac0edff7668f217357a054 
> 
> 
> Diff: https://reviews.apache.org/r/60214/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>